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

[Paolo Valerio]

Dumitru Ceara <dceara at redhat.com> writes:

> On 6/21/21 12:06 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,
>
> Thanks for the patch!  I tested it and it works fine for OVN.  I have a
> few comments/questions below.
>

Thanks for the test and for the review.

>>  lib/conntrack.c         |   30 +++++++++++++++++++++++++++++-
>>  tests/system-traffic.at |   35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 64 insertions(+), 1 deletion(-)
>> 
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 99198a601..7e8b16a3e 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1281,6 +1281,33 @@ 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)
>
> Nit: indentation.
>

ACK

>> +{
>> +    bool found;
>> +
>> +    if (natted) {
>> +        /* if the packet has been already natted (e.g. a previous
>> +         * action took place), retrieve it performing a lookup of its
>> +         * reverse key. */
>> +        conn_key_reverse(&ctx->key);
>> +    }
>> +
>> +    found = conn_key_lookup(ct, &ctx->key, ctx->hash,
>> +                            now, &ctx->conn, &ctx->reply);
>> +    if (natted) {
>> +        if (OVS_LIKELY(found)) {
>> +            ctx->reply = !ctx->reply;
>> +            ctx->key = ctx->reply ? ctx->conn->rev_key : ctx->conn->key;
>> +            ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
>> +        } else {
>> +            /* in case of failure restore the initial key. */
>> +            conn_key_reverse(&ctx->key);
>
> Can the lookup actually fail?  I mean, if the packet was natted, there
> must have been a connection on which it got natted.  Anyway, I think we
> should probably also increment a coverage counter.  I guess dropping
> such packets would be hard, right?
>

I agree, it should not fail. If I'm not missing something, if it
happens, it should be because there's been a problem somewhere else
(e.g. a polluted ct_state value or more in general an unexpected
scenario). For this reason, I think it's better not to drop it or even
set it as invalid.

Yes, the coverage counter gives more meaning to the else branch.
Alternatively, we could probably even remove it. I would leave the NULL
check or equivalent.

I have no strong preference.
WDYT?

> Thanks,
> Dumitru