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

[Aaron Conole]

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;
 -    }


>  }
>  
> @@ -2255,8 +2264,8 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
>                conn->nat_info->nat_action & NAT_ACTION_SRC_PORT
>            ? true : false;
>      union ct_addr first_addr = ct_addr;
> -    bool pat_enabled = conn->key.nw_proto != IPPROTO_ICMP &&
> -                       conn->key.nw_proto != IPPROTO_ICMPV6;
> +    bool pat_enabled = conn->key.nw_proto == IPPROTO_TCP ||
> +                       conn->key.nw_proto == IPPROTO_UDP;
>  
>      while (true) {
>          if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at

Nice, thanks for this.

> index 3ed03d9..b7aca93 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2341,6 +2341,35 @@ NXST_FLOW reply:
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([conntrack - generic IP protocol])
> +CHECK_CONNTRACK()
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg ofproto_dpif_upcall:dbg])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_DATA([flows.txt], [dnl
> +table=0, priority=1,action=drop
> +table=0, priority=10,arp,action=normal
> +table=0, priority=100,ip,action=ct(table=1)
> +table=1, priority=100,in_port=1,ip,ct_state=+trk+new,action=ct(commit)
> +table=1, priority=100,in_port=1,ct_state=+trk+est,action=normal
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=01005e00001200005e000101080045c0002800000000ff7019cdc0a8001ee0000012210164010001ba52c0a800010000000000000000000000000000 actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=192\.168\.0\.30,"], [], [dnl
> +112,orig=(src=192.168.0.30,dst=224.0.0.18,sport=0,dport=0),reply=(src=224.0.0.18,dst=192.168.0.30,sport=0,dport=0)
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([conntrack - ICMP related])
>  AT_SKIP_IF([test $HAVE_NC = no])
>  CHECK_CONNTRACK()
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev