[ovs-dev] [PATCH] conntrack: add generic IP protocol support

Eelco Chaudron echaudro at redhat.com
Thu Sep 17 08:43:23 UTC 2020



On 16 Sep 2020, at 17:45, Aaron Conole wrote:

> Eelco Chaudron <echaudro at redhat.com> writes:
>
>> Currently, userspace conntrack only tracks TCP, UDP, and ICMP, and 
>> all
>> other IP protocols are discarded, and the +inv state is returned. 
>> This
>> is not in line with the kernel conntrack. Where if no L4 information 
>> can
>> be extracted it's treated as generic L3. The change below mimics the
>> behavior of the kernel.
>>
>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>>  lib/conntrack-private.h |    3 +++
>>  lib/conntrack.c         |   27 ++++++++++++++++++---------
>>  tests/system-traffic.at |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 50 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
>> index 9a8ca39..85329e8 100644
>> --- a/lib/conntrack-private.h
>> +++ b/lib/conntrack-private.h
>> @@ -59,6 +59,9 @@ struct conn_key {
>>      uint8_t nw_proto;
>>  };
>>
>> +/* Verify that nw_proto stays uint8_t as it's used to index into 
>> l4_protos[] */
>> +BUILD_ASSERT_DECL(sizeof(((struct conn_key *)0)->nw_proto) == 
>> sizeof(uint8_t));
>> +
>>  /* This is used for alg expectations; an expectation is a
>>   * context created in preparation for establishing a data
>>   * connection. The expectation is created by the control
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 0cbc8f6..cb42f26 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -143,12 +143,7 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx 
>> *ctx,
>>  static void
>>  expectation_clean(struct conntrack *ct, const struct conn_key 
>> *master_key);
>>
>> -static struct ct_l4_proto *l4_protos[] = {
>> -    [IPPROTO_TCP] = &ct_proto_tcp,
>> -    [IPPROTO_UDP] = &ct_proto_other,
>> -    [IPPROTO_ICMP] = &ct_proto_icmp4,
>> -    [IPPROTO_ICMPV6] = &ct_proto_icmp6,
>> -};
>> +static struct ct_l4_proto *l4_protos[UINT8_MAX + 1];
>>
>>  static void
>>  handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx 
>> *ctx,
>> @@ -296,6 +291,7 @@ ct_print_conn_info(const struct conn *c, const 
>> char *log_msg,
>>  struct conntrack *
>>  conntrack_init(void)
>>  {
>> +    static struct ovsthread_once setup_l4_once = 
>> OVSTHREAD_ONCE_INITIALIZER;
>>      struct conntrack *ct = xzalloc(sizeof *ct);
>>
>>      ovs_rwlock_init(&ct->resources_lock);
>> @@ -322,6 +318,18 @@ conntrack_init(void)
>>      ct->clean_thread = ovs_thread_create("ct_clean", 
>> clean_thread_main, ct);
>>      ct->ipf = ipf_init();
>>
>> +    /* Initialize the l4 protocols. */
>> +    if (ovsthread_once_start(&setup_l4_once)) {
>> +        for (int i = 0; i < ARRAY_SIZE(l4_protos); i++) {
>> +            l4_protos[i] = &ct_proto_other;
>> +        }
>> +        /* IPPROTO_UDP uses ct_proto_other, so no need to initialize 
>> it. */
>> +        l4_protos[IPPROTO_TCP] = &ct_proto_tcp;
>> +        l4_protos[IPPROTO_ICMP] = &ct_proto_icmp4;
>> +        l4_protos[IPPROTO_ICMPV6] = &ct_proto_icmp6;
>> +
>> +        ovsthread_once_done(&setup_l4_once);
>> +    }
>>      return ct;
>>  }
>>
>> @@ -1971,7 +1979,8 @@ extract_l4(struct conn_key *key, const void 
>> *data, size_t size, bool *related,
>>                  validate_checksum))
>>                 && extract_l4_icmp6(key, data, size, related);
>>      } else {
>> -        return false;
>> +        /* For all other protocols we do not have L4 keys, so keep 
>> them zero */
>> +        return true;
>>      }
>
> Maybe eliminate the entire "} else {" branch to improve readability.
>
>  -    } else {
>  -        return false;
>  +    /* For all other protocols we do not have L4 keys, so keep them 
> zero */
>  +    return true;
>  -    }
>

I agree looks nicer, got blinded by the change only. I’ll sent out a 
V2.

<SNIP>



More information about the dev mailing list