diff mbox series

[ovs-dev,v5,8/9] datapath: Add support for conntrack timeout policy

Message ID 1567030469-120137-9-git-send-email-yihung.wei@gmail.com
State Accepted
Commit ebe62ec1b9157bfdcc13288e38e67c05a41dc293
Headers show
Series Support zone-based conntrack timeout policy | expand

Commit Message

Yi-Hung Wei Aug. 28, 2019, 10:14 p.m. UTC
This patch adds support for specifying a timeout policy for a
connection in connection tracking system in kernel datapath.
The timeout policy will be attached to a connection when the
connection is committed to conntrack.

This patch introduces a new odp field OVS_CT_ATTR_TIMEOUT in the
ct action that specifies the timeout policy in the datapath.
In the following patch, during the upcall process, the vswitchd will use
the ct_zone to look up the corresponding timeout policy and fill
OVS_CT_ATTR_TIMEOUT if it is available.

The datapath code is from the following two net-next upstream commits.

Upstream commit:
commit 06bd2bdf19d2f3d22731625e1a47fa1dff5ac407
Author: Yi-Hung Wei <yihung.wei@gmail.com>
Date:   Tue Mar 26 11:31:14 2019 -0700

    openvswitch: Add timeout support to ct action

    Add support for fine-grain timeout support to conntrack action.
    The new OVS_CT_ATTR_TIMEOUT attribute of the conntrack action
    specifies a timeout to be associated with this connection.
    If no timeout is specified, it acts as is, that is the default
    timeout for the connection will be automatically applied.

    Example usage:
    $ nfct timeout add timeout_1 inet tcp syn_sent 100 established 200
    $ ovs-ofctl add-flow br0 in_port=1,ip,tcp,action=ct(commit,timeout=timeout_1)

    CC: Pravin Shelar <pshelar@ovn.org>
    CC: Pablo Neira Ayuso <pablo@netfilter.org>
    Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

commit 6d670497e01803b486aa72cc1a718401ab986896
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Tue Apr 2 09:53:14 2019 +0300

    openvswitch: use after free in __ovs_ct_free_action()

    We free "ct_info->ct" and then use it on the next line when we pass it
    to nf_ct_destroy_timeout().  This patch swaps the order to avoid the use
    after free.

    Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
    Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
    Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 datapath/conntrack.c                              | 30 ++++++++++++++++++++++-
 datapath/linux/compat/include/linux/openvswitch.h |  4 +++
 lib/dpif-netdev.c                                 |  4 +++
 lib/odp-util.c                                    | 29 +++++++++++++++++++---
 tests/odp.at                                      |  1 +
 5 files changed, 63 insertions(+), 5 deletions(-)

Comments

0-day Robot Aug. 28, 2019, 11:22 p.m. UTC | #1
Bleep bloop.  Greetings Yi-Hung Wei, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Dan Carpenter <dan.carpenter@oracle.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Yi-Hung Wei <yihung.wei@gmail.com>
Lines checked: 286, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 292febb3c83e..f85d0a2572f6 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -29,6 +29,7 @@ 
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_labels.h>
 #include <net/netfilter/nf_conntrack_seqadj.h>
+#include <net/netfilter/nf_conntrack_timeout.h>
 #include <net/netfilter/nf_conntrack_zones.h>
 #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
 #include <net/ipv6_frag.h>
@@ -86,6 +87,7 @@  struct ovs_conntrack_info {
 	u32 eventmask;              /* Mask of 1 << IPCT_*. */
 	struct md_mark mark;
 	struct md_labels labels;
+	char timeout[CTNL_TIMEOUT_NAME_MAX];
 #ifdef CONFIG_NF_NAT_NEEDED
 	struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. */
 #endif
@@ -1519,6 +1521,8 @@  static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
 #endif
 	[OVS_CT_ATTR_EVENTMASK]	= { .minlen = sizeof(u32),
 				    .maxlen = sizeof(u32) },
+	[OVS_CT_ATTR_TIMEOUT] = { .minlen = 1,
+				  .maxlen = CTNL_TIMEOUT_NAME_MAX },
 };
 
 static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
