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

Ilya Maximets i.maximets at ovn.org
Wed Mar 18 13:57:53 UTC 2020


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.


More information about the dev mailing list