diff mbox series

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

Message ID 20201012113812.15711.15012.stgit@netdev64
State Accepted
Headers show
Series [ovs-dev,v3] conntrack: add generic IP protocol support | expand

Commit Message

Eelco Chaudron Oct. 12, 2020, 11:38 a.m. UTC
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@redhat.com>
---
v3: Added NEWS item for this feature.
    Small style fixes suggested by Ilya.
v2: Small style fix suggested by Aaron Conole.

 NEWS                    |    4 ++++
 lib/conntrack-private.h |    3 +++
 lib/conntrack.c         |   29 +++++++++++++++++++----------
 tests/system-traffic.at |   29 +++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 10 deletions(-)

Comments

Flavio Leitner Dec. 16, 2020, 8:04 p.m. UTC | #1
On Mon, Oct 12, 2020 at 07:38:38AM -0400, Eelco Chaudron wrote:
> 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@redhat.com>
> ---

This patch conflicts with NEWS file, otherwise:

Acked-by: Flavio Leitner <fbl@sysclose.org>
Eelco Chaudron Dec. 17, 2020, 8:20 a.m. UTC | #2
On 16 Dec 2020, at 21:04, Flavio Leitner wrote:

> On Mon, Oct 12, 2020 at 07:38:38AM -0400, Eelco Chaudron wrote:
>> 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@redhat.com>
>> ---
>
> This patch conflicts with NEWS file, otherwise:
>
> Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks Flavio, Ilya do you want a new revision with the updated NEWS 
file, or will you fix it at merge?

//Eelco
Ilya Maximets Dec. 17, 2020, 11:25 a.m. UTC | #3
On 12/17/20 9:20 AM, Eelco Chaudron wrote:
> 
> 
> On 16 Dec 2020, at 21:04, Flavio Leitner wrote:
> 
>> On Mon, Oct 12, 2020 at 07:38:38AM -0400, Eelco Chaudron wrote:
>>> 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@redhat.com>
>>> ---
>>
>> This patch conflicts with NEWS file, otherwise:
>>
>> Acked-by: Flavio Leitner <fbl@sysclose.org>
> 
> Thanks Flavio, Ilya do you want a new revision with the updated NEWS file, or will you fix it at merge?

I can resolve that conflict.  No problem.

Best regards, Ilya Maximets.
Ilya Maximets Dec. 21, 2020, 11:08 p.m. UTC | #4
On 12/16/20 9:04 PM, Flavio Leitner wrote:
> On Mon, Oct 12, 2020 at 07:38:38AM -0400, Eelco Chaudron wrote:
>> 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@redhat.com>
>> ---
> 
> This patch conflicts with NEWS file, otherwise:
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> 

Applied.  Thanks!

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 4619e73..f63ed1a 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,10 @@  Post-v2.14.0
      * Removed support for vhost-user dequeue zero-copy.
    - The environment variable OVS_UNBOUND_CONF, if set, is now used
      as the DNS resolver's (unbound) configuration file.
+   - Userspace datapath:
+     * Add generic IP protocol support to conntrack. With this change, all
+       none UDP, TCP, and ICMP traffic will be treated as general L3
+       traffic, i.e. using 3 tupples.
 
 
 v2.14.0 - 17 Aug 2020
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 3434753..4b1da03 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(MEMBER_SIZEOF(struct conn_key, 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 f42ba4b..66e43b3 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -146,12 +146,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,
@@ -293,6 +288,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);
@@ -320,6 +316,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;
 }
 
@@ -1982,9 +1990,10 @@  extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
         return (!related || check_l4_icmp6(key, data, size, l3,
                 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;
 }
 
 static bool
@@ -2267,8 +2276,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
index 02f0e27..db081d4 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2333,6 +2333,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()