On 3/18/20 2:57 PM, Ilya Maximets wrote: > On 3/5/20 12:28 PM, Dumitru Ceara wrote: >> When a new conntrack zone is entered, the ct_state field is zeroed in >> order to avoid using state information from different zones. >> >> One such scenario is when a packet is double NATed. Assuming two zones >> and 3 flows performing the following actions in order on the packet: >> 1. ct(zone=5,nat), recirc >> 2. ct(zone=1), recirc >> 3. ct(zone=1,nat) >> >> If at step #1 the packet matches an existing NAT entry, it will get >> translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT. >> At step #2 the new tuple might match an existing connection and >> pkt->md.ct_zone is set to 1. >> If at step #3 the packet matches an existing NAT entry in zone 1, >> handle_nat() will be called to perform the translation but it will >> return early because the packet's zone matches the conntrack zone and >> the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the >> translations in zone 5. >> >> In order to reliably detect when a packet enters a new conntrack zone >> we also need to make sure that the pkt->md.ct_zone is properly >> initialized if pkt->md.ct_state is non-zero. This already happens for >> most cases. The only exception is when matched conntrack connection is >> of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To >> cover this path we now call write_ct_md() in that case too. Remove >> setting the CS_TRACKED flag as in this case as it will be done by the >> new call to write_ct_md(). >> >> Also, the error path above seems hard to hit as it would've caused a >> crash due to dereferencing a NULL pointer when calling: >> 'ct_print_conn_info(conn, log_msg, VLL_INFO, true, true)'. This patch >> changes the call to log the 'rev_conn' info instead. >> >> CC: Darrell Ball <dlu...@gmail.com> >> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.") >> Signed-off-by: Dumitru Ceara <dce...@redhat.com> >> >> --- >> V2: >> - Address Ilya's comments: >> - revert changes to pkt_metadata_init(). >> - update ct_state in process_one() only if ct_state is already >> non-zero. >> - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state >> is non-zero. >> - Fix NULL pointer dereference in process_one() if conn_type is >> CT_CONN_TYPE_UN_NAT and master conn is not found. >> --- > > 'Fixes' tag seems a bit strange to me. I think it should be: > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") > > Regarding the issue. I've spent some time exploring the conntrack code > and also researching the original patch that introduced this code. > The issue was raised during the review of the original patch 286de2729955 > by Daniele: https://patchwork.ozlabs.org/patch/743108/ > And Darrell said that he actually had the similar code that clears ct_state > during zone transition at the beginning of process_one(). But, he decided > that most of such issues are likely configuration bugs that should by raised > to user with warnings. > However, such warnings was never introduced and since this causes a real > issue in OVN we should actually have this clearing of conntrack state on > zone transitioning. > > Acked-by: Ilya Maximets <i.maxim...@ovn.org> > > Darrell, Ben, I'd like to have some comments on this from you since I'm > not an expert in this area. Otherwise, I'm going to apply this patch on > next week. > > Best regards, Ilya Maximets. >
Hi Ilya, Thanks for the review. I'll send a v3 with updated 'Fixes' tag and your ack before next week unless there are more concerns from other reviewers. Thanks, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev