[ovs-dev] [PATCH v7 1/4] conntrack: handle already natted packets
Paolo Valerio
pvalerio at redhat.com
Tue Jun 29 16:04:21 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.
>
>>
>> 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).
> Regards,
> Dumitru
More information about the dev
mailing list