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

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

Commit Message

Eelco Chaudron June 1, 2018, 5:03 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.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---

lib/netdev-vport.c | 147 +++++++++++++++++++++++++++++++++++++++++++--
 tests/tunnel.at    |  29 +++++++++
 2 files changed, 171 insertions(+), 5 deletions(-)

* V2
  Updated commit message as its now clear that the OVS  implementation
  does not support VNI multiplexing on the same UDP port.

* V3
  Back to Cascardo's original approach, i.e. storing some global port
  information in the class. This avoids checking all available tunnels
  on every tunnel add/delete/update.

Comments

Ben Pfaff July 10, 2018, 9:45 p.m. | #1
On Fri, Jun 01, 2018 at 07:03:55PM +0200, 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.
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>

Thanks for the patch, and sorry that I'm so slow.

Does this support the case where, in a single database transaction, a
GBP VXLAN tunnel is removed and a non-GBP VXLAN tunnel is created that
would otherwise interfere with one another (and the converse case)?  If
so, could that be included in the test?

"sparse" doesn't like the initialization strategy:

../lib/netdev-vport.c:1207:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1211:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1215:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1219:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1220:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1221:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1225:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1229:9: error: Using plain integer as NULL pointer
../lib/netdev-vport.c:1261:13: error: Using plain integer as NULL pointer

Thanks,

Ben.
Eelco Chaudron July 11, 2018, 8:01 a.m. | #2
On 10 Jul 2018, at 23:45, Ben Pfaff wrote:

> On Fri, Jun 01, 2018 at 07:03:55PM +0200, 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.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>
> Thanks for the patch, and sorry that I'm so slow.

No problem, will be on PTO later today so will take some time before 
I’ll send a v4.

> Does this support the case where, in a single database transaction, a
> GBP VXLAN tunnel is removed and a non-GBP VXLAN tunnel is created that
> would otherwise interfere with one another (and the converse case)?  
> If
> so, could that be included in the test?

I did some testing earlier and it does. Will add some test cases for it.

> "sparse" doesn't like the initialization strategy:
>
> ../lib/netdev-vport.c:1207:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1211:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1215:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1219:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1220:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1221:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1225:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1229:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1261:13: error: Using plain integer as NULL 
> pointer

Will look at this also…
Eelco Chaudron Aug. 24, 2018, 9:38 a.m. | #3
On 10 Jul 2018, at 23:45, Ben Pfaff wrote:

> On Fri, Jun 01, 2018 at 07:03:55PM +0200, 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.
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>
> Thanks for the patch, and sorry that I'm so slow.
>
> Does this support the case where, in a single database transaction, a
> GBP VXLAN tunnel is removed and a non-GBP VXLAN tunnel is created that
> would otherwise interfere with one another (and the converse case)?  
> If
> so, could that be included in the test?
>
> "sparse" doesn't like the initialization strategy:
>
> ../lib/netdev-vport.c:1207:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1211:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1215:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1219:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1220:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1221:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1225:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1229:9: error: Using plain integer as NULL 
> pointer
> ../lib/netdev-vport.c:1261:13: error: Using plain integer as NULL 
> pointer
>

Hi Ben some of this is existing code, however, which version of Sparse 
are you using, as I’m not getting any error?

> Thanks,
>
> Ben.
Eelco Chaudron Aug. 24, 2018, 10:27 a.m. | #4
On 24 Aug 2018, at 11:38, Eelco Chaudron wrote:

> On 10 Jul 2018, at 23:45, Ben Pfaff wrote:
>
>> On Fri, Jun 01, 2018 at 07:03:55PM +0200, Eelco Chaudron wrote:
>>
>> "sparse" doesn't like the initialization strategy:
>>
>> ../lib/netdev-vport.c:1207:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1211:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1215:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1219:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1220:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1221:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1225:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1229:9: error: Using plain integer as NULL 
>> pointer
>> ../lib/netdev-vport.c:1261:13: error: Using plain integer as NULL 
>> pointer
>>
>
> Hi Ben some of this is existing code, however, which version of Sparse 
> are you using, as I’m not getting any error?
>

