[ovs-dev,v2,1/1] netdev-vport: reject concomitant incompatible tunnels

Message ID 4c6b5f0eaf60f9243deb1ef700bea3421be8c0e3.1518186627.git.echaudro@redhat.com
State Changes Requested
Headers show
Series
  • [ovs-dev,v2,1/1] netdev-vport: reject concomitant incompatible tunnels
Related show

Commit Message

Eelco Chaudron Feb. 9, 2018, 2:42 p.m.
This patch will make sure VXLAN tunnels with and without the group
based policy (GBP) option enabled can not coexist on the same
destination UDP port.

In theory, VXLAN tunnel with and without GBP enables can be
multiplexed on the same UDP port as long as different VNI's are
used. However currently OVS does not support this, hence this patch to
check for this condition.

v1->v2
  Updated commit message as its now clear that the OVS  implementation
  does not support VNI multiplexing on the same UDP port.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/netdev-vport.c | 97 ++++++++++++++++++++++++++++++++++++++++++++----------
 tests/tunnel.at    | 29 ++++++++++++++++
 2 files changed, 108 insertions(+), 18 deletions(-)

Comments

Eelco Chaudron April 12, 2018, 9:46 a.m. | #1
Any feedback on this patch?

On 09/02/18 15:42, Eelco Chaudron wrote:
> This patch will make sure VXLAN tunnels with and without the group
> based policy (GBP) option enabled can not coexist on the same
> destination UDP port.
>
> In theory, VXLAN tunnel with and without GBP enables can be
> multiplexed on the same UDP port as long as different VNI's are
> used. However currently OVS does not support this, hence this patch to
> check for this condition.
>
> v1->v2
>    Updated commit message as its now clear that the OVS  implementation
>    does not support VNI multiplexing on the same UDP port.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>   lib/netdev-vport.c | 97 ++++++++++++++++++++++++++++++++++++++++++++----------
>   tests/tunnel.at    | 29 ++++++++++++++++
>   2 files changed, 108 insertions(+), 18 deletions(-)
>
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 52aa12d79..bc8226d62 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -66,11 +66,13 @@ static uint64_t rt_change_seqno;
>   static int get_patch_config(const struct netdev *netdev, struct smap *args);
>   static int get_tunnel_config(const struct netdev *, struct smap *args);
>   static bool tunnel_check_status_change__(struct netdev_vport *);
> +static const struct vport_class *netdev_vport_get_class_by_name(const char *);
>   
>   struct vport_class {
>       const char *dpif_port;
>       struct netdev_class netdev_class;
>   };
> +static const struct vport_class vport_classes[];
>   
>   bool
>   netdev_vport_is_vport_class(const struct netdev_class *class)
> @@ -421,6 +423,43 @@ default_pt_mode(enum tunnel_layers layers)
>       return layers == TNL_L3 ? NETDEV_PT_LEGACY_L3 : NETDEV_PT_LEGACY_L2;
>   }
>   
> +static bool
> +is_concomitant_vxlan_tunnel_present(struct netdev_vport *dev,
> +                                    const struct netdev_tunnel_config *tnl_cfg) {
> +    bool is_present = false;
> +    struct shash device_shash;
> +    struct shash_node *node;
> +    const char *type = netdev_get_type(&dev->up);
> +    const struct vport_class *vport_class;
> +
> +    if (strcmp(type, "vxlan")) {
> +        return false;
> +    }
> +
> +    shash_init(&device_shash);
> +    vport_class = netdev_vport_get_class_by_name("vxlan");
> +    ovs_assert(vport_class);
> +
> +    netdev_get_devices(&vport_class->netdev_class, &device_shash);
> +
> +    SHASH_FOR_EACH (node, &device_shash) {
> +        struct netdev *netdev_ = node->data;
> +        struct netdev_vport *netdev = netdev_vport_cast(netdev_);
> +
> +        if (netdev != dev &&
> +            tnl_cfg->dst_port == netdev->tnl_cfg.dst_port &&
> +            (tnl_cfg->exts & (1 << OVS_VXLAN_EXT_GBP)) !=
> +            (netdev->tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GBP))) {
> +            is_present = true;
> +        }
> +        netdev_close(netdev_);
> +    }
> +
> +    shash_destroy(&device_shash);
> +
> +    return is_present;
> +}
> +
>   static int
>   set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp)
>   {
> @@ -614,6 +653,15 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp)
>                                  &tnl_cfg.out_key_present,
>                                  &tnl_cfg.out_key_flow);
>   
> +    if (is_concomitant_vxlan_tunnel_present(dev, &tnl_cfg)) {
> +        ds_put_format(&errors, "%s: VXLAN-GBP, and non-VXLAN-GBP "
> +                      "tunnels can't be configured on the same "
> +                      "dst_port\n",
> +                      name);
> +        err = EEXIST;
> +        goto out;
> +    }
> +
>       ovs_mutex_lock(&dev->mutex);
>       if (memcmp(&dev->tnl_cfg, &tnl_cfg, sizeof tnl_cfg)) {
>           dev->tnl_cfg = tnl_cfg;
> @@ -959,27 +1007,40 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
>                             BUILD_HEADER, PUSH_HEADER, POP_HEADER,               \
>                             GET_IFINDEX) }}
>   
> +/* The name of the dpif_port should be short enough to accomodate adding
> + * a port number to the end if one is necessary. */
> +static const struct vport_class vport_classes[] = {
> +    TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header,
> +                                        netdev_tnl_push_udp_header,
> +                                        netdev_geneve_pop_header,
> +                                        NETDEV_VPORT_GET_IFINDEX),
> +    TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header,
> +                                   netdev_gre_push_header,
> +                                   netdev_gre_pop_header,
> +                                   NULL),
> +    TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header,
> +                                       netdev_tnl_push_udp_header,
> +                                       netdev_vxlan_pop_header,
> +                                       NETDEV_VPORT_GET_IFINDEX),
> +    TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL, NULL),
> +    TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL, NULL),
> +};
> +
> +static const struct vport_class *
> +netdev_vport_get_class_by_name(const char *name) {
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(vport_classes); i++) {
> +        if (!strcmp(vport_classes[i].netdev_class.type, name)) {
> +            return &vport_classes[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
>   void
>   netdev_vport_tunnel_register(void)
>   {
> -    /* The name of the dpif_port should be short enough to accomodate adding
> -     * a port number to the end if one is necessary. */
> -    static const struct vport_class vport_classes[] = {
> -        TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header,
> -                                            netdev_tnl_push_udp_header,
> -                                            netdev_geneve_pop_header,
> -                                            NETDEV_VPORT_GET_IFINDEX),
> -        TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header,
> -                                       netdev_gre_push_header,
> -                                       netdev_gre_pop_header,
> -                                       NULL),
> -        TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header,
> -                                           netdev_tnl_push_udp_header,
> -                                           netdev_vxlan_pop_header,
> -                                           NETDEV_VPORT_GET_IFINDEX),
> -        TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL, NULL),
> -        TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL, NULL),
> -    };
>       static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>   
>       if (ovsthread_once_start(&once)) {
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index 3c217b344..f9276796a 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -676,6 +676,35 @@ AT_CHECK([tail -1 stdout], [0],
>     [Datapath actions: set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=4789,flags(df))),4789
>   ])
>   
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([tunnel - concomitant incompatible tunnels on the same port])
> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \
> +                    options:remote_ip=flow ofport_request=1])
> +
> +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \
> +                    options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [0],
> +  [], [ignore])
> +
> +AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0],
> +  [p2: could not set configuration (File exists)
> +])
> +
> +OVS_VSWITCHD_STOP(["/p2: VXLAN-GBP, and non-VXLAN-GBP tunnels can't be configured on the same dst_port/d
> +/p2: could not set configuration (File exists)/d"])
> +AT_CLEANUP
> +
> +AT_SETUP([tunnel - concomitant incompatible tunnels on different ports])
> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \
> +                    options:remote_ip=flow ofport_request=1])
> +
> +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan options:dst_port=9000 \
> +                    options:remote_ip=flow options:exts=gbp ofport_request=2])
> +
> +AT_CHECK([grep p2 ovs-vswitchd.log | sed "s/^.*\(bridge br0:.*\)$/\1/"], [0],
> +  [bridge br0: added interface p2 on port 2
> +])
>   
>   OVS_VSWITCHD_STOP
>   AT_CLEANUP
Ben Pfaff April 13, 2018, 6:02 p.m. | #2
On Fri, Feb 09, 2018 at 03:42:56PM +0100, Eelco Chaudron wrote:
> This patch will make sure VXLAN tunnels with and without the group
> based policy (GBP) option enabled can not coexist on the same
> destination UDP port.
> 
> In theory, VXLAN tunnel with and without GBP enables can be
> multiplexed on the same UDP port as long as different VNI's are
> used. However currently OVS does not support this, hence this patch to
> check for this condition.
> 
> v1->v2
>   Updated commit message as its now clear that the OVS  implementation
>   does not support VNI multiplexing on the same UDP port.
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

