[ovs-dev,v3] netdev-vport: Use the dst_port in tunnel netdev name

Message ID 1554122243-125096-1-git-send-email-chrism@mellanox.com
State New
Headers show
Series
  • [ovs-dev,v3] netdev-vport: Use the dst_port in tunnel netdev name
Related show

Commit Message

Chris Mi April 1, 2019, 12:37 p.m.
If tunnel device dst_port is not the default one, "ovs-dpctl dump-flows"
will fail. The error message for vxlan is:

netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: No such device

That's because when calling netdev_vport_construct() for netdev
vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port
value is in the netdev name. Use it to avoid the error.

Signed-off-by: Chris Mi <chrism@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---

v1                                                                                                                                                                                     
==                                                                                                                                                                                     
                                                                                                                                                                                       
Any comment about this patch? We are not sure if it is correct                                                                                                                         
to verify the port from the name. If it is correct, is it applicable                                                                                                                   
for other tunnels? Thanks!                                                                                                                                                             
                                                                                                                                                                                       
v2                                                                                                                                                                                     
==                                                                                                                                                                                     
                                                                                                                                                                                       
Apply the same fix to other tunnel types according to Flavio Leitner's                                                                                                                 
comment. 

v3
==

Addressed Ben Pfaff's comment to deal with the string correctly.

 lib/netdev-vport.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Chris Mi April 12, 2019, 4:57 a.m. | #1
Any other comment about this patch?

Thanks,
Chris

> -----Original Message-----
> From: Chris Mi <chrism@mellanox.com>
> Sent: Monday, April 1, 2019 8:37 PM
> To: blp@ovn.org; ovs-dev@openvswitch.org; fbl@sysclose.org
> Cc: simon.horman@netronome.com; Roi Dayan <roid@mellanox.com>; Chris
> Mi <chrism@mellanox.com>
> Subject: [ovs-dev][PATCH v3] netdev-vport: Use the dst_port in tunnel
> netdev name
> 
> If tunnel device dst_port is not the default one, "ovs-dpctl dump-flows"
> will fail. The error message for vxlan is:
> 
> netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed:
> No such device
> 
> That's because when calling netdev_vport_construct() for netdev
> vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port value is
> in the netdev name. Use it to avoid the error.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---
> 
> v1
> ==
> 
> Any comment about this patch? We are not sure if it is correct
> to verify the port from the name. If it is correct, is it applicable
> for other tunnels? Thanks!
> 
> v2
> ==
> 
> Apply the same fix to other tunnel types according to Flavio Leitner's
> comment.
> 
> v3
> ==
> 
> Addressed Ben Pfaff's comment to deal with the string correctly.
> 
>  lib/netdev-vport.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 808a43f..7906980
> 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -189,22 +189,34 @@ netdev_vport_alloc(void)  int
> netdev_vport_construct(struct netdev *netdev_)  {
> +    const struct netdev_class *class = netdev_get_class(netdev_);
> +    const char *dpif_port = netdev_vport_class_get_dpif_port(class);
>      struct netdev_vport *dev = netdev_vport_cast(netdev_);
> +    const char *p, *name = netdev_get_name(netdev_);
>      const char *type = netdev_get_type(netdev_);
> +    uint16_t port = 0;
> 
>      ovs_mutex_init(&dev->mutex);
>      eth_addr_random(&dev->etheraddr);
> 
> -    /* Add a default destination port for tunnel ports if none specified. */
> +    if ((strlen(name) > strlen(dpif_port) + 1) &&
> +        (!strncmp(name, dpif_port, strlen(dpif_port)))) {
> +        p = name + strlen(dpif_port) + 1;
> +        port = atoi(p);
> +    }
> +
> +    /* If a destination port for tunnel ports is specified in the netdev
> +     * name, use it instead of the default one. Otherwise, use the default
> +     * destination port */
>      if (!strcmp(type, "geneve")) {
> -        dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT);
> +        dev->tnl_cfg.dst_port = port ? htons(port) :
> + htons(GENEVE_DST_PORT);
>      } else if (!strcmp(type, "vxlan")) {
> -        dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT);
> +        dev->tnl_cfg.dst_port = port ? htons(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);
> +        dev->tnl_cfg.dst_port = port ? htons(port) :
> + htons(LISP_DST_PORT);
>      } else if (!strcmp(type, "stt")) {
> -        dev->tnl_cfg.dst_port = htons(STT_DST_PORT);
> +        dev->tnl_cfg.dst_port = port ? htons(port) :
> + htons(STT_DST_PORT);
>      }
> 
>      dev->tnl_cfg.dont_fragment = true;
> --
> 1.8.3.1
Ben Pfaff April 12, 2019, 5:37 p.m. | #2
On Mon, Apr 01, 2019 at 08:37:23AM -0400, Chris Mi wrote:
> If tunnel device dst_port is not the default one, "ovs-dpctl dump-flows"
> will fail. The error message for vxlan is:
> 
> netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed: No such device
> 
> That's because when calling netdev_vport_construct() for netdev
> vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port
> value is in the netdev name. Use it to avoid the error.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>

