[PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

The function vxlan_dev_create() (only used by ovs) never calls
rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
never set to RTNL_LINK_INITIALIZED.
During the deletion phase, the function rollback_registered_many() sends
a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.

Fixes: dcc38c033b32 ("openvswitch: Re-add CONFIG_OPENVSWITCH_VXLAN")
CC: Thomas Graf <tgraf@suug.ch>
CC: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/vxlan.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index f999db2f97b4..f7669d60b3f7 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2972,6 +2972,12 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
 		return ERR_PTR(err);
 	}
 
+	err = rtnl_configure_link(dev, NULL);
+	if (err < 0) {
+		free_netdev(dev);
+		return ERR_PTR(err);
+	}
+
 	return dev;
 }
 EXPORT_SYMBOL_GPL(vxlan_dev_create);
-- 
2.4.2

[PATCH net 2/2] ovs/gre: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

The function gretap_fb_dev_create() (only used by ovs) never calls
rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
never set to RTNL_LINK_INITIALIZED.
During the deletion phase, the function rollback_registered_many() sends
a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.

Fixes: b2acd1dc3949 ("openvswitch: Use regular GRE net_device instead of vport")
CC: Thomas Graf <tgraf@suug.ch>
CC: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ip_gre.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 4d2025f7ec57..c6a46ee87a3a 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1146,6 +1146,10 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 	if (err)
 		goto out;
 
+	err = rtnl_configure_link(dev, NULL);
+	if (err < 0)
+		goto out;
+
 	return dev;
 out:
 	free_netdev(dev);
-- 
2.4.2

Re: [PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[pravin shelar]

On Fri, Jun 10, 2016 at 2:32 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> The function vxlan_dev_create() (only used by ovs) never calls
> rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
> never set to RTNL_LINK_INITIALIZED.
> During the deletion phase, the function rollback_registered_many() sends
> a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.
>
> Fixes: dcc38c033b32 ("openvswitch: Re-add CONFIG_OPENVSWITCH_VXLAN")
> CC: Thomas Graf <tgraf@suug.ch>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  drivers/net/vxlan.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index f999db2f97b4..f7669d60b3f7 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2972,6 +2972,12 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
>                 return ERR_PTR(err);
>         }
>
> +       err = rtnl_configure_link(dev, NULL);
> +       if (err < 0) {
> +               free_netdev(dev);
> +               return ERR_PTR(err);
> +       }
> +
Patch looks good except the error handling code. earlier call to
vxlan_dev_configure() registers the net-device  and add the device to
vxlan device list. it needs to be undone before freeing the device.

Re: [PATCH net 2/2] ovs/gre: fix rtnl notifications on iface deletion

[pravin shelar]

On Fri, Jun 10, 2016 at 2:32 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> The function gretap_fb_dev_create() (only used by ovs) never calls
> rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
> never set to RTNL_LINK_INITIALIZED.
> During the deletion phase, the function rollback_registered_many() sends
> a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.
>
> Fixes: b2acd1dc3949 ("openvswitch: Use regular GRE net_device instead of vport")
> CC: Thomas Graf <tgraf@suug.ch>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/ipv4/ip_gre.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 4d2025f7ec57..c6a46ee87a3a 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -1146,6 +1146,10 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
>         if (err)
>                 goto out;
>
> +       err = rtnl_configure_link(dev, NULL);
> +       if (err < 0)
> +               goto out;
> +

As the first patch, similar issue exist here. But existing error
handling code already is buggy.

>         return dev;
>  out:
>         free_netdev(dev);
> --
> 2.4.2
>