Ignore, as I can see them trough Travis with the same version. Must be 
my setup :(

Guess it’s time for weekend…

//Eelco
Ben Pfaff Aug. 24, 2018, 5:49 p.m. | #5
On Fri, Aug 24, 2018 at 11:38:05AM +0200, Eelco Chaudron wrote:
> 
> 
> On 10 Jul 2018, at 23:45, Ben Pfaff wrote:
> 
> >On Fri, Jun 01, 2018 at 07:03:55PM +0200, 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.
> >>
> >>Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> >
> >Thanks for the patch, and sorry that I'm so slow.
> >
> >Does this support the case where, in a single database transaction, a
> >GBP VXLAN tunnel is removed and a non-GBP VXLAN tunnel is created that
> >would otherwise interfere with one another (and the converse case)?  If
> >so, could that be included in the test?
> >
> >"sparse" doesn't like the initialization strategy:
> >
> >../lib/netdev-vport.c:1207:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1211:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1215:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1219:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1220:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1221:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1225:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1229:9: error: Using plain integer as NULL pointer
> >../lib/netdev-vport.c:1261:13: error: Using plain integer as NULL pointer
> >
> 
> Hi Ben some of this is existing code, however, which version of Sparse are
> you using, as I’m not getting any error?

I appear to be using:

    $ sparse --version
    v0.5.0-44-g40791b9

via git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.  It looks
like v0.5.2 exists, so I should probably upgrade.

What version do you have?

However, this might all be obsoleted by the changes that should go in in
Ian Stokes' next pull request, which should include the following patch
to make the code here a little less awful:
        https://patchwork.ozlabs.org/patch/957990/
Eelco Chaudron Aug. 24, 2018, 6:26 p.m. | #6
On 24 Aug 2018, at 19:49, Ben Pfaff wrote:

> On Fri, Aug 24, 2018 at 11:38:05AM +0200, Eelco Chaudron wrote:
>>
>> Hi Ben some of this is existing code, however, which version of 
>> Sparse are
>> you using, as I’m not getting any error?
>
> I appear to be using:
>
>     $ sparse --version
>     v0.5.0-44-g40791b9
>
> via git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.  It looks
> like v0.5.2 exists, so I should probably upgrade.

I was using 5.2, but I think it’s a problem in combination with my GCC 
version.
Will try figure this out next week…

> What version do you have?
>
> However, this might all be obsoleted by the changes that should go in 
> in
> Ian Stokes' next pull request, which should include the following 
> patch
> to make the code here a little less awful:
>         https://patchwork.ozlabs.org/patch/957990/

I’ll wait for this to go in before sending a v4.
Eelco Chaudron Aug. 27, 2018, 3:31 p.m. | #7
On 11 Jul 2018, at 10:01, Eelco Chaudron wrote:

> On 10 Jul 2018, at 23:45, Ben Pfaff wrote:

<SNIP>
>> Does this support the case where, in a single database transaction, a
>> GBP VXLAN tunnel is removed and a non-GBP VXLAN tunnel is created 
>> that
>> would otherwise interfere with one another (and the converse case)?  
>> If
>> so, could that be included in the test?
>
> I did some testing earlier and it does. Will add some test cases for 
> it.


Tried it again and due to the order of adding and removing the actual 
netdev you do get the error, but it will be corrected by the system when 
the netdev gets removed. However, when using the test framework I do not 
see this behavior.

Is this due to known test framework limitation regarding cleanup, use of 
(vport)netdevs?

If not known, I’ll try to figure out why it’s not working on the 
test framework.

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 1dae7e031..2be86cf19 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -41,6 +41,7 @@ 
 #include "packets.h"
 #include "openvswitch/poll-loop.h"
 #include "route-table.h"
+#include "simap.h"
 #include "smap.h"
 #include "socket-util.h"
 #include "unaligned.h"
@@ -66,10 +67,14 @@  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 void update_vxlan_global_cfg(struct netdev *,
+                                    struct netdev_tunnel_config *,
+                                    struct netdev_tunnel_config *);
 
 struct vport_class {
     const char *dpif_port;
     struct netdev_class netdev_class;
+    struct simap global_cfg_tracker;
 };
 
 bool
@@ -78,7 +83,7 @@  netdev_vport_is_vport_class(const struct netdev_class *class)
     return is_vport_class(class);
 }
 
