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

Paolo Valerio pvalerio at redhat.com
Fri Jun 25 13:58:29 UTC 2021


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

I see your point and I understand it.
AFAICS, it doesn't seem to be an easy way to do it.

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

ACK.

> Regards,
> Dumitru



More information about the dev mailing list