| Message ID | 1554122243-125096-1-git-send-email-chrism@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] netdev-vport: Use the dst_port in tunnel netdev name | expand |
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
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.
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.
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;