[ovs-dev] conntrack: add display support for sctp
diff mbox series

Message ID 20190517160402.28095-1-aconole@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] conntrack: add display support for sctp
Related show

Commit Message

Aaron Conole May 17, 2019, 4:04 p.m. UTC
Currently, only the netlink datapath supports SCTP connection tracking,
but at least this removes the warning message that will pop up when
running something like:

   ovs-appctl dpctl/dump-conntrack

This doesn't impact any conntrack functionality, just the display.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
I did the heartbeat states by hardcoding the values in the ENUM since
they are fixed in the linux UAPI, and have existed for a long time,
but it could also make sense to rework this patch so that those states
are omitted by the ifdef rather than defining them (I'll submit a v2
if that's preferred approach - but this seems like the more flexible
option).

 acinclude.m4            | 12 +++++++
 configure.ac            |  1 +
 lib/ct-dpif.c           | 19 +++++++++++
 lib/ct-dpif.h           | 30 ++++++++++++++++++
 lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 131 insertions(+), 1 deletion(-)

Comments

0-day Robot May 17, 2019, 4:56 p.m. UTC | #1
Bleep bloop.  Greetings Aaron Conole, 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.


build:
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netdev-tc-offloads.lo -MD -MP -MF lib/.deps/netdev-tc-offloads.Tpo -c lib/netdev-tc-offloads.c -o lib/netdev-tc-offloads.o
depbase=`echo lib/netlink-conntrack.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/netlink-conntrack.lo -MD -MP -MF $depbase.Tpo -c -o lib/netlink-conntrack.lo lib/netlink-conntrack.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netlink-conntrack.lo -MD -MP -MF lib/.deps/netlink-conntrack.Tpo -c lib/netlink-conntrack.c -o lib/netlink-conntrack.o
In file included from lib/netlink-conntrack.h:22:0,
                 from lib/netlink-conntrack.c:19:
lib/ct-dpif.h:105:39: error: redeclaration of enumerator 'SCTP_CONNTRACK_SHUTDOWN_ACK_SENT'
 #define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1
                                       ^
In file included from lib/netlink-conntrack.c:27:0:
/usr/include/linux/netfilter/nf_conntrack_sctp.h:15:2: note: previous definition of 'SCTP_CONNTRACK_SHUTDOWN_ACK_SENT' was here
  SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
  ^
In file included from lib/netlink-conntrack.h:22:0,
                 from lib/netlink-conntrack.c:19:
lib/ct-dpif.h:105:71: error: expected ',' or '}' before '+' token
 #define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1
                                                                       ^
make[2]: *** [lib/netlink-conntrack.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


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

Thanks,
0-day Robot
David Marchand May 20, 2019, 7:34 a.m. UTC | #2
On Fri, May 17, 2019 at 6:05 PM Aaron Conole <aconole@redhat.com> wrote:

> Currently, only the netlink datapath supports SCTP connection tracking,
> but at least this removes the warning message that will pop up when
> running something like:
>
>    ovs-appctl dpctl/dump-conntrack
>
> This doesn't impact any conntrack functionality, just the display.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> I did the heartbeat states by hardcoding the values in the ENUM since
> they are fixed in the linux UAPI, and have existed for a long time,
> but it could also make sense to rework this patch so that those states
> are omitted by the ifdef rather than defining them (I'll submit a v2
> if that's preferred approach - but this seems like the more flexible
> option).
>
>  acinclude.m4            | 12 +++++++
>  configure.ac            |  1 +
>  lib/ct-dpif.c           | 19 +++++++++++
>  lib/ct-dpif.h           | 30 ++++++++++++++++++
>  lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 131 insertions(+), 1 deletion(-)
>
>
[snip]

@@ -497,6 +503,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds,
>                           tcp_flags);
>  }
>
> +static void
> +ct_dpif_format_protoinfo_sctp(struct ds *ds,
> +                              const struct ct_dpif_protoinfo *protoinfo)
> +{
> +    ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state,
> +                        ct_dpif_sctp_state_string);
> +    ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u",
> protoinfo->sctp.vtag_orig,
> +                  protoinfo->sctp.vtag_reply);
>

PRIu32 for both of them?
Aaron Conole May 20, 2019, 3:20 p.m. UTC | #3
David Marchand <david.marchand@redhat.com> writes:

> On Fri, May 17, 2019 at 6:05 PM Aaron Conole <aconole@redhat.com> wrote:
>
>  Currently, only the netlink datapath supports SCTP connection tracking,
>  but at least this removes the warning message that will pop up when
>  running something like:
>
>     ovs-appctl dpctl/dump-conntrack
>
>  This doesn't impact any conntrack functionality, just the display.
>
>  Signed-off-by: Aaron Conole <aconole@redhat.com>
>  ---
>  I did the heartbeat states by hardcoding the values in the ENUM since
>  they are fixed in the linux UAPI, and have existed for a long time,
>  but it could also make sense to rework this patch so that those states
>  are omitted by the ifdef rather than defining them (I'll submit a v2
>  if that's preferred approach - but this seems like the more flexible
>  option).
>
>   acinclude.m4            | 12 +++++++
>   configure.ac            |  1 +
>   lib/ct-dpif.c           | 19 +++++++++++
>   lib/ct-dpif.h           | 30 ++++++++++++++++++
>   lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++-
>   5 files changed, 131 insertions(+), 1 deletion(-)
>
> [snip]
>
>  @@ -497,6 +503,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds,
>                            tcp_flags);
>   }
>
>  +static void
>  +ct_dpif_format_protoinfo_sctp(struct ds *ds,
>  +                              const struct ct_dpif_protoinfo *protoinfo)
>  +{
>  +    ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state,
>  +                        ct_dpif_sctp_state_string);
>  +    ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u", protoinfo->sctp.vtag_orig,
>  +                  protoinfo->sctp.vtag_reply);
>
> PRIu32 for both of them?

Sure - that makes sense.  I'll spin a v3.

Patch
diff mbox series

diff --git a/acinclude.m4 b/acinclude.m4
index b532a4579..ca770ec81 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -212,6 +212,18 @@  AC_DEFUN([OVS_CHECK_LINUX_TC], [
                [Define to 1 if TCA_SKBEDIT_FLAGS is available.])])
 ])
 
+dnl OVS_CHECK_LINUX_SCTP_CT
+dnl
+dnl Checks for kernels which need additional SCTP state
+AC_DEFUN([OVS_CHECK_LINUX_SCTP_CT], [
+  AC_COMPILE_IFELSE([
+    AC_LANG_PROGRAM([#include <linux/netfilter/nf_conntrack_sctp.h], [
+        int x = SCTP_CONNTRACK_HEARTBEAT_SENT;
+    ])],
+    [AC_DEFINE([HAVE_SCTP_CONNTRACK_HEARTBEATS], [1],
+               [Define to 1 if SCTP_CONNTRACK_HEARTBEAT_SENT is available.])])
+])
+
 dnl OVS_FIND_DEPENDENCY(FUNCTION, SEARCH_LIBS, NAME_TO_PRINT)
 dnl
 dnl Check for a function in a library list.
diff --git a/configure.ac b/configure.ac
index 505e3d041..2dbe9a917 100644
--- a/configure.ac
+++ b/configure.ac
@@ -186,6 +186,7 @@  AC_ARG_VAR(KARCH, [Kernel Architecture String])
 AC_SUBST(KARCH)
 OVS_CHECK_LINUX
 OVS_CHECK_LINUX_TC
+OVS_CHECK_LINUX_SCTP_CT
 OVS_CHECK_DPDK
 OVS_CHECK_PRAGMA_MESSAGE
 AC_SUBST([OVS_CFLAGS])
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index b2c9b4309..17e348ea9 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -428,6 +428,12 @@  const char *ct_dpif_tcp_state_string[] = {
 #undef CT_DPIF_TCP_STATE
 };
 
+const char *ct_dpif_sctp_state_string[] = {
+#define CT_DPIF_SCTP_STATE(STATE) [CT_DPIF_SCTP_STATE_##STATE] = #STATE,
+    CT_DPIF_SCTP_STATES
+#undef CT_DPIF_SCTP_STATE
+};
+
 static void
 ct_dpif_format_enum__(struct ds *ds, const char *title, unsigned int state,
                       const char *names[], unsigned int max)
@@ -497,6 +503,16 @@  ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds,
                          tcp_flags);
 }
 
+static void
+ct_dpif_format_protoinfo_sctp(struct ds *ds,
+                              const struct ct_dpif_protoinfo *protoinfo)
+{
+    ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state,
+                        ct_dpif_sctp_state_string);
+    ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u", protoinfo->sctp.vtag_orig,
+                  protoinfo->sctp.vtag_reply);
+}
+
 static void
 ct_dpif_format_protoinfo(struct ds *ds, const char *title,
                          const struct ct_dpif_protoinfo *protoinfo,
@@ -514,6 +530,9 @@  ct_dpif_format_protoinfo(struct ds *ds, const char *title,
                 ct_dpif_format_protoinfo_tcp(ds, protoinfo);
             }
             break;
+        case IPPROTO_SCTP:
+            ct_dpif_format_protoinfo_sctp(ds, protoinfo);
+            break;
         }
         if (title) {
             ds_put_cstr(ds, ")");
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 0151cfea4..34e41ed30 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -101,6 +101,31 @@  enum ct_dpif_tcp_flags {
 #undef CT_DPIF_TCP_FLAG
 };
 
+#ifndef HAVE_SCTP_CONNTRACK_HEARTBEATS
+#define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1
+#define SCTP_CONNTRACK_HEARTBEAT_ACKED SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+2
+#endif
+
+extern const char *ct_dpif_sctp_state_string[];
+
+#define CT_DPIF_SCTP_STATES \
+    CT_DPIF_SCTP_STATE(CLOSED) \
+    CT_DPIF_SCTP_STATE(COOKIE_WAIT) \
+    CT_DPIF_SCTP_STATE(COOKIE_ECHOED) \
+    CT_DPIF_SCTP_STATE(ESTABLISHED) \
+    CT_DPIF_SCTP_STATE(SHUTDOWN_SENT) \
+    CT_DPIF_SCTP_STATE(SHUTDOWN_RECD) \
+    CT_DPIF_SCTP_STATE(SHUTDOWN_ACK_SENT) \
+    CT_DPIF_SCTP_STATE(HEARTBEAT_SENT) \
+    CT_DPIF_SCTP_STATE(HEARTBEAT_ACKED) \
+    CT_DPIF_SCTP_STATE(MAX_NUM)
+
+enum ct_dpif_sctp_state {
+#define CT_DPIF_SCTP_STATE(STATE) CT_DPIF_SCTP_STATE_##STATE,
+    CT_DPIF_SCTP_STATES
+#undef CT_DPIF_SCTP_STATE
+};
+
 struct ct_dpif_protoinfo {
     uint16_t proto; /* IPPROTO_* */
     union {
@@ -112,6 +137,11 @@  struct ct_dpif_protoinfo {
             uint8_t flags_orig;
             uint8_t flags_reply;
         } tcp;
+        struct {
+            uint8_t state;
+            uint32_t vtag_orig;
+            uint32_t vtag_reply;
+        } sctp;
     };
 };
 
diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 42be1d9ce..7631ba5d5 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -717,6 +717,73 @@  nl_ct_parse_protoinfo_tcp(struct nlattr *nla,
     return parsed;
 }
 
+/* Translate netlink SCTP state to CT_DPIF_SCTP state. */
+static uint8_t
+nl_ct_sctp_state_to_dpif(uint8_t state)
+{
+#ifdef _WIN32
+    /* For now, return the CT_DPIF_SCTP state. Not sure what windows does. */
+    return state;
+#else
+    switch (state) {
+    case SCTP_CONNTRACK_COOKIE_WAIT:
+        return CT_DPIF_SCTP_STATE_COOKIE_WAIT;
+    case SCTP_CONNTRACK_COOKIE_ECHOED:
+        return CT_DPIF_SCTP_STATE_COOKIE_ECHOED;
+    case SCTP_CONNTRACK_ESTABLISHED:
+        return CT_DPIF_SCTP_STATE_ESTABLISHED;
+    case SCTP_CONNTRACK_SHUTDOWN_SENT:
+        return CT_DPIF_SCTP_STATE_SHUTDOWN_SENT;
+    case SCTP_CONNTRACK_SHUTDOWN_RECD:
+        return CT_DPIF_SCTP_STATE_SHUTDOWN_RECD;
+    case SCTP_CONNTRACK_SHUTDOWN_ACK_SENT:
+        return CT_DPIF_SCTP_STATE_SHUTDOWN_ACK_SENT;
+    case SCTP_CONNTRACK_HEARTBEAT_SENT:
+        return CT_DPIF_SCTP_STATE_HEARTBEAT_SENT;
+    case SCTP_CONNTRACK_HEARTBEAT_ACKED:
+        return CT_DPIF_SCTP_STATE_HEARTBEAT_ACKED;
+    case SCTP_CONNTRACK_CLOSED:
+        /* Fall Through. */
+    case SCTP_CONNTRACK_NONE:
+        /* Fall Through. */
+    default:
+        return CT_DPIF_SCTP_STATE_CLOSED;
+    }
+#endif
+}
+
+static bool
+nl_ct_parse_protoinfo_sctp(struct nlattr *nla,
+                           struct ct_dpif_protoinfo *protoinfo)
+{
+    static const struct nl_policy policy[] = {
+        [CTA_PROTOINFO_SCTP_STATE] = { .type = NL_A_U8, .optional = false },
+        [CTA_PROTOINFO_SCTP_VTAG_ORIGINAL] = { .type = NL_A_U32,
+                                               .optional = false },
+        [CTA_PROTOINFO_SCTP_VTAG_REPLY] = { .type = NL_A_U32,
+                                            .optional = false },
+    };
+    struct nlattr *attrs[ARRAY_SIZE(policy)];
+    bool parsed;
+
+    parsed = nl_parse_nested(nla, policy, attrs, ARRAY_SIZE(policy));
+    if (parsed) {
+        protoinfo->proto = IPPROTO_SCTP;
+
+        protoinfo->sctp.state = nl_ct_sctp_state_to_dpif(
+            nl_attr_get_u8(attrs[CTA_PROTOINFO_SCTP_STATE]));
+        protoinfo->sctp.vtag_orig = nl_attr_get_u32(
+            attrs[CTA_PROTOINFO_SCTP_VTAG_ORIGINAL]);
+        protoinfo->sctp.vtag_reply = nl_attr_get_u32(
+            attrs[CTA_PROTOINFO_SCTP_VTAG_REPLY]);
+    } else {
+        VLOG_ERR_RL(&rl, "Could not parse nested SCTP protoinfo options. "
+                    "Possibly incompatible Linux kernel version.");
+    }
+
+    return parsed;
+}
+
 static bool
 nl_ct_parse_protoinfo(struct nlattr *nla, struct ct_dpif_protoinfo *protoinfo)
 {
@@ -737,7 +804,8 @@  nl_ct_parse_protoinfo(struct nlattr *nla, struct ct_dpif_protoinfo *protoinfo)
             parsed = nl_ct_parse_protoinfo_tcp(attrs[CTA_PROTOINFO_TCP],
                                                protoinfo);
         } else if (attrs[CTA_PROTOINFO_SCTP]) {
-            VLOG_WARN_RL(&rl, "SCTP protoinfo not yet supported!");
+            parsed = nl_ct_parse_protoinfo_sctp(attrs[CTA_PROTOINFO_SCTP],
+                                                protoinfo);
         } else {
             VLOG_WARN_RL(&rl, "Empty protoinfo!");
         }