Thanks for the update.

Doesn't this make tunnel configuration O(n**2) in the number of tunnels?
It looks like every tunnel checks at configuration time whether there is
another tunnel of the same kind.  I know of some configurations with
hundreds (thousands?) of tunnels.  Is there a way to make it cheaper?

Thanks,

Ben.
Eelco Chaudron May 9, 2018, 2:41 p.m. | #3
On 13/04/18 20:02, Ben Pfaff wrote:
> On Fri, Feb 09, 2018 at 03:42:56PM +0100, Eelco Chaudron wrote:
>> This patch will make sure VXLAN tunnels with and without the group
>> based policy (GBP) option enabled can not coexist on the same
>> destination UDP port.
>>
>> In theory, VXLAN tunnel with and without GBP enables can be
>> multiplexed on the same UDP port as long as different VNI's are
>> used. However currently OVS does not support this, hence this patch to
>> check for this condition.
>>
>> v1->v2
>>    Updated commit message as its now clear that the OVS  implementation
>>    does not support VNI multiplexing on the same UDP port.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> Thanks for the update.
>
> Doesn't this make tunnel configuration O(n**2) in the number of tunnels?
> It looks like every tunnel checks at configuration time whether there is
> another tunnel of the same kind.  I know of some configurations with
> hundreds (thousands?) of tunnels.  Is there a way to make it cheaper?
>

