[PATCH] net: clear iflink when moving to a new netns

[PATCH] net: clear iflink when moving to a new netns

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691

macvlan and vlan both use iflink to identify its lower device,
however, after such device is moved to the new netns, its iflink
would become meaningless as ifindex is per netns. So, instead of
forbid them moving to another netns, just clear this field so that
it will not be dumped at least.

Cc: David S. Miller <davem@davemloft.net>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Cong Wang <cwang@twopensource.com>
---
 net/core/dev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4ad1b78..5e88b0c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6608,12 +6608,11 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	dev_net_set(dev, net);
 
 	/* If there is an ifindex conflict assign a new one */
-	if (__dev_get_by_index(net, dev->ifindex)) {
-		int iflink = (dev->iflink == dev->ifindex);
+	if (__dev_get_by_index(net, dev->ifindex))
 		dev->ifindex = dev_new_index(net);
-		if (iflink)
-			dev->iflink = dev->ifindex;
-	}
+
+	/* Old iflink is meaningless in the new namespace */
+	dev->iflink = dev->ifindex;
 
 	/* Send a netdev-add uevent to the new namespace */
 	kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
-- 
1.8.3.1

[PATCH] macvlan: unregister net device when netdev_upper_dev_link() fails

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

rtnl_newlink() doesn't unregister it for us on failure.

Cc: Patrick McHardy <kaber@trash.net>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Cong Wang <cwang@twopensource.com>
---
 drivers/net/macvlan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 8433de4..a5d2189 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -879,14 +879,15 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	dev->priv_flags |= IFF_MACVLAN;
 	err = netdev_upper_dev_link(lowerdev, dev);
 	if (err)
-		goto destroy_port;
-
+		goto unregister_netdev;
 
 	list_add_tail_rcu(&vlan->list, &port->vlans);
 	netif_stacked_transfer_operstate(lowerdev, dev);
 
 	return 0;
 
+unregister_netdev:
+	unregister_netdevice(dev);
 destroy_port:
 	port->count -= 1;
 	if (!port->count)
-- 
1.8.3.1

[PATCH] net: correct error path in rtnl_newlink()

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

I saw the following BUG when ->newlink() fails in rtnl_newlink():

[   40.240058] kernel BUG at net/core/dev.c:6438!

this is due to free_netdev() is not supposed to be called before
netdev is completely unregistered, therefore it is not correct
to call free_netdev() here, at least for ops->newlink!=NULL case,
many drivers call it in ->destructor so that rtnl_unlock() will
take care of it, we probably don't need to do anything here.

Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Cong Wang <cwang@twopensource.com>
---
 net/core/rtnetlink.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 048dc8d..1a0dac2 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1963,16 +1963,21 @@ replay:
 
 		dev->ifindex = ifm->ifi_index;
 
