| Message ID | 1536956449-3001-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] gre: Rename fallback devices to avoid udev's interference | expand |
> On Sep 14, 2018, at 1:20 PM, Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > @@ -1710,9 +1710,14 @@ static void ip6gre_destroy_tunnels(struct net *net, struct list_head *head) > static int __net_init ip6gre_init_net(struct net *net) > { > struct ip6gre_net *ign = net_generic(net, ip6gre_net_id); > + const char *dev_name = "ovs-ip6gre0"; > int err; > > - ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6gre0", > + if (strlen(dev_name) > IFNAMSIZ) > + return -E2BIG; > + > + ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), > + dev_name, > NET_NAME_UNKNOWN, > ip6gre_tunnel_setup); This string is allocated statically, so I don't think you need to do any checks on its length. > if (!ign->fb_tunnel_dev) { > diff --git a/datapath/linux/compat/ip6_tunnel.c b/datapath/linux/compat/ip6_tunnel.c > index 56fd8b4dd342..a7a1a586c920 100644 > --- a/datapath/linux/compat/ip6_tunnel.c > +++ b/datapath/linux/compat/ip6_tunnel.c > @@ -355,7 +355,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net *net, struct __ip6_tnl_parm *p) > if (p->name[0]) > strlcpy(name, p->name, IFNAMSIZ); > else > - sprintf(name, "ip6tnl%%d"); > + strlcpy(name, "ovs-ip6tnl%%d", IFNAMSIZ); I think you need to drop one of the "%", since it changed from a sprintf() to a strlcpy(). The string length concern I'd raised in v1 was around the run-time replacement of these "%d" arguments. I checked with Greg, and he said that the number is not greater than three digits, so we should be fine with this change, since we have room for up to a five digit number. > @@ -2087,13 +2087,17 @@ static int __net_init ip6_tnl_init_net(struct net *net) > { > struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id); > struct ip6_tnl *t = NULL; > + const char *dev_name = "ovs-ip6tnl0"; > int err; > > + if (strlen(dev_name) > IFNAMSIZ) > + return -E2BIG; > + > ip6n->tnls[0] = ip6n->tnls_wc; > ip6n->tnls[1] = ip6n->tnls_r_l; > > err = -ENOMEM; > - ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6tnl0", > + ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), dev_name, > NET_NAME_UNKNOWN, ip6_tnl_dev_setup); Again, I don't think you need run-time checks for this statically allocated string. > static void __net_exit ipgre_tap_exit_net(struct net *net) > diff --git a/datapath/linux/compat/ip_tunnel.c b/datapath/linux/compat/ip_tunnel.c > index d16e60fbfef0..0da8a06ecd26 100644 > --- a/datapath/linux/compat/ip_tunnel.c > +++ b/datapath/linux/compat/ip_tunnel.c > @@ -130,11 +130,12 @@ static struct net_device *__ip_tunnel_create(struct net *net, > if (parms->name[0]) > strlcpy(name, parms->name, IFNAMSIZ); > else { > - if (strlen(ops->kind) > (IFNAMSIZ - 3)) { > + if (strlen(ops->kind) > (IFNAMSIZ - 7)) { I think is correct that we need to increase this by four, since we added "ovs-". However, if the largest replacement for "%d" is three digits, should that check upstream be for "IFNAMESIZ - 4" to account for the nul-terminator? This is a question that applies to the Linux kernel upstream, too. > err = -E2BIG; > goto failed; > } > - strlcpy(name, ops->kind, IFNAMSIZ); > + strlcpy(name, "ovs-", IFNAMSIZ); > + strlcat(name, ops->kind, IFNAMSIZ); > strncat(name, "%d", 2); This isn't new to your changes, but we've already done string length sanity checks earlier, so these aren't adding much, and we don't even both with the last one, which would be the most likely to overflow. Regardless, as we discussed offline, it might be just cleaner to use sprintf() or snprintf(). --Justin
Thanks for the review, I sent out a v4. On Fri, Sep 14, 2018 at 4:41 PM Justin Pettit <jpettit@ovn.org> wrote: > > > On Sep 14, 2018, at 1:20 PM, Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > > > @@ -1710,9 +1710,14 @@ static void ip6gre_destroy_tunnels(struct net > *net, struct list_head *head) > > static int __net_init ip6gre_init_net(struct net *net) > > { > > struct ip6gre_net *ign = net_generic(net, ip6gre_net_id); > > + const char *dev_name = "ovs-ip6gre0"; > > int err; > > > > - ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), > "ip6gre0", > > + if (strlen(dev_name) > IFNAMSIZ) > > + return -E2BIG; > > + > > + ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), > > + dev_name, > > NET_NAME_UNKNOWN, > > ip6gre_tunnel_setup); > > This string is allocated statically, so I don't think you need to do any > checks on its length. > > > if (!ign->fb_tunnel_dev) { > > diff --git a/datapath/linux/compat/ip6_tunnel.c > b/datapath/linux/compat/ip6_tunnel.c > > index 56fd8b4dd342..a7a1a586c920 100644 > > --- a/datapath/linux/compat/ip6_tunnel.c > > +++ b/datapath/linux/compat/ip6_tunnel.c > > @@ -355,7 +355,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net > *net, struct __ip6_tnl_parm *p) > > if (p->name[0]) > > strlcpy(name, p->name, IFNAMSIZ); > > else > > - sprintf(name, "ip6tnl%%d"); > > + strlcpy(name, "ovs-ip6tnl%%d", IFNAMSIZ); > > I think you need to drop one of the "%", since it changed from a sprintf() > to a strlcpy(). > > The string length concern I'd raised in v1 was around the run-time > replacement of these "%d" arguments. I checked with Greg, and he said that > the number is not greater than three digits, so we should be fine with this > change, since we have room for up to a five digit number. > > > @@ -2087,13 +2087,17 @@ static int __net_init ip6_tnl_init_net(struct > net *net) > > { > > struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id); > > struct ip6_tnl *t = NULL; > > + const char *dev_name = "ovs-ip6tnl0"; > > int err; > > > > + if (strlen(dev_name) > IFNAMSIZ) > > + return -E2BIG; > > + > > ip6n->tnls[0] = ip6n->tnls_wc; > > ip6n->tnls[1] = ip6n->tnls_r_l; > > > > err = -ENOMEM; > > - ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6tnl0", > > + ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), dev_name, > > NET_NAME_UNKNOWN, > ip6_tnl_dev_setup); > > Again, I don't think you need run-time checks for this statically > allocated string. > > > static void __net_exit ipgre_tap_exit_net(struct net *net) > > diff --git a/datapath/linux/compat/ip_tunnel.c > b/datapath/linux/compat/ip_tunnel.c > > index d16e60fbfef0..0da8a06ecd26 100644 > > --- a/datapath/linux/compat/ip_tunnel.c > > +++ b/datapath/linux/compat/ip_tunnel.c > > @@ -130,11 +130,12 @@ static struct net_device > *__ip_tunnel_create(struct net *net, > > if (parms->name[0]) > > strlcpy(name, parms->name, IFNAMSIZ); > > else { > > - if (strlen(ops->kind) > (IFNAMSIZ - 3)) { > > + if (strlen(ops->kind) > (IFNAMSIZ - 7)) { > > I think is correct that we need to increase this by four, since we added > "ovs-". However, if the largest replacement for "%d" is three digits, > should that check upstream be for "IFNAMESIZ - 4" to account for the > nul-terminator? This is a question that applies to the Linux kernel > upstream, too. > > > err = -E2BIG; > > goto failed; > > } > > - strlcpy(name, ops->kind, IFNAMSIZ); > > + strlcpy(name, "ovs-", IFNAMSIZ); > > + strlcat(name, ops->kind, IFNAMSIZ); > > strncat(name, "%d", 2); > > This isn't new to your changes, but we've already done string length > sanity checks earlier, so these aren't adding much, and we don't even both > with the last one, which would be the most likely to overflow. Regardless, > as we discussed offline, it might be just cleaner to use sprintf() or > snprintf(). > > --Justin > > >
diff --git a/datapath/linux/compat/ip6_gre.c b/datapath/linux/compat/ip6_gre.c index 00dbefc9b099..cbd002ed23f6 100644 --- a/datapath/linux/compat/ip6_gre.c +++ b/datapath/linux/compat/ip6_gre.c @@ -377,7 +377,7 @@ static struct ip6_tnl *ip6gre_tunnel_locate(struct net *net, if (parms->name[0]) strlcpy(name, parms->name, IFNAMSIZ); else - strcpy(name, "ip6gre%d"); + strlcpy(name, "ovs-ip6gre%d", IFNAMSIZ); dev = alloc_netdev(sizeof(*t), name, NET_NAME_UNKNOWN, ip6gre_tunnel_setup); @@ -1710,9 +1710,14 @@ static void ip6gre_destroy_tunnels(struct net *net, struct list_head *head) static int __net_init ip6gre_init_net(struct net *net) { struct ip6gre_net *ign = net_generic(net, ip6gre_net_id); + const char *dev_name = "ovs-ip6gre0"; int err; - ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6gre0", + if (strlen(dev_name) > IFNAMSIZ) + return -E2BIG; + + ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), + dev_name, NET_NAME_UNKNOWN, ip6gre_tunnel_setup); if (!ign->fb_tunnel_dev) { diff --git a/datapath/linux/compat/ip6_tunnel.c b/datapath/linux/compat/ip6_tunnel.c index 56fd8b4dd342..a7a1a586c920 100644 --- a/datapath/linux/compat/ip6_tunnel.c +++ b/datapath/linux/compat/ip6_tunnel.c @@ -355,7 +355,7 @@ static struct ip6_tnl *ip6_tnl_create(struct net *net, struct __ip6_tnl_parm *p) if (p->name[0]) strlcpy(name, p->name, IFNAMSIZ); else - sprintf(name, "ip6tnl%%d"); + strlcpy(name, "ovs-ip6tnl%%d", IFNAMSIZ); dev = alloc_netdev(sizeof(*t), name, NET_NAME_UNKNOWN, ip6_tnl_dev_setup); @@ -1410,7 +1410,7 @@ ip6_tnl_parm_to_user(struct ip6_tnl_parm *u, const struct __ip6_tnl_parm *p) * %SIOCCHGTUNNEL: change tunnel parameters to those given * %SIOCDELTUNNEL: delete tunnel * - * The fallback device "ip6tnl0", created during module + * The fallback device "ovs-ip6tnl0", created during module * initialization, can be used for creating other tunnel devices. * * Return: @@ -2087,13 +2087,17 @@ static int __net_init ip6_tnl_init_net(struct net *net) { struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id); struct ip6_tnl *t = NULL; + const char *dev_name = "ovs-ip6tnl0"; int err; + if (strlen(dev_name) > IFNAMSIZ) + return -E2BIG; + ip6n->tnls[0] = ip6n->tnls_wc; ip6n->tnls[1] = ip6n->tnls_r_l; err = -ENOMEM; - ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), "ip6tnl0", + ip6n->fb_tnl_dev = alloc_netdev(sizeof(struct ip6_tnl), dev_name, NET_NAME_UNKNOWN, ip6_tnl_dev_setup); if (!ip6n->fb_tnl_dev) diff --git a/datapath/linux/compat/ip_gre.c b/datapath/linux/compat/ip_gre.c index 05132ba9494a..b7322c58e420 100644 --- a/datapath/linux/compat/ip_gre.c +++ b/datapath/linux/compat/ip_gre.c @@ -1463,7 +1463,7 @@ static struct pernet_operations erspan_net_ops = { static int __net_init ipgre_tap_init_net(struct net *net) { - return ip_tunnel_init_net(net, gre_tap_net_id, &ipgre_tap_ops, "gretap0"); + return ip_tunnel_init_net(net, gre_tap_net_id, &ipgre_tap_ops, "ovs-gretap0"); } static void __net_exit ipgre_tap_exit_net(struct net *net) diff --git a/datapath/linux/compat/ip_tunnel.c b/datapath/linux/compat/ip_tunnel.c index d16e60fbfef0..0da8a06ecd26 100644 --- a/datapath/linux/compat/ip_tunnel.c +++ b/datapath/linux/compat/ip_tunnel.c @@ -130,11 +130,12 @@ static struct net_device *__ip_tunnel_create(struct net *net, if (parms->name[0]) strlcpy(name, parms->name, IFNAMSIZ); else { - if (strlen(ops->kind) > (IFNAMSIZ - 3)) { + if (strlen(ops->kind) > (IFNAMSIZ - 7)) { err = -E2BIG; goto failed; } - strlcpy(name, ops->kind, IFNAMSIZ); + strlcpy(name, "ovs-", IFNAMSIZ); + strlcat(name, ops->kind, IFNAMSIZ); strncat(name, "%d", 2); }
On certain kernel versions, when openvswitch kernel module creates a gre0 interface, the kernel’s gre module will jump out and compete to control the gre0 interface. This will cause the failure of openvswitch kernel module loading. This fix renames fallback devices by adding a prefix "ovs-". Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> VMware Issue: #2162866 --- Please backport this patch to upstream OVS down to 2.9, thanks. v1->v2: Added sanity check for device names, thanks Justin. v2->v3: Fix an indent error. datapath/linux/compat/ip6_gre.c | 9 +++++++-- datapath/linux/compat/ip6_tunnel.c | 10 +++++++--- datapath/linux/compat/ip_gre.c | 2 +- datapath/linux/compat/ip_tunnel.c | 5 +++-- 4 files changed, 18 insertions(+), 8 deletions(-)