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

Dumitru Ceara dceara at redhat.com
Thu Mar 19 19:24:37 UTC 2020


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 <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
> 

V3 available at: https://patchwork.ozlabs.org/patch/1258393/

Thanks,
Dumitru



More information about the dev mailing list