-		if (ops->newlink)
+		if (ops->newlink) {
 			err = ops->newlink(net, dev, tb, data);
-		else
+			/* Drivers should call free_netdev() in ->destructor
+			 * and unregister it on failure so that device could be
+			 * finally freed in rtnl_unlock.
+			 */
+			if (err < 0)
+				goto out;
+		} else {
 			err = register_netdevice(dev);
-
-		if (err < 0) {
-			free_netdev(dev);
-			goto out;
+			if (err < 0) {
+				free_netdev(dev);
+				goto out;
+			}
 		}
-
 		err = rtnl_configure_link(dev, ifm);
 		if (err < 0)
 			unregister_netdevice(dev);
-- 
1.8.3.1

Re: [PATCH] net: clear iflink when moving to a new netns

[Nicolas Dichtel]

Le 12/02/2014 00:51, Cong Wang a écrit :
> From: Cong Wang <cwang@twopensource.com>
>
> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
>
> macvlan and vlan both use iflink to identify its lower device,
> however, after such device is moved to the new netns, its iflink
> would become meaningless as ifindex is per netns. So, instead of
> forbid them moving to another netns, just clear this field so that
> it will not be dumped at least.
>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
I wonder if this patch breaks things in ip tunnels.
For example, ip6_tunnel uses iflink to find tunnels that are bound to an interface.
If you reset this field, ipip6_tunnel_lookup() will fail when the tunnel moves
to another netns.

Re: [PATCH] net: clear iflink when moving to a new netns

[Stephen Hemminger]

On Tue, 11 Feb 2014 15:51:28 -0800
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> From: Cong Wang <cwang@twopensource.com>
> 
> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
> 
> macvlan and vlan both use iflink to identify its lower device,
> however, after such device is moved to the new netns, its iflink
> would become meaningless as ifindex is per netns. So, instead of
> forbid them moving to another netns, just clear this field so that
> it will not be dumped at least.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
> ---
>  net/core/dev.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4ad1b78..5e88b0c2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6608,12 +6608,11 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
>  	dev_net_set(dev, net);
>  
>  	/* If there is an ifindex conflict assign a new one */
> -	if (__dev_get_by_index(net, dev->ifindex)) {
> -		int iflink = (dev->iflink == dev->ifindex);
> +	if (__dev_get_by_index(net, dev->ifindex))
>  		dev->ifindex = dev_new_index(net);
> -		if (iflink)
> -			dev->iflink = dev->ifindex;
> -	}
> +
> +	/* Old iflink is meaningless in the new namespace */
> +	dev->iflink = dev->ifindex;
>  
>  	/* Send a netdev-add uevent to the new namespace */
>  	kobject_uevent(&dev->dev.kobj, KOBJ_ADD);

This also breaks propogation of state changes from lower device
to upper device. Things like carrier and up/down.

Re: [PATCH] net: clear iflink when moving to a new netns

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Tue, 2014-02-11 at 15:51 -0800, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
> 
> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
> 
> macvlan and vlan both use iflink to identify its lower device,
> however, after such device is moved to the new netns, its iflink
> would become meaningless as ifindex is per netns. So, instead of
> forbid them moving to another netns, just clear this field so that
> it will not be dumped at least.
[...]

And what if it's moved back into the same netns?

I think iflink should be changed to a net_device pointer, so it remains
valid for in-kernel users but rtnetlink can do the netns check before
revealing it to userland.

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] net: clear iflink when moving to a new netns

[Cong Wang]

On Wed, Feb 12, 2014 at 7:43 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 12/02/2014 00:51, Cong Wang a écrit :
>
>> From: Cong Wang <cwang@twopensource.com>
>>
>> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
>>
>> macvlan and vlan both use iflink to identify its lower device,
>> however, after such device is moved to the new netns, its iflink
>> would become meaningless as ifindex is per netns. So, instead of
>> forbid them moving to another netns, just clear this field so that
>> it will not be dumped at least.
>>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> Signed-off-by: Cong Wang <cwang@twopensource.com>
>
> I wonder if this patch breaks things in ip tunnels.
> For example, ip6_tunnel uses iflink to find tunnels that are bound to an
> interface.
> If you reset this field, ipip6_tunnel_lookup() will fail when the tunnel
> moves
> to another netns.

Most tunnels set NETIF_F_NETNS_LOCAL, ip6_tunnel should set it too
(need a patch). So this is not a problem.

Re: [PATCH] net: clear iflink when moving to a new netns

[Cong Wang]

On Wed, Feb 12, 2014 at 8:33 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This also breaks propogation of state changes from lower device
> to upper device. Things like carrier and up/down.
>

macvlan_device_event() handles this pretty well by using a pointer to
net_device.
I don't see it is a problem from the code.

Re: [PATCH] net: clear iflink when moving to a new netns

[Cong Wang]

On Wed, Feb 12, 2014 at 3:18 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Tue, 2014-02-11 at 15:51 -0800, Cong Wang wrote:
>> From: Cong Wang <cwang@twopensource.com>
>>
>> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
>>
>> macvlan and vlan both use iflink to identify its lower device,
>> however, after such device is moved to the new netns, its iflink
>> would become meaningless as ifindex is per netns. So, instead of
>> forbid them moving to another netns, just clear this field so that
>> it will not be dumped at least.
> [...]
>
> And what if it's moved back into the same netns?
>

