[ovs-dev] [PATCH v8 1/4] conntrack: handle already natted packets

[Dumitru Ceara]

On 7/6/21 3:03 PM, Paolo Valerio wrote:
> when a packet gets dnatted and then recirculated, it could be possible
> that it matches another rule that performs another nat action.
> The kernel datapath handles this situation turning to a no-op the
> second nat action, so natting only once the packet.  In the userspace
> datapath instead, when the ct action gets executed, an initial lookup
> of the translated packet fails to retrieve the connection related to
> the packet, leading to the creation of a new entry in ct for the src
> nat action with a subsequent failure of the connection establishment.
> 
> with the following flows:
> 
> table=0,priority=30,in_port=1,ip,nw_dst=192.168.2.100,actions=ct(commit,nat(dst=10.1.1.2:80),table=1)
> table=0,priority=20,in_port=2,ip,actions=ct(nat,table=1)
> table=0,priority=10,ip,actions=resubmit(,2)
> table=0,priority=10,arp,actions=NORMAL
> table=0,priority=0,actions=drop
> table=1,priority=5,ip,actions=ct(commit,nat(src=10.1.1.240),table=2)
> table=2,in_port=ovs-l0,actions=2
> table=2,in_port=ovs-r0,actions=1
> 
> establishing a connection from 10.1.1.1 to 192.168.2.100 the outcome is:
> 
> tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.240,sport=80,dport=4000),protoinfo=(state=ESTABLISHED)
> tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED)
> 
> with this patch applied the outcome is:
> 
> tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED)
> 
> The patch performs, for already natted packets, a lookup of the
> reverse key in order to retrieve the related entry, it also adds a
> test case that besides testing the scenario ensures that the other ct
> actions are executed.
> 
> Reported-by: Dumitru Ceara <dceara at redhat.com>
> Signed-off-by: Paolo Valerio <pvalerio at redhat.com>
> ---

Hi Paolo,

>  lib/conntrack.c         |   32 +++++++++++++++++++++++++++++++-
>  tests/system-traffic.at |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 2e803cac6..b6a35edc0 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -47,6 +47,7 @@ COVERAGE_DEFINE(conntrack_full);
>  COVERAGE_DEFINE(conntrack_long_cleanup);
>  COVERAGE_DEFINE(conntrack_l3csum_err);
>  COVERAGE_DEFINE(conntrack_l4csum_err);
> +COVERAGE_DEFINE(conntrack_lookup_natted_miss);
>  
>  struct conn_lookup_ctx {
>      struct conn_key key;
> @@ -1282,6 +1283,34 @@ process_one_fast(uint16_t zone, const uint32_t *setmark,
>      }
>  }
>  
> +static void
> +initial_conn_lookup(struct conntrack *ct, struct conn_lookup_ctx *ctx,
> +                    long long now, bool natted)
> +{
> +    if (natted) {
> +        /* if the packet has been already natted (e.g. a previous

Really tiny nit: s/if/If

I guess this can be easily fixed by maintainers at apply time.

I tested this patch (also with OVN) and ran OVS and OVN self and system
tests.  The results look good to me, thanks!

Acked-by: Dumitru Ceara <dceara at redhat.com>

Regards,
Dumitru