Hi Greg, I checked that the 'else' part in the above patch is actually executed. So it should be necessary to keep them.
Thanks, Yifeng On Mon, Sep 17, 2018 at 9:51 AM Yifeng Sun <pkusunyif...@gmail.com> wrote: > Hi Greg, > > Sure, thanks for the review. I will work on it. > > Yifeng > > On Mon, Sep 17, 2018 at 8:49 AM Gregory Rose <gvrose8...@gmail.com> wrote: > >> On 9/15/2018 9:24 PM, Yifeng Sun wrote: >> > 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 <pkusunyif...@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. >> > v3->v4: Fix by code review, also fix a potenial bug. >> > >> > datapath/linux/compat/ip6_gre.c | 5 +++-- >> > datapath/linux/compat/ip6_tunnel.c | 6 +++--- >> > datapath/linux/compat/ip_gre.c | 2 +- >> > datapath/linux/compat/ip_tunnel.c | 7 +++---- >> > 4 files changed, 10 insertions(+), 10 deletions(-) >> > >> > diff --git a/datapath/linux/compat/ip6_gre.c >> b/datapath/linux/compat/ip6_gre.c >> > index 00dbefc9b099..182785273c6f 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); >> > @@ -1712,7 +1712,8 @@ static int __net_init ip6gre_init_net(struct net >> *net) >> > struct ip6gre_net *ign = net_generic(net, ip6gre_net_id); >> > int err; >> > >> > - ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), >> "ip6gre0", >> > + ign->fb_tunnel_dev = alloc_netdev(sizeof(struct ip6_tnl), >> > + "ovs-ip6gre0", >> > 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..9f4bae7dd3d1 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: >> > @@ -2093,7 +2093,7 @@ static int __net_init ip6_tnl_init_net(struct net >> *net) >> > 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), >> "ovs-ip6tnl0", >> > 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..ae5d5a058663 100644 >> > --- a/datapath/linux/compat/ip_tunnel.c >> > +++ b/datapath/linux/compat/ip_tunnel.c >> > @@ -130,12 +130,11 @@ 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)) { >> > + err = snprintf(name, IFNAMSIZ - 3, "ovs-%s%%d", >> ops->kind); >> > + if (err >= IFNAMSIZ - 3) >> > err = -E2BIG; >> > + if (err < 0) >> > goto failed; >> > - } >> > - strlcpy(name, ops->kind, IFNAMSIZ); >> > - strncat(name, "%d", 2); >> > } >> > >> > ASSERT_RTNL(); >> >> Thanks Yifeng for all your work on this. >> >> This looks OK but I'm not sure the additional protections against a >> device name being too long are >> really needed. I think in all cases the parms->name will be filled out >> since OVS is the only user. >> Have you tested whether the else code path will ever be entered? >> Sometimes kernel code gets >> into our backports even though it would never be executed within the >> context of OVS. It would >> be good to check this. >> >> Thanks, >> >> - Greg >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev