On 3/19/20 9:12 AM, Dumitru Ceara wrote: > 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 <[email protected]> >>> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.") >>> Signed-off-by: Dumitru Ceara <[email protected]> >>> >>> --- >>> 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 <[email protected]> >> >> 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 >
V3 available at: https://patchwork.ozlabs.org/patch/1258393/ Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