Thanks for the reply, and sorry for the late response, as this was on my low
priority stack...

I looked at it again and you are right this is an expensive operation with a
lot of tunnels, especially with the smap creation.

It looks like Cascardo's original patch with a simap per port might be less
expensive. However, he forgot the cleanup and with his approach, we need to
walk all tunnels to make sure this is the last tunnel and we can remove the
simap entry. I could do a shash and keep a tunnel count to avoid the cleanup
walk. Or maybe some other way to quickly find the vport with a hmap...

I'll investigate the options a bit more and come with a v3.

Cheers,

Eelco
Ben Pfaff May 9, 2018, 3:11 p.m. | #4
On Wed, May 09, 2018 at 04:41:05PM +0200, Eelco Chaudron wrote:
> On 13/04/18 20:02, Ben Pfaff wrote:
> >On Fri, Feb 09, 2018 at 03:42:56PM +0100, Eelco Chaudron wrote:
> >>This patch will make sure VXLAN tunnels with and without the group
> >>based policy (GBP) option enabled can not coexist on the same
> >>destination UDP port.
> >>
> >>In theory, VXLAN tunnel with and without GBP enables can be
> >>multiplexed on the same UDP port as long as different VNI's are
> >>used. However currently OVS does not support this, hence this patch to
> >>check for this condition.
> >>
> >>v1->v2
> >>   Updated commit message as its now clear that the OVS  implementation
> >>   does not support VNI multiplexing on the same UDP port.
> >>
> >>Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> >Thanks for the update.
> >
> >Doesn't this make tunnel configuration O(n**2) in the number of tunnels?
> >It looks like every tunnel checks at configuration time whether there is
> >another tunnel of the same kind.  I know of some configurations with
> >hundreds (thousands?) of tunnels.  Is there a way to make it cheaper?
> >
> 
> Thanks for the reply, and sorry for the late response, as this was on my low
> priority stack...
> 
> I looked at it again and you are right this is an expensive operation with a
> lot of tunnels, especially with the smap creation.
> 
> It looks like Cascardo's original patch with a simap per port might be less
> expensive. However, he forgot the cleanup and with his approach, we need to
> walk all tunnels to make sure this is the last tunnel and we can remove the
> simap entry. I could do a shash and keep a tunnel count to avoid the cleanup
> walk. Or maybe some other way to quickly find the vport with a hmap...
> 
> I'll investigate the options a bit more and come with a v3.

Thanks for taking a look.

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 52aa12d79..bc8226d62 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -66,11 +66,13 @@  static uint64_t rt_change_seqno;
 static int get_patch_config(const struct netdev *netdev, struct smap *args);
 static int get_tunnel_config(const struct netdev *, struct smap *args);
 static bool tunnel_check_status_change__(struct netdev_vport *);
+static const struct vport_class *netdev_vport_get_class_by_name(const char *);
 
 struct vport_class {
     const char *dpif_port;
     struct netdev_class netdev_class;
 };
+static const struct vport_class vport_classes[];
 
 bool
 netdev_vport_is_vport_class(const struct netdev_class *class)
@@ -421,6 +423,43 @@  default_pt_mode(enum tunnel_layers layers)
     return layers == TNL_L3 ? NETDEV_PT_LEGACY_L3 : NETDEV_PT_LEGACY_L2;
 }
 
