diff mbox series

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

Message ID 20190222021016.6458-1-chrism@mellanox.com
State Superseded
Headers show
Series [ovs-dev,v2] netdev-vport: Use the dst_port in tunnel netdev name | expand

Commit Message

Chris Mi Feb. 22, 2019, 2:10 a.m. UTC 
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.

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

Comments

Ben Pfaff Feb. 22, 2019, 10:31 p.m. UTC | #1 
On Fri, Feb 22, 2019 at 10:10:16AM +0800, 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>
> ---
> 
> 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.

Thanks for the patch!

It looks to me that if 'name' and 'dpif_port' are exactly the same, then
this code will read past the null terminator in 'dpif_port':
+    if (!strncmp(name, dpif_port, strlen(dpif_port))) {
+        p = name + strlen(dpif_port) + 1;
+        port = atoi(p);
+    }

Thanks,

Ben.
Chris Mi April 1, 2019, 7:16 a.m. UTC | #2 
Any comment about this patch?

Thanks,
Chris

> -----Original Message-----
> From: Chris Mi <chrism@mellanox.com>
> Sent: Friday, February 22, 2019 10:10 AM
> To: fbl@sysclose.org; ovs-dev@openvswitch.org
> Cc: simon.horman@netronome.com; Roi Dayan <roid@mellanox.com>; Chris
> Mi <chrism@mellanox.com>
> Subject: [ovs-dev][PATCH v2] 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.
> 
>  lib/netdev-vport.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 808a43f99..6c8d99ae6 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -189,22 +189,33 @@ 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 (!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;
> --
> 2.14.4
Roi Dayan April 1, 2019, 9:24 a.m. UTC | #3 
Hi Chris,

We missed a reply from Ben.

Thanks,
Roi


On 23/02/2019 00:31, Ben Pfaff wrote:
> On Fri, Feb 22, 2019 at 10:10:16AM +0800, 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>
>> ---
>>
>> 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.
> 
> Thanks for the patch!
> 
> It looks to me that if 'name' and 'dpif_port' are exactly the same, then
> this code will read past the null terminator in 'dpif_port':
> +    if (!strncmp(name, dpif_port, strlen(dpif_port))) {
> +        p = name + strlen(dpif_port) + 1;
> +        port = atoi(p);
> +    }
> 
> Thanks,
> 
> Ben.

thanks

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&amp;data=02%7C01%7Croid%40mellanox.com%7C8f2606644f12424838bd08d69915ee86%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636864716813284651&amp;sdata=qV4EkAL6jHroYEOGlZkv3YLuZelEc6E8osQDa%2FPD18Y%3D&amp;reserved=0
>
Chris Mi April 1, 2019, 12:40 p.m. UTC | #4 
Hi Ben,

I'm sorry that we missed your comment for so long time.
I added another check to address your comment. Hopefully it is enough.
Please help us review again.

Thanks,
Chris

> -----Original Message-----
> From: Roi Dayan
> Sent: Monday, April 1, 2019 5:24 PM
> To: Ben Pfaff <blp@ovn.org>; Chris Mi <chrism@mellanox.com>
> Cc: fbl@sysclose.org; simon.horman@netronome.com; ovs-
> dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2] netdev-vport: Use the dst_port in tunnel
> netdev name
> 
> Hi Chris,
> 
> We missed a reply from Ben.
> 
> Thanks,
> Roi
> 
> 
> On 23/02/2019 00:31, Ben Pfaff wrote:
> > On Fri, Feb 22, 2019 at 10:10:16AM +0800, 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>
> >> ---
> >>
> >> 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.
> >
> > Thanks for the patch!
> >
> > It looks to me that if 'name' and 'dpif_port' are exactly the same,
> > then this code will read past the null terminator in 'dpif_port':
> > +    if (!strncmp(name, dpif_port, strlen(dpif_port))) {
> > +        p = name + strlen(dpif_port) + 1;
> > +        port = atoi(p);
> > +    }
> >
> > Thanks,
> >
> > Ben.
> 
> thanks
> 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> i
> > l.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
> dev&amp;data=02%7C01%7Cro
> >
> id%40mellanox.com%7C8f2606644f12424838bd08d69915ee86%7Ca652971c7d
> 2e4d9
> >
> ba6a4d149256f461b%7C0%7C0%7C636864716813284651&amp;sdata=qV4EkA
> L6jHroY
> > EOGlZkv3YLuZelEc6E8osQDa%2FPD18Y%3D&amp;reserved=0
> >
diff mbox series

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 808a43f99..6c8d99ae6 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -189,22 +189,33 @@  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 (!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;