@@ -1604,6 +1608,15 @@  static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 			info->have_eventmask = true;
 			info->eventmask = nla_get_u32(a);
 			break;
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+		case OVS_CT_ATTR_TIMEOUT:
+			memcpy(info->timeout, nla_data(a), nla_len(a));
+			if (!memchr(info->timeout, '\0', nla_len(a))) {
+				OVS_NLERR(log, "Invalid conntrack helper");
+				return -EINVAL;
+			}
+			break;
+#endif
 
 		default:
 			OVS_NLERR(log, "Unknown conntrack attr (%d)",
@@ -1685,6 +1698,14 @@  int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
 		OVS_NLERR(log, "Failed to allocate conntrack template");
 		return -ENOMEM;
 	}
+
+	if (ct_info.timeout[0]) {
+		if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto,
+				      ct_info.timeout))
+			pr_info_ratelimited("Failed to associated timeout "
+					    "policy `%s'\n", ct_info.timeout);
+	}
+
 	if (helper) {
 		err = ovs_ct_add_helper(&ct_info, helper, key, log);
 		if (err)
@@ -1809,6 +1830,10 @@  int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
 	if (ct_info->have_eventmask &&
 	    nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))
 		return -EMSGSIZE;
+	if (ct_info->timeout[0]) {
+		if (nla_put_string(skb, OVS_CT_ATTR_TIMEOUT, ct_info->timeout))
+			return -EMSGSIZE;
+	}
 
 #ifdef CONFIG_NF_NAT_NEEDED
 	if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
@@ -1830,8 +1855,11 @@  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
 {
 	if (ct_info->helper)
 		nf_conntrack_helper_put(ct_info->helper);
-	if (ct_info->ct)
+	if (ct_info->ct) {
+		if (ct_info->timeout[0])
+			nf_ct_destroy_timeout(ct_info->ct);
 		nf_ct_tmpl_free(ct_info->ct);
+	}
 }
 
 #if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 65a003a62cf5..7b16b1d5bfe0 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -801,6 +801,7 @@  struct ovs_action_push_tnl {
  * be received on NFNLGRP_CONNTRACK_NEW and NFNLGRP_CONNTRACK_DESTROY groups,
  * respectively.  Remaining bits control the changes for which an event is
  * delivered on the NFNLGRP_CONNTRACK_UPDATE group.