Thanks for the bug fix.

This makes lots of tests fail.  Here's a typical Address Sanitizer
report:

=================================================================
==17273==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f59d4295571 bp 0x7ffd20ad0cf0 sp 0x7ffd20ad0478 T0)
==17273==The signal is caused by a READ memory access.
==17273==Hint: address points to the zero page.
    #0 0x7f59d4295570  (/lib/x86_64-linux-gnu/libc.so.6+0x15c570)
    #1 0x7f59d49ba3bc  (/lib/x86_64-linux-gnu/libasan.so.5+0xa83bc)
    #2 0x555e5c332bc0 in netdev_vport_construct ../lib/netdev-vport.c:202
    #3 0x555e5c3366fc in netdev_open ../lib/netdev.c:426
    #4 0x555e5c161916 in iface_do_create ../vswitchd/bridge.c:1789
    #5 0x555e5c161916 in iface_create ../vswitchd/bridge.c:1842
    #6 0x555e5c161916 in bridge_add_ports__ ../vswitchd/bridge.c:936
    #7 0x555e5c16e656 in bridge_add_ports ../vswitchd/bridge.c:952
    #8 0x555e5c16e656 in bridge_reconfigure ../vswitchd/bridge.c:666
    #9 0x555e5c17584e in bridge_run ../vswitchd/bridge.c:3037
    #10 0x555e5c157534 in main ../vswitchd/ovs-vswitchd.c:125
    #11 0x7f59d415d09a in __libc_start_main ../csu/libc-start.c:308
    #12 0x555e5c159f79 in _start (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x11af79)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x15c570) 
==17273==ABORTING

Thanks,

Ben.
Chris Mi April 13, 2019, 8:18 a.m. | #3
Hi Ben,

