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

[Dumitru Ceara]

On 6/29/21 6:04 PM, Paolo Valerio wrote:
> Dumitru Ceara <dceara at redhat.com> writes:
> 
>> 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.
>>
> 
> Speaking with Gaëtan, he brought a good point we didn't directly
> discuss about in this thread, but that needs to be considered.
> 
> Removing a conn (e.g. flushing) during the recirculation would lead to a
> lookup failure. The counter would then count those cases as well
> potentially adding noise (if we consider it as an indication of
> potential problems).
> 

Thanks for the follow up!  I see, ok; I still think it's worth having
the counter as I assume the valid cases (e.g., flushing) don't happen
too often.  Like that we'd still have some indication that there might
be a problem.

In any case, I'll leave it up to you, Gaëtan, and other OVS reviewers to
decide if it's useful to have the counter or not.

If you fix the indentation nit I mentioned initially, and regardless of
adding a new counter or not, feel free to add my ack to next revision:

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

Regards,
Dumitru