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

From: Cong Wang <redacted>

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 <redacted>
Cc: Eric Dumazet <redacted>
Cc: Hannes Frederic Sowa <redacted>,
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>
---
 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

From: Cong Wang <redacted>

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

Cc: Patrick McHardy <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>
---
 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

From: Cong Wang <redacted>

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 <redacted>
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>
---
 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

Le 12/02/2014 00:51, Cong Wang a écrit :

From: Cong Wang <redacted>

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 <redacted>
Cc: Eric Dumazet <redacted>
Cc: Hannes Frederic Sowa <redacted>,
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>

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.

On Tue, 11 Feb 2014 15:51:28 -0800
Cong Wang [off-list ref] wrote:

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

On Tue, 2014-02-11 at 15:51 -0800, Cong Wang wrote:

From: Cong Wang <redacted>

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.

On Wed, Feb 12, 2014 at 7:43 AM, Nicolas Dichtel
[off-list ref] wrote:

Le 12/02/2014 00:51, Cong Wang a écrit :

quoted

From: Cong Wang <redacted>

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 <redacted>
Cc: Eric Dumazet <redacted>
Cc: Hannes Frederic Sowa <redacted>,
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>

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.

On Wed, Feb 12, 2014 at 8:33 AM, Stephen Hemminger
[off-list ref] 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.

On Wed, Feb 12, 2014 at 3:18 PM, Ben Hutchings [off-list ref] wrote:

On Tue, 2014-02-11 at 15:51 -0800, Cong Wang wrote:

quoted

From: Cong Wang <redacted>

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.

Cong Wang [off-list ref] writes:

On Wed, Feb 12, 2014 at 7:43 AM, Nicolas Dichtel
[off-list ref] wrote:

quoted

Le 12/02/2014 00:51, Cong Wang a écrit :

quoted

From: Cong Wang <redacted>

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 <redacted>
Cc: Eric Dumazet <redacted>
Cc: Hannes Frederic Sowa <redacted>,
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>

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

Cong Wang [off-list ref] writes:

On Wed, Feb 12, 2014 at 8:33 AM, Stephen Hemminger
[off-list ref] wrote:

quoted

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

From: Cong Wang <redacted>
Date: Tue, 11 Feb 2014 15:51:29 -0800

From: Cong Wang <redacted>

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

Cc: Patrick McHardy <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>

Applied.

From: Cong Wang <redacted>
Date: Tue, 11 Feb 2014 15:51:30 -0800

From: Cong Wang <redacted>

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 <redacted>
Signed-off-by: Cong Wang <redacted>
Signed-off-by: Cong Wang <redacted>

Applied.

On Wed, Feb 12, 2014 at 6:00 PM, Eric W. Biederman
[off-list ref] 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.