[ovs-dev] [PATCH v2] conntrack: Reset ct_state when entering a new zone.

[Dumitru Ceara]

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 <dlu998 at gmail.com>
>> Fixes: 967bb5c5cd90 ("conntrack: Add rcu support.")
>> Signed-off-by: Dumitru Ceara <dceara at 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.maximets at 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