* [PATCH] macvlan: unregister net device when netdev_upper_dev_link() fails
2014-02-11 23:51 [PATCH] net: clear iflink when moving to a new netns Cong Wang
@ 2014-02-11 23:51 ` Cong Wang
2014-02-13 22:13 ` David Miller
2014-02-11 23:51 ` [PATCH] net: correct error path in rtnl_newlink() Cong Wang
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-02-11 23:51 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Patrick McHardy, David S. Miller, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH] net: correct error path in rtnl_newlink()
2014-02-11 23:51 [PATCH] net: clear iflink when moving to a new netns Cong Wang
2014-02-11 23:51 ` [PATCH] macvlan: unregister net device when netdev_upper_dev_link() fails Cong Wang
@ 2014-02-11 23:51 ` Cong Wang
2014-02-13 22:13 ` David Miller
2014-02-12 15:43 ` [PATCH] net: clear iflink when moving to a new netns Nicolas Dichtel
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-02-11 23:51 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller, Eric Dumazet, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] net: correct error path in rtnl_newlink()
2014-02-11 23:51 ` [PATCH] net: correct error path in rtnl_newlink() Cong Wang
@ 2014-02-13 22:13 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2014-02-13 22:13 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, eric.dumazet, cwang
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-11 23:51 [PATCH] net: clear iflink when moving to a new netns Cong Wang
2014-02-11 23:51 ` [PATCH] macvlan: unregister net device when netdev_upper_dev_link() fails Cong Wang
2014-02-11 23:51 ` [PATCH] net: correct error path in rtnl_newlink() Cong Wang
@ 2014-02-12 15:43 ` Nicolas Dichtel
2014-02-13 1:18 ` Cong Wang
2014-02-12 16:33 ` Stephen Hemminger
2014-02-12 23:18 ` Ben Hutchings
4 siblings, 1 reply; 14+ messages in thread
From: Nicolas Dichtel @ 2014-02-12 15:43 UTC (permalink / raw)
To: Cong Wang, netdev
Cc: David S. Miller, Eric W. Biederman, Eric Dumazet,
Hannes Frederic Sowa, Cong Wang
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-12 15:43 ` [PATCH] net: clear iflink when moving to a new netns Nicolas Dichtel
@ 2014-02-13 1:18 ` Cong Wang
2014-02-13 2:00 ` Eric W. Biederman
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-02-13 1:18 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: Cong Wang, netdev, David S. Miller, Eric W. Biederman,
Eric Dumazet, Hannes Frederic Sowa
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-13 1:18 ` Cong Wang
@ 2014-02-13 2:00 ` Eric W. Biederman
2014-02-13 22:44 ` Cong Wang
0 siblings, 1 reply; 14+ messages in thread
From: Eric W. Biederman @ 2014-02-13 2:00 UTC (permalink / raw)
To: Cong Wang
Cc: Nicolas Dichtel, Cong Wang, netdev, David S. Miller, Eric Dumazet,
Hannes Frederic Sowa
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-13 2:00 ` Eric W. Biederman
@ 2014-02-13 22:44 ` Cong Wang
0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2014-02-13 22:44 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Nicolas Dichtel, Cong Wang, netdev, David S. Miller, Eric Dumazet,
Hannes Frederic Sowa
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-11 23:51 [PATCH] net: clear iflink when moving to a new netns Cong Wang
` (2 preceding siblings ...)
2014-02-12 15:43 ` [PATCH] net: clear iflink when moving to a new netns Nicolas Dichtel
@ 2014-02-12 16:33 ` Stephen Hemminger
2014-02-13 1:20 ` Cong Wang
2014-02-12 23:18 ` Ben Hutchings
4 siblings, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2014-02-12 16:33 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, David S. Miller, Eric W. Biederman, Eric Dumazet,
Hannes Frederic Sowa, Cong Wang
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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-12 16:33 ` Stephen Hemminger
@ 2014-02-13 1:20 ` Cong Wang
2014-02-13 2:01 ` Eric W. Biederman
0 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2014-02-13 1:20 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Cong Wang, netdev, David S. Miller, Eric W. Biederman,
Eric Dumazet, Hannes Frederic Sowa
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-13 1:20 ` Cong Wang
@ 2014-02-13 2:01 ` Eric W. Biederman
0 siblings, 0 replies; 14+ messages in thread
From: Eric W. Biederman @ 2014-02-13 2:01 UTC (permalink / raw)
To: Cong Wang
Cc: Stephen Hemminger, Cong Wang, netdev, David S. Miller,
Eric Dumazet, Hannes Frederic Sowa
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-11 23:51 [PATCH] net: clear iflink when moving to a new netns Cong Wang
` (3 preceding siblings ...)
2014-02-12 16:33 ` Stephen Hemminger
@ 2014-02-12 23:18 ` Ben Hutchings
2014-02-13 1:34 ` Cong Wang
4 siblings, 1 reply; 14+ messages in thread
From: Ben Hutchings @ 2014-02-12 23:18 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, David S. Miller, Eric W. Biederman, Eric Dumazet,
Hannes Frederic Sowa, Cong Wang
[-- 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 --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] net: clear iflink when moving to a new netns
2014-02-12 23:18 ` Ben Hutchings
@ 2014-02-13 1:34 ` Cong Wang
0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2014-02-13 1:34 UTC (permalink / raw)
To: Ben Hutchings
Cc: Cong Wang, netdev, David S. Miller, Eric W. Biederman,
Eric Dumazet, Hannes Frederic Sowa
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.
^ permalink raw reply [flat|nested] 14+ messages in thread