[ovs-dev] [PATCH v4] gre: Rename fallback devices to avoid udev's interference

Yifeng Sun pkusunyifeng at gmail.com
Mon Sep 17 20:40:27 UTC 2018


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 <pkusunyifeng at 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 <gvrose8192 at 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 <pkusunyifeng at 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
>>
>


More information about the dev mailing list