> -----Original Message-----
> From: Ben Pfaff <blp@ovn.org>
> Sent: Saturday, April 13, 2019 1:38 AM
> To: Chris Mi <chrism@mellanox.com>
> Cc: ovs-dev@openvswitch.org; fbl@sysclose.org;
> simon.horman@netronome.com; Roi Dayan <roid@mellanox.com>
> Subject: Re: [ovs-dev][PATCH v3] netdev-vport: Use the dst_port in tunnel
> netdev name
> 
> On Mon, Apr 01, 2019 at 08:37:23AM -0400, Chris Mi wrote:
> > If tunnel device dst_port is not the default one, "ovs-dpctl dump-flows"
> > will fail. The error message for vxlan is:
> >
> > netdev_linux|INFO|ioctl(SIOCGIFINDEX) on vxlan_sys_4789 device failed:
> > No such device
> >
> > That's because when calling netdev_vport_construct() for netdev
> > vxlan_sys_xxxx, the default dst_port is used. Actually, the dst_port
> > value is in the netdev name. Use it to avoid the error.
> >
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> 
> Thanks for the bug fix.
> 
> This makes lots of tests fail.  Here's a typical Address Sanitizer
> report:
> 
> ==========================================================
> =======
> ==17273==ERROR: AddressSanitizer: SEGV on unknown address
> 0x000000000000 (pc 0x7f59d4295571 bp 0x7ffd20ad0cf0 sp 0x7ffd20ad0478 T0)
> ==17273==The signal is caused by a READ memory access.
> ==17273==Hint: address points to the zero page.
>     #0 0x7f59d4295570  (/lib/x86_64-linux-gnu/libc.so.6+0x15c570)
>     #1 0x7f59d49ba3bc  (/lib/x86_64-linux-gnu/libasan.so.5+0xa83bc)
>     #2 0x555e5c332bc0 in netdev_vport_construct ../lib/netdev-vport.c:202
>     #3 0x555e5c3366fc in netdev_open ../lib/netdev.c:426
>     #4 0x555e5c161916 in iface_do_create ../vswitchd/bridge.c:1789
>     #5 0x555e5c161916 in iface_create ../vswitchd/bridge.c:1842
>     #6 0x555e5c161916 in bridge_add_ports__ ../vswitchd/bridge.c:936
>     #7 0x555e5c16e656 in bridge_add_ports ../vswitchd/bridge.c:952
>     #8 0x555e5c16e656 in bridge_reconfigure ../vswitchd/bridge.c:666
>     #9 0x555e5c17584e in bridge_run ../vswitchd/bridge.c:3037
>     #10 0x555e5c157534 in main ../vswitchd/ovs-vswitchd.c:125
>     #11 0x7f59d415d09a in __libc_start_main ../csu/libc-start.c:308
>     #12 0x555e5c159f79 in _start (/home/blp/nicira/ovs/_build/vswitchd/ovs-
> vswitchd+0x11af79)
> 
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-
> gnu/libc.so.6+0x15c570)
> ==17273==ABORTING
Thanks for verifying it. I think the error is because strlen() can't deal with NULL pointer correctly.
I added another check for NULL pointer. Hopefully we're fine this time. Please help review v4 again.

Thanks,
Chris
> 
> Thanks,
> 
> Ben.

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 808a43f..7906980 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -189,22 +189,34 @@  netdev_vport_alloc(void)
 int
 netdev_vport_construct(struct netdev *netdev_)
 {
+    const struct netdev_class *class = netdev_get_class(netdev_);
+    const char *dpif_port = netdev_vport_class_get_dpif_port(class);
     struct netdev_vport *dev = netdev_vport_cast(netdev_);
+    const char *p, *name = netdev_get_name(netdev_);
     const char *type = netdev_get_type(netdev_);
+    uint16_t port = 0;
 
     ovs_mutex_init(&dev->mutex);
     eth_addr_random(&dev->etheraddr);
 
-    /* Add a default destination port for tunnel ports if none specified. */
+    if ((strlen(name) > strlen(dpif_port) + 1) &&
+        (!strncmp(name, dpif_port, strlen(dpif_port)))) {
+        p = name + strlen(dpif_port) + 1;
+        port = atoi(p);
+    }
+
+    /* If a destination port for tunnel ports is specified in the netdev
+     * name, use it instead of the default one. Otherwise, use the default
+     * destination port */
     if (!strcmp(type, "geneve")) {
-        dev->tnl_cfg.dst_port = htons(GENEVE_DST_PORT);
+        dev->tnl_cfg.dst_port = port ? htons(port) : htons(GENEVE_DST_PORT);
     } else if (!strcmp(type, "vxlan")) {
-        dev->tnl_cfg.dst_port = htons(VXLAN_DST_PORT);
+        dev->tnl_cfg.dst_port = port ? htons(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);
+        dev->tnl_cfg.dst_port = port ? htons(port) : htons(LISP_DST_PORT);
     } else if (!strcmp(type, "stt")) {
-        dev->tnl_cfg.dst_port = htons(STT_DST_PORT);
+        dev->tnl_cfg.dst_port = port ? htons(port) : htons(STT_DST_PORT);
     }
 
     dev->tnl_cfg.dont_fragment = true;