Re: [PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[Jesse Gross]

On Fri, Jun 10, 2016 at 2:32 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> The function vxlan_dev_create() (only used by ovs) never calls
> rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
> never set to RTNL_LINK_INITIALIZED.
> During the deletion phase, the function rollback_registered_many() sends
> a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.
>
> Fixes: dcc38c033b32 ("openvswitch: Re-add CONFIG_OPENVSWITCH_VXLAN")
> CC: Thomas Graf <tgraf@suug.ch>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Does Geneve need this as well?

Re: [PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

Le 10/06/2016 19:50, pravin shelar a écrit :
[snip]
> Patch looks good except the error handling code. earlier call to
> vxlan_dev_configure() registers the net-device  and add the device to
> vxlan device list. it needs to be undone before freeing the device.
> 
Ok, will fix it (and also for GRE).


Thank you,
Nicolas

Re: [PATCH net 1/2] ovs/vxlan: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

Le 10/06/2016 19:52, Jesse Gross a écrit :
[snip]
> Does Geneve need this as well?
> 
Yes, will fix it.


Thank you,
Nicolas

[PATCH net v2 0/4] ovs: fix rtnl notifications on interface deletion

[Nicolas Dichtel]

There was no rtnl notifications for interfaces (gre, vxlan, geneve) created
by ovs. This problem is fixed by adjusting the creation path.

v1 -> v2:
 - add patch #1 and #4
 - rework error handling in patch #2

 drivers/net/geneve.c | 14 ++++++++++---
 drivers/net/vxlan.c  | 58 ++++++++++++++++++++++++++++++----------------------
 net/ipv4/ip_gre.c    | 14 ++++++++++---
 3 files changed, 56 insertions(+), 30 deletions(-)

Regards,
Nicolas

[PATCH net v2 1/4] ovs/gre,geneve: fix error path when creating an iface

[Nicolas Dichtel]

After ipgre_newlink()/geneve_configure() call, the netdev is registered.

Fixes: 7e059158d57b ("vxlan, gre, geneve: Set a large MTU on ovs-created tunnel devices")
CC: David Wragg <david@weave.works>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/geneve.c | 10 +++++++---
 net/ipv4/ip_gre.c    | 10 +++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index cadefe4fdaa2..086c2dae4c3d 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1508,6 +1508,7 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 {
 	struct nlattr *tb[IFLA_MAX + 1];
 	struct net_device *dev;
+	LIST_HEAD(list_kill);
 	int err;
 
 	memset(tb, 0, sizeof(tb));
@@ -1519,8 +1520,10 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 	err = geneve_configure(net, dev, &geneve_remote_unspec,
 			       0, 0, 0, 0, htons(dst_port), true,
 			       GENEVE_F_UDP_ZERO_CSUM6_RX);
-	if (err)
-		goto err;
+	if (err) {
+		free_netdev(dev);
+		return ERR_PTR(err);
+	}
 
 	/* openvswitch users expect packet sizes to be unrestricted,
 	 * so set the largest MTU we can.
@@ -1532,7 +1535,8 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 	return dev;
 
  err:
-	free_netdev(dev);
+	geneve_dellink(dev, &list_kill);
+	unregister_netdevice_many(&list_kill);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(geneve_dev_create_fb);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 4d2025f7ec57..08deba679c8c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1121,6 +1121,7 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 {
 	struct nlattr *tb[IFLA_MAX + 1];
 	struct net_device *dev;
+	LIST_HEAD(list_kill);
 	struct ip_tunnel *t;
 	int err;
 
@@ -1136,8 +1137,10 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 	t->collect_md = true;
 
 	err = ipgre_newlink(net, dev, tb, NULL);
-	if (err < 0)
-		goto out;
+	if (err < 0) {
+		free_netdev(dev);
+		return ERR_PTR(err);
+	}
 
 	/* openvswitch users expect packet sizes to be unrestricted,
 	 * so set the largest MTU we can.
@@ -1148,7 +1151,8 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 
 	return dev;
 out:
-	free_netdev(dev);
+	ip_tunnel_dellink(dev, &list_kill);
+	unregister_netdevice_many(&list_kill);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(gretap_fb_dev_create);
-- 
2.4.2

[PATCH net v2 2/4] ovs/vxlan: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

The function vxlan_dev_create() (only used by ovs) never calls
rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
never set to RTNL_LINK_INITIALIZED.
During the deletion phase, the function rollback_registered_many() sends
a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.

Note that the function vxlan_dev_create() is moved after the rtnl stuff so
that vxlan_dellink() can be called in this function.

Fixes: dcc38c033b32 ("openvswitch: Re-add CONFIG_OPENVSWITCH_VXLAN")
CC: Thomas Graf <tgraf@suug.ch>
CC: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/vxlan.c | 58 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index f999db2f97b4..b3b9db68f758 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2952,30 +2952,6 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
 	return 0;
 }
 
-struct net_device *vxlan_dev_create(struct net *net, const char *name,
-				    u8 name_assign_type, struct vxlan_config *conf)
-{
-	struct nlattr *tb[IFLA_MAX+1];
-	struct net_device *dev;
-	int err;
-
-	memset(&tb, 0, sizeof(tb));
-
-	dev = rtnl_create_link(net, name, name_assign_type,
-			       &vxlan_link_ops, tb);
-	if (IS_ERR(dev))
-		return dev;
-
-	err = vxlan_dev_configure(net, dev, conf);
-	if (err < 0) {
-		free_netdev(dev);
-		return ERR_PTR(err);
-	}
-
-	return dev;
-}
-EXPORT_SYMBOL_GPL(vxlan_dev_create);
-
 static int vxlan_newlink(struct net *src_net, struct net_device *dev,
 			 struct nlattr *tb[], struct nlattr *data[])
 {
@@ -3268,6 +3244,40 @@ static struct rtnl_link_ops vxlan_link_ops __read_mostly = {
 	.get_link_net	= vxlan_get_link_net,
 };
 
+struct net_device *vxlan_dev_create(struct net *net, const char *name,
+				    u8 name_assign_type,
+				    struct vxlan_config *conf)
+{
+	struct nlattr *tb[IFLA_MAX + 1];
+	struct net_device *dev;
+	int err;
+
+	memset(&tb, 0, sizeof(tb));
+
+	dev = rtnl_create_link(net, name, name_assign_type,
+			       &vxlan_link_ops, tb);
+	if (IS_ERR(dev))
+		return dev;
+
+	err = vxlan_dev_configure(net, dev, conf);
+	if (err < 0) {
+		free_netdev(dev);
+		return ERR_PTR(err);
+	}
+
+	err = rtnl_configure_link(dev, NULL);
+	if (err < 0) {
+		LIST_HEAD(list_kill);
+
+		vxlan_dellink(dev, &list_kill);
+		unregister_netdevice_many(&list_kill);
+		return ERR_PTR(err);
+	}
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(vxlan_dev_create);
+
 static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
 					     struct net_device *dev)
 {
-- 
2.4.2

[PATCH net v2 3/4] ovs/gre: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

The function gretap_fb_dev_create() (only used by ovs) never calls
rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
never set to RTNL_LINK_INITIALIZED.
During the deletion phase, the function rollback_registered_many() sends
a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.

Fixes: b2acd1dc3949 ("openvswitch: Use regular GRE net_device instead of vport")
CC: Thomas Graf <tgraf@suug.ch>
CC: Pravin B Shelar <pshelar@nicira.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ip_gre.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 08deba679c8c..07c5cf1838d8 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1149,6 +1149,10 @@ struct net_device *gretap_fb_dev_create(struct net *net, const char *name,
 	if (err)
 		goto out;
 
+	err = rtnl_configure_link(dev, NULL);
+	if (err < 0)
+		goto out;
+
 	return dev;
 out:
 	ip_tunnel_dellink(dev, &list_kill);
-- 
2.4.2

[PATCH net v2 4/4] ovs/geneve: fix rtnl notifications on iface deletion

[Nicolas Dichtel]

The function geneve_dev_create_fb() (only used by ovs) never calls
rtnl_configure_link(). The consequence is that dev->rtnl_link_state is
never set to RTNL_LINK_INITIALIZED.
During the deletion phase, the function rollback_registered_many() sends
a RTM_DELLINK only if dev->rtnl_link_state is set to RTNL_LINK_INITIALIZED.

Fixes: e305ac6cf5a1 ("geneve: Add support to collect tunnel metadata.")
CC: Pravin B Shelar <pshelar@nicira.com>
CC: Jesse Gross <jesse@nicira.com>
CC: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/geneve.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 086c2dae4c3d..305a04e45a13 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1532,6 +1532,10 @@ struct net_device *geneve_dev_create_fb(struct net *net, const char *name,
 	if (err)
 		goto err;
 
+	err = rtnl_configure_link(dev, NULL);
+	if (err < 0)
+		goto err;
+
 	return dev;
 
  err:
-- 
2.4.2

Re: [PATCH net v2 0/4] ovs: fix rtnl notifications on interface deletion

[pravin shelar]

On Mon, Jun 13, 2016 at 1:31 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
>
> There was no rtnl notifications for interfaces (gre, vxlan, geneve) created
> by ovs. This problem is fixed by adjusting the creation path.
>
> v1 -> v2:
>  - add patch #1 and #4
>  - rework error handling in patch #2
>
>  drivers/net/geneve.c | 14 ++++++++++---
>  drivers/net/vxlan.c  | 58 ++++++++++++++++++++++++++++++----------------------
>  net/ipv4/ip_gre.c    | 14 ++++++++++---
>  3 files changed, 56 insertions(+), 30 deletions(-)
>

All patches looks good to me.

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH net v2 0/4] ovs: fix rtnl notifications on interface deletion

[David Miller]

From: pravin shelar <pshelar@ovn.org>
Date: Mon, 13 Jun 2016 12:39:16 -0700

> On Mon, Jun 13, 2016 at 1:31 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>>
>> There was no rtnl notifications for interfaces (gre, vxlan, geneve) created
>> by ovs. This problem is fixed by adjusting the creation path.
>>
>> v1 -> v2:
>>  - add patch #1 and #4
>>  - rework error handling in patch #2
>>
>>  drivers/net/geneve.c | 14 ++++++++++---
>>  drivers/net/vxlan.c  | 58 ++++++++++++++++++++++++++++++----------------------
>>  net/ipv4/ip_gre.c    | 14 ++++++++++---
>>  3 files changed, 56 insertions(+), 30 deletions(-)
>>
> 
> All patches looks good to me.
> 
> Acked-by: Pravin B Shelar <pshelar@ovn.org>

Series applied, thanks.