+static bool
+is_concomitant_vxlan_tunnel_present(struct netdev_vport *dev,
+                                    const struct netdev_tunnel_config *tnl_cfg) {
+    bool is_present = false;
+    struct shash device_shash;
+    struct shash_node *node;
+    const char *type = netdev_get_type(&dev->up);
+    const struct vport_class *vport_class;
+
+    if (strcmp(type, "vxlan")) {
+        return false;
+    }
+
+    shash_init(&device_shash);
+    vport_class = netdev_vport_get_class_by_name("vxlan");
+    ovs_assert(vport_class);
+
+    netdev_get_devices(&vport_class->netdev_class, &device_shash);
+
+    SHASH_FOR_EACH (node, &device_shash) {
+        struct netdev *netdev_ = node->data;
+        struct netdev_vport *netdev = netdev_vport_cast(netdev_);
+
+        if (netdev != dev &&
+            tnl_cfg->dst_port == netdev->tnl_cfg.dst_port &&
+            (tnl_cfg->exts & (1 << OVS_VXLAN_EXT_GBP)) !=
+            (netdev->tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GBP))) {
+            is_present = true;
+        }
+        netdev_close(netdev_);
+    }
+
+    shash_destroy(&device_shash);
+
+    return is_present;
+}
+
 static int
 set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp)
 {
@@ -614,6 +653,15 @@  set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp)
                                &tnl_cfg.out_key_present,
                                &tnl_cfg.out_key_flow);
 
+    if (is_concomitant_vxlan_tunnel_present(dev, &tnl_cfg)) {
+        ds_put_format(&errors, "%s: VXLAN-GBP, and non-VXLAN-GBP "
+                      "tunnels can't be configured on the same "
+                      "dst_port\n",
+                      name);
+        err = EEXIST;
+        goto out;
+    }
+
     ovs_mutex_lock(&dev->mutex);
     if (memcmp(&dev->tnl_cfg, &tnl_cfg, sizeof tnl_cfg)) {
         dev->tnl_cfg = tnl_cfg;
@@ -959,27 +1007,40 @@  netdev_vport_get_ifindex(const struct netdev *netdev_)
                           BUILD_HEADER, PUSH_HEADER, POP_HEADER,               \
                           GET_IFINDEX) }}
 
+/* The name of the dpif_port should be short enough to accomodate adding
+ * a port number to the end if one is necessary. */
+static const struct vport_class vport_classes[] = {
+    TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header,
+                                        netdev_tnl_push_udp_header,
+                                        netdev_geneve_pop_header,
+                                        NETDEV_VPORT_GET_IFINDEX),
+    TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header,
+                                   netdev_gre_push_header,
+                                   netdev_gre_pop_header,
+                                   NULL),
+    TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header,
+                                       netdev_tnl_push_udp_header,
+                                       netdev_vxlan_pop_header,
+                                       NETDEV_VPORT_GET_IFINDEX),
+    TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL, NULL),
+    TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL, NULL),
+};
+
+static const struct vport_class *
+netdev_vport_get_class_by_name(const char *name) {
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(vport_classes); i++) {
+        if (!strcmp(vport_classes[i].netdev_class.type, name)) {
+            return &vport_classes[i];
+        }
+    }
+    return NULL;
+}
+
 void
 netdev_vport_tunnel_register(void)
 {
-    /* The name of the dpif_port should be short enough to accomodate adding
-     * a port number to the end if one is necessary. */
-    static const struct vport_class vport_classes[] = {
-        TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header,
-                                            netdev_tnl_push_udp_header,
-                                            netdev_geneve_pop_header,
-                                            NETDEV_VPORT_GET_IFINDEX),
-        TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header,
-                                       netdev_gre_push_header,
-                                       netdev_gre_pop_header,
-                                       NULL),
-        TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header,
-                                           netdev_tnl_push_udp_header,
-                                           netdev_vxlan_pop_header,
-                                           NETDEV_VPORT_GET_IFINDEX),
-        TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL, NULL),
-        TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL, NULL),
-    };
     static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
 
     if (ovsthread_once_start(&once)) {
diff --git a/tests/tunnel.at b/tests/tunnel.at
index 3c217b344..f9276796a 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -676,6 +676,35 @@  AT_CHECK([tail -1 stdout], [0],
   [Datapath actions: set(tunnel(dst=1.1.1.1,ttl=64,tp_dst=4789,flags(df))),4789
 ])
 
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([tunnel - concomitant incompatible tunnels on the same port])
+OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \
+                    options:remote_ip=flow ofport_request=1])
+
+AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \
+                    options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [0],
+  [], [ignore])
+
+AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0],
+  [p2: could not set configuration (File exists)
+])
+
+OVS_VSWITCHD_STOP(["/p2: VXLAN-GBP, and non-VXLAN-GBP tunnels can't be configured on the same dst_port/d
+/p2: could not set configuration (File exists)/d"])
+AT_CLEANUP
+
+AT_SETUP([tunnel - concomitant incompatible tunnels on different ports])
+OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \
+                    options:remote_ip=flow ofport_request=1])
+
+AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan options:dst_port=9000 \
+                    options:remote_ip=flow options:exts=gbp ofport_request=2])
+
+AT_CHECK([grep p2 ovs-vswitchd.log | sed "s/^.*\(bridge br0:.*\)$/\1/"], [0],
+  [bridge br0: added interface p2 on port 2
+])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP