[ovs-dev] [PATCH] conntrack: Reset ct_state when entering a new zone.

[Ilya Maximets]

On 3/4/20 2:01 PM, Dumitru Ceara wrote:
> On 1/30/20 3:16 PM, Dumitru Ceara wrote:
>> When a new conntrack zone is entered, the ct_state field is zeroed in
>> order to avoid using state information from different zones.
>>
>> One such scenario is when a packet is double NATed. Assuming two zones
>> and 3 flows performing the following actions in order on the packet:
>> 1. ct(zone=5,nat), recirc
>> 2. ct(zone=1), recirc
>> 3. ct(zone=1,nat)
>>
>> If at step #1 the packet matches an existing NAT entry, it will get
>> translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT.
>> At step #2 the new tuple might match an existing connection and
>> pkt->md.ct_zone is set to 1.
>> If at step #3 the packet matches an existing NAT entry in zone 1,
>> handle_nat() will be called to perform the translation but it will
>> return early because the packet's zone matches the conntrack zone and
>> the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the
>> translations in zone 5.
>>
>> In order to reliably detect when a packet enters a new conntrack zone
>> we also need to zero out the pkt->md.ct_zone field when initializing
>> metadata in pkt_metadata_init().
>>
>> CC: Darrell Ball <dlu998 at gmail.com>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>  lib/conntrack.c | 5 +++++
>>  lib/packets.h   | 5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index ff5a894..e4d934a 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1277,6 +1277,11 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
>>              const struct nat_action_info_t *nat_action_info,
>>              ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper)
>>  {
>> +    /* Reset ct_state whenever entering a new zone. */
>> +    if (pkt->md.ct_zone != zone) {
>> +        pkt->md.ct_state = 0;
>> +    }
>> +
>>      bool create_new_conn = false;
>>      conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply);
>>      struct conn *conn = ctx->conn;
>> diff --git a/lib/packets.h b/lib/packets.h
>> index 5d7f82c..fae64bb 100644
>> --- a/lib/packets.h
>> +++ b/lib/packets.h
>> @@ -161,6 +161,11 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port)
>>       */
>>      memset(md, 0, offsetof(struct pkt_metadata, ct_orig_tuple_ipv6));
>>  
>> +    /* Explicitly zero out ct_zone in order to be able to properly determine
>> +     * when a packet enters a new conntrack zone.
>> +     */
>> +    md->ct_zone = 0;

I'm not an expert in conntrack, but I'm bothered about this change in
init function.  This function works for every single packet and any
modification might significantly affect performance.

There is an assumption that conntrack fields of packet metadata are
never used if ct_state is zero.  So, every user of these fields must
be sure that ct_state is correctly initialized.
Will it work if we'll change your 'if' statement in 'process_one'
function to something like:

    if (pkt->md.ct_state && pkt->md.ct_zone != zone) {
        pkt->md.ct_state = 0;
    }

and will not change metadata initialization code?

>> +
>>      /* It can be expensive to zero out all of the tunnel metadata. However,
>>       * we can just zero out ip_dst and the rest of the data will never be
>>       * looked at. */
>>
> 
> Hi,
> 
> Just a reminder about this patch.
> 
> OVN LB harpinning system tests on OVN master and OVN-20.03 with OVS
> userspace datapath fail because of the issue this patch addresses.
> 
> Thanks,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>