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

Dumitru Ceara dceara at redhat.com
Fri Jun 25 12:08:40 UTC 2021


On 6/25/21 2:01 PM, Paolo Valerio wrote:
> 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.

I'm not sure, won't this create horrible to debug bugs when packets get
forwarded in an unexpected way?  Setting it as invalid isn't good enough
in my opinion because there might be flows later in the pipeline that
perform actions (other than drop) on packets with ct_state +inv.

The problem I have (because I don't know the conntrack code) is that I
see no easy way to drop the packet.

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

I would prefer a coverage counter, at least to have an indication of a
problem "somewhere" in conntrack.

Regards,
Dumitru



More information about the dev mailing list