Good point!

> I think iflink should be changed to a net_device pointer, so it remains
> valid for in-kernel users but rtnetlink can do the netns check before
> revealing it to userland.
>

I will try this approach.

Thanks.

Re: [PATCH] net: clear iflink when moving to a new netns

[Eric W. Biederman]

Cong Wang <cwang@twopensource.com> writes:

> On Wed, Feb 12, 2014 at 7:43 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>> Le 12/02/2014 00:51, Cong Wang a écrit :
>>
>>> From: Cong Wang <cwang@twopensource.com>
>>>
>>> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
>>>
>>> macvlan and vlan both use iflink to identify its lower device,
>>> however, after such device is moved to the new netns, its iflink
>>> would become meaningless as ifindex is per netns. So, instead of
>>> forbid them moving to another netns, just clear this field so that
>>> it will not be dumped at least.
>>>
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> Signed-off-by: Cong Wang <cwang@twopensource.com>
>>
>> I wonder if this patch breaks things in ip tunnels.
>> For example, ip6_tunnel uses iflink to find tunnels that are bound to an
>> interface.
>> If you reset this field, ipip6_tunnel_lookup() will fail when the tunnel
>> moves
>> to another netns.
>
> Most tunnels set NETIF_F_NETNS_LOCAL, ip6_tunnel should set it too
> (need a patch). So this is not a problem.

There was an effort not long ago to make tunnels safe to pass between
namespaces.  NETIF_F_NETNS_LOCAL was removed from ip6_tunnel in that
effort.  Apparently something was overlooked.

Making iflink a netdevice reference or finding a way to remove it
entirely seems better that masking the problem.

Eric

Re: [PATCH] net: clear iflink when moving to a new netns

[Eric W. Biederman]

Cong Wang <cwang@twopensource.com> writes:

> On Wed, Feb 12, 2014 at 8:33 AM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>>
>> This also breaks propogation of state changes from lower device
>> to upper device. Things like carrier and up/down.
>>
>
> macvlan_device_event() handles this pretty well by using a pointer to
> net_device.
> I don't see it is a problem from the code.

A quick grep shows two uses of iflink in net/core/link_watch.c that are
likely broken by your proposed change.

Eric

Re: [PATCH] macvlan: unregister net device when netdev_upper_dev_link() fails

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 11 Feb 2014 15:51:29 -0800

> From: Cong Wang <cwang@twopensource.com>
> 
> rtnl_newlink() doesn't unregister it for us on failure.
> 
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>

Applied.

Re: [PATCH] net: correct error path in rtnl_newlink()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 11 Feb 2014 15:51:30 -0800

> From: Cong Wang <cwang@twopensource.com>
> 
> I saw the following BUG when ->newlink() fails in rtnl_newlink():
> 
> [   40.240058] kernel BUG at net/core/dev.c:6438!
> 
> this is due to free_netdev() is not supposed to be called before
> netdev is completely unregistered, therefore it is not correct
> to call free_netdev() here, at least for ops->newlink!=NULL case,
> many drivers call it in ->destructor so that rtnl_unlock() will
> take care of it, we probably don't need to do anything here.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>

Applied.

Re: [PATCH] net: clear iflink when moving to a new netns

[Cong Wang]

On Wed, Feb 12, 2014 at 6:00 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> There was an effort not long ago to make tunnels safe to pass between
> namespaces.  NETIF_F_NETNS_LOCAL was removed from ip6_tunnel in that
> effort.  Apparently something was overlooked.
>
> Making iflink a netdevice reference or finding a way to remove it
> entirely seems better that masking the problem.
>

Yeah, looks like the trend is allowing more stacked devices to move to
a new netns.
Sounds good. I am working on a draft patch now.

Thanks.