On Mon, Sep 17, 2018 at 9:51 AM Yifeng Sun <pkusunyif...@gmail.com
<mailto: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
<mailto: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
<mailto: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