-static const struct vport_class *
+static struct vport_class *
 vport_class_cast(const struct netdev_class *class)
 {
     ovs_assert(is_vport_class(class));
@@ -195,6 +200,7 @@  netdev_vport_construct(struct netdev *netdev_)
         dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT);
     } else if (!strcmp(type, "vxlan")) {
         dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT);
+        update_vxlan_global_cfg(netdev_, NULL, &dev->tnl_cfg);
     } else if (!strcmp(type, "lisp")) {
         dev->tnl_cfg.dst_port = htons(LISP_DST_PORT);
     } else if (!strcmp(type, "stt")) {
@@ -210,6 +216,11 @@  static void
 netdev_vport_destruct(struct netdev *netdev_)
 {
     struct netdev_vport *netdev = netdev_vport_cast(netdev_);
+    const char *type = netdev_get_type(netdev_);
+
+    if (!strcmp(type, "vxlan")) {
+        update_vxlan_global_cfg(netdev_, &netdev->tnl_cfg, NULL);
+    }
 
     free(netdev->peer);
     ovs_mutex_destroy(&netdev->mutex);
@@ -421,6 +432,117 @@  default_pt_mode(enum tunnel_layers layers)
     return layers == TNL_L3 ? NETDEV_PT_LEGACY_L3 : NETDEV_PT_LEGACY_L2;
 }
 
+static char *
+vxlan_get_port_ext_gbp_str(uint16_t port, bool gbp,
+                           char namebuf[], size_t bufsize)
+{
+    snprintf(namebuf, bufsize, "dst_port_%d%s",
+             port, gbp ? "_gbp" : "");
+
+    return namebuf;
+}
+
+static void
+update_vxlan_global_cfg(struct netdev *netdev,
+                        struct netdev_tunnel_config *old_cfg,
+                        struct netdev_tunnel_config *new_cfg)
+{
+    unsigned int count;
+    char namebuf[20];
+    const char *type = netdev_get_type(netdev);
+    struct vport_class *vclass = vport_class_cast(netdev_get_class(netdev));
+
+    if (strcmp(type, "vxlan") ||
+        (old_cfg != NULL && new_cfg != NULL &&
+         old_cfg->dst_port == new_cfg->dst_port &&
+         old_cfg->exts == new_cfg->exts)) {
+        return;
+    }
+
+    if (old_cfg != NULL) {
+        vxlan_get_port_ext_gbp_str(ntohs(old_cfg->dst_port),
+                                   old_cfg->exts &
+                                   (1 << OVS_VXLAN_EXT_GBP),
+                                   namebuf, sizeof(namebuf));
+
+        count = simap_get(&vclass->global_cfg_tracker, namebuf);
+        if (count != 0) {
+            if (--count) {
+                simap_put(&vclass->global_cfg_tracker, namebuf, count);
+            } else {
+                simap_find_and_delete(&vclass->global_cfg_tracker, namebuf);
+           }
+        }
+    }
+
+    if (new_cfg != NULL) {
+        vxlan_get_port_ext_gbp_str(ntohs(new_cfg->dst_port),
+                                   new_cfg->exts &
+                                   (1 << OVS_VXLAN_EXT_GBP),
+                                   namebuf, sizeof(namebuf));
+
+        simap_increase(&vclass->global_cfg_tracker, namebuf, 1);
+    }
+}
+
+static bool
+is_concomitant_vxlan_tunnel_present(struct netdev_vport *dev,
+                                    const struct netdev_tunnel_config *tnl_cfg)
+{
+    char namebuf[20];
+    const char *type = netdev_get_type(&dev->up);
+    struct vport_class *vclass = vport_class_cast(netdev_get_class(&dev->up));
+
+    if (strcmp(type, "vxlan")) {
+        return false;
+    }
+
+    if (dev->tnl_cfg.dst_port == tnl_cfg->dst_port &&
+        (dev->tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GBP)) ==
+        (tnl_cfg->exts & (1 << OVS_VXLAN_EXT_GBP))) {
+
+        if (ntohs(dev->tnl_cfg.dst_port) == VXLAN_DST_PORT) {
+            /* Special case where we kept the default port/gbp, only ok if
+               the opposite of the default does not exits */
+            vxlan_get_port_ext_gbp_str(ntohs(tnl_cfg->dst_port),
+                                       !(tnl_cfg->exts &
+                                         (1 << OVS_VXLAN_EXT_GBP)),
+                                       namebuf, sizeof(namebuf));
+
+            if (simap_get(&vclass->global_cfg_tracker, namebuf) > 0) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /* Same port: ok if no one is left with the previous configuration */
+    if (dev->tnl_cfg.dst_port == tnl_cfg->dst_port) {
+        vxlan_get_port_ext_gbp_str(ntohs(dev->tnl_cfg.dst_port),
+                                   dev->tnl_cfg.exts &
+                                   (1 << OVS_VXLAN_EXT_GBP),
+                                   namebuf, sizeof(namebuf));
+
+        if (simap_get(&vclass->global_cfg_tracker, namebuf) > 1) {
+            return true;
+        }
+
+        return false;
+    }
+
+    /* Different port: ok if the opposite gbp option does not yet exists */
+    vxlan_get_port_ext_gbp_str(ntohs(tnl_cfg->dst_port),
+                               !(tnl_cfg->exts &
+                                 (1 << OVS_VXLAN_EXT_GBP)),
+                               namebuf, sizeof(namebuf));
+
+    if (simap_get(&vclass->global_cfg_tracker, namebuf) > 0) {
+        return true;
+    }
+
+    return false;
+}
+
 static int
 set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp)
 {
@@ -678,6 +800,16 @@  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;
+    }
+    update_vxlan_global_cfg(dev_, &dev->tnl_cfg, &tnl_cfg);
+
     ovs_mutex_lock(&dev->mutex);
     if (memcmp(&dev->tnl_cfg, &tnl_cfg, sizeof tnl_cfg)) {
         dev->tnl_cfg = tnl_cfg;
@@ -1062,14 +1194,15 @@  netdev_vport_get_ifindex(const struct netdev *netdev_)
                           get_netdev_tunnel_config,                            \
                           tunnel_get_status,                                   \
                           BUILD_HEADER, PUSH_HEADER, POP_HEADER,               \
-                          GET_IFINDEX) }}
+                          GET_IFINDEX) },                                      \
+      {{0}} }
 
 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[] = {
+    static struct vport_class vport_classes[] = {
         TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header,
                                             netdev_tnl_push_udp_header,
                                             netdev_geneve_pop_header,
@@ -1103,6 +1236,7 @@  netdev_vport_tunnel_register(void)
         int i;
 
         for (i = 0; i < ARRAY_SIZE(vport_classes); i++) {
+            simap_init(&vport_classes[i].global_cfg_tracker);
             netdev_register_provider(&vport_classes[i].netdev_class);
         }
 
@@ -1116,12 +1250,15 @@  netdev_vport_tunnel_register(void)
 void
 netdev_vport_patch_register(void)
 {
-    static const struct vport_class patch_class =
+    static struct vport_class patch_class =
         { NULL,
             { "patch", false,
               VPORT_FUNCTIONS(get_patch_config,
                               set_patch_config,
                               NULL,
-                              NULL, NULL, NULL, NULL, NULL) }};
+                              NULL, NULL, NULL, NULL, NULL)},
+          {{0}} };
+
+    simap_init(&patch_class.global_cfg_tracker);
     netdev_register_provider(&patch_class.netdev_class);
 }
diff --git a/tests/tunnel.at b/tests/tunnel.at
index 2bc004cd8..3ac9cd83f 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -831,6 +831,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