+ * @OVS_CT_ATTR_TIMEOUT: Variable length string defining conntrack timeout.
  */
 enum ovs_ct_attr {
 	OVS_CT_ATTR_UNSPEC,
@@ -813,6 +814,9 @@  enum ovs_ct_attr {
 	OVS_CT_ATTR_NAT,        /* Nested OVS_NAT_ATTR_* */
 	OVS_CT_ATTR_FORCE_COMMIT,  /* No argument */
 	OVS_CT_ATTR_EVENTMASK,  /* u32 mask of IPCT_* events. */
+	OVS_CT_ATTR_TIMEOUT,    /* Associate timeout with this connection for
+				 * fine-grain timeout tuning. */
+
 	__OVS_CT_ATTR_MAX
 };
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2079e368fb52..7240a3e6f3c8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7204,6 +7204,10 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
                 /* Silently ignored, as userspace datapath does not generate
                  * netlink events. */
                 break;
+            case OVS_CT_ATTR_TIMEOUT:
+                /* Userspace datapath does not support customized timeout
+                 * policy yet. */
+                break;
             case OVS_CT_ATTR_NAT: {
                 const struct nlattr *b_nest;
                 unsigned int left_nest;
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 84ea4c148f11..28c3209031ce 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -930,6 +930,8 @@  static const struct nl_policy ovs_conntrack_policy[] = {
     [OVS_CT_ATTR_HELPER] = { .type = NL_A_STRING, .optional = true,
                              .min_len = 1, .max_len = 16 },
     [OVS_CT_ATTR_NAT] = { .type = NL_A_UNSPEC, .optional = true },
+    [OVS_CT_ATTR_TIMEOUT] = { .type = NL_A_STRING, .optional = true,
+                              .min_len = 1, .max_len = 32 },
 };
 
 static void
@@ -941,7 +943,7 @@  format_odp_conntrack_action(struct ds *ds, const struct nlattr *attr)
         ovs_32aligned_u128 mask;
     } *label;
     const uint32_t *mark;
-    const char *helper;
+    const char *helper, *timeout;
     uint16_t zone;
     bool commit, force;
     const struct nlattr *nat;
@@ -957,10 +959,12 @@  format_odp_conntrack_action(struct ds *ds, const struct nlattr *attr)
     mark = a[OVS_CT_ATTR_MARK] ? nl_attr_get(a[OVS_CT_ATTR_MARK]) : NULL;
     label = a[OVS_CT_ATTR_LABELS] ? nl_attr_get(a[OVS_CT_ATTR_LABELS]): NULL;
     helper = a[OVS_CT_ATTR_HELPER] ? nl_attr_get(a[OVS_CT_ATTR_HELPER]) : NULL;
+    timeout = a[OVS_CT_ATTR_TIMEOUT] ?
+                nl_attr_get(a[OVS_CT_ATTR_TIMEOUT]) : NULL;
     nat = a[OVS_CT_ATTR_NAT];
 
     ds_put_format(ds, "ct");
-    if (commit || force || zone || mark || label || helper || nat) {
+    if (commit || force || zone || mark || label || helper || timeout || nat) {
         ds_put_cstr(ds, "(");
         if (commit) {
             ds_put_format(ds, "commit,");
@@ -983,6 +987,9 @@  format_odp_conntrack_action(struct ds *ds, const struct nlattr *attr)
         if (helper) {
             ds_put_format(ds, "helper=%s,", helper);
         }
+        if (timeout) {
+            ds_put_format(ds, "timeout=%s", timeout);
+        }
         if (nat) {
             format_odp_ct_nat(ds, nat);
         }
@@ -1909,8 +1916,8 @@  parse_conntrack_action(const char *s_, struct ofpbuf *actions)
     const char *s = s_;
 
     if (ovs_scan(s, "ct")) {
-        const char *helper = NULL;
-        size_t helper_len = 0;
+        const char *helper = NULL, *timeout = NULL;
+        size_t helper_len = 0, timeout_len = 0;
         bool commit = false;
         bool force_commit = false;
         uint16_t zone = 0;
@@ -1987,6 +1994,16 @@  find_end:
                     s += helper_len;
                     continue;
                 }
+                if (ovs_scan(s, "timeout=%n", &n)) {
+                    s += n;
+                    timeout_len = strcspn(s, delimiters_end);
+                    if (!timeout_len || timeout_len > 31) {
+                        return -EINVAL;
+                    }
+                    timeout = s;
+                    s += timeout_len;
+                    continue;
+                }
 
                 n = scan_ct_nat(s, &nat_params);
                 if (n > 0) {
@@ -2027,6 +2044,10 @@  find_end:
             nl_msg_put_string__(actions, OVS_CT_ATTR_HELPER, helper,
                                 helper_len);
         }
+        if (timeout) {
+            nl_msg_put_string__(actions, OVS_CT_ATTR_TIMEOUT, timeout,
+                                timeout_len);
+        }
         if (have_nat) {
             nl_msg_put_ct_nat(&nat_params, actions);
         }
diff --git a/tests/odp.at b/tests/odp.at
index 8e4ba4615548..3ab9ad62dda2 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -345,6 +345,7 @@  ct(commit,mark=0xa0a0a0a0/0xfefefefe)
 ct(commit,label=0x1234567890abcdef1234567890abcdef/0xf1f2f3f4f5f6f7f8f9f0fafbfcfdfeff)
 ct(commit,helper=ftp)
 ct(commit,helper=tftp)
+ct(commit,timeout=ovs_tp_1_tcp4)
 ct(nat)
 ct(commit,nat(src))
 ct(commit,nat(dst))