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

Eelco Chaudron echaudro at redhat.com
Thu Apr 12 09:46:46 UTC 2018


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 at 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




More information about the dev mailing list