[Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

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

There is no point to allow moving a macvlan device to
another namespace while the lower device is still in
this namespace. tunnels already set this flag.

Cc: Patrick McHardy <kaber@trash.net>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <cwang@twopensource.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

---
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 8433de4..9bc3b13 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -539,7 +539,7 @@ static int macvlan_init(struct net_device *dev)
 	dev->state		= (dev->state & ~MACVLAN_STATE_MASK) |
 				  (lowerdev->state & MACVLAN_STATE_MASK);
 	dev->features 		= lowerdev->features & MACVLAN_FEATURES;
-	dev->features		|= NETIF_F_LLTX;
+	dev->features		|= NETIF_F_LLTX | NETIF_F_NETNS_LOCAL;
 	dev->gso_max_size	= lowerdev->gso_max_size;
 	dev->iflink		= lowerdev->ifindex;
 	dev->hard_header_len	= lowerdev->hard_header_len;
@@ -699,7 +699,7 @@ static netdev_features_t macvlan_fix_features(struct net_device *dev,
 	features = netdev_increment_features(vlan->lowerdev->features,
 					     features,
 					     mask);
-	features |= NETIF_F_LLTX;
+	features |= NETIF_F_LLTX | NETIF_F_NETNS_LOCAL;
 
 	return features;
 }

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Hannes Frederic Sowa]

On Mon, Feb 10, 2014 at 05:36:33PM -0800, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
> 
> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
> 
> There is no point to allow moving a macvlan device to
> another namespace while the lower device is still in
> this namespace. tunnels already set this flag.

Can't we solve this somehow differently, like not showing anything at all
etc.? I guess this is a feature some people use and haven't noticed yet.

Greetings,

  Hannes

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Cong Wang]

On Mon, Feb 10, 2014 at 5:45 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mon, Feb 10, 2014 at 05:36:33PM -0800, Cong Wang wrote:
>> From: Cong Wang <cwang@twopensource.com>
>>
>> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
>>
>> There is no point to allow moving a macvlan device to
>> another namespace while the lower device is still in
>> this namespace. tunnels already set this flag.
>
> Can't we solve this somehow differently, like not showing anything at all
> etc.? I guess this is a feature some people use and haven't noticed yet.
>

I don't understand what you mean by "not showing anything at all".
I assume you mean mac1@xxx, not matter whether we show xxx
here, mac1 relies on xxx to function.

Please give a real use case rather than just guessing, I don't think
there is any valid case until we support moving multiple devices into
a netns atomically.

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Hannes Frederic Sowa]

On Mon, Feb 10, 2014 at 06:25:51PM -0800, Cong Wang wrote:
> On Mon, Feb 10, 2014 at 5:45 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Mon, Feb 10, 2014 at 05:36:33PM -0800, Cong Wang wrote:
> >> From: Cong Wang <cwang@twopensource.com>
> >>
> >> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
> >>
> >> There is no point to allow moving a macvlan device to
> >> another namespace while the lower device is still in
> >> this namespace. tunnels already set this flag.
> >
> > Can't we solve this somehow differently, like not showing anything at all
> > etc.? I guess this is a feature some people use and haven't noticed yet.
> >
> 
> I don't understand what you mean by "not showing anything at all".
> I assume you mean mac1@xxx, not matter whether we show xxx
> here, mac1 relies on xxx to function.

Sorry, I have no idea how to resolve this easily, maybe set the ifindex to
something generic. I'll try to think about it.

Maybe revserve an id and install a generic name for it, so old software
doesn't get confused.

> Please give a real use case rather than just guessing, I don't think
> there is any valid case until we support moving multiple devices into
> a netns atomically.

Setting up a macvlan and moving it into another namespace without moving
the parent device is a nice feature. I am not an administrator, so I don't
use that stuff often, but given you can easily spawn namespaces and put
applications into them, one of the easiest things to connect those to
local network without routing over veth and such is the macvlan interface.

Greetings,

  Hannes

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Eric Dumazet]

On Mon, 2014-02-10 at 17:36 -0800, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
> 
> BZ: https://bugzilla.kernel.org/show_bug.cgi?id=66691
> 
> There is no point to allow moving a macvlan device to
> another namespace while the lower device is still in
> this namespace. tunnels already set this flag.

What do you mean ?

This is probably one of the needed/useful feature of macvlan !

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Eric Dumazet]

On Tue, 2014-02-11 at 03:40 +0100, Hannes Frederic Sowa wrote:

> Setting up a macvlan and moving it into another namespace without moving
> the parent device is a nice feature. I am not an administrator, so I don't
> use that stuff often, but given you can easily spawn namespaces and put
> applications into them, one of the easiest things to connect those to
> local network without routing over veth and such is the macvlan interface.

Exactly.

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Cong Wang]

On Mon, Feb 10, 2014 at 8:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2014-02-11 at 03:40 +0100, Hannes Frederic Sowa wrote:
>
>> Setting up a macvlan and moving it into another namespace without moving
>> the parent device is a nice feature. I am not an administrator, so I don't
>> use that stuff often, but given you can easily spawn namespaces and put
>> applications into them, one of the easiest things to connect those to
>> local network without routing over veth and such is the macvlan interface.
>
> Exactly.
>

Exactly broken by design.

IFA_LINK is an ifindex, ifindex is per-netns. macvlan should not use IFA_LINK.

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Eric Dumazet]

On Mon, 2014-02-10 at 20:41 -0800, Cong Wang wrote:

> Exactly broken by design.

Design of what ? Do you have a pointer to a document about this
'design' ?

> 
> IFA_LINK is an ifindex, ifindex is per-netns. macvlan should not use IFA_LINK.

When this was 'designed', ifindex were not per netns.

Apparently nobody spotted this when ifindexes become per netns.

I am sure we can find a solution, keeping this very useful
functionality.

BTW, are Cong Wang <cwang@twopensource.com> and Cong Wang
<xiyou.wangcong@gmail.com> different persons ?

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Eric Dumazet]

On Mon, 2014-02-10 at 21:21 -0800, Eric Dumazet wrote:
> On Mon, 2014-02-10 at 20:41 -0800, Cong Wang wrote:
> 
> > Exactly broken by design.
> 
> Design of what ? Do you have a pointer to a document about this
> 'design' ?
> 
> > 
> > IFA_LINK is an ifindex, ifindex is per-netns. macvlan should not use IFA_LINK.
> 
> When this was 'designed', ifindex were not per netns.
> 
> Apparently nobody spotted this when ifindexes become per netns.
> 
> I am sure we can find a solution, keeping this very useful
> functionality.

Simple patch would be :

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 048dc8d183aa..31bbba34fd1e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -963,6 +963,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
 #endif
 	    (dev->ifindex != dev->iflink &&
+	     __dev_get_by_index(dev_net(dev), dev->iflink) &&
 	     nla_put_u32(skb, IFLA_LINK, dev->iflink)) ||
 	    (upper_dev &&
 	     nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex)) ||

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Eric Dumazet]

On Mon, 2014-02-10 at 21:37 -0800, Eric Dumazet wrote:

> 
> Simple patch would be :
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 048dc8d183aa..31bbba34fd1e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -963,6 +963,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
>  #endif
>  	    (dev->ifindex != dev->iflink &&
> +	     __dev_get_by_index(dev_net(dev), dev->iflink) &&
>  	     nla_put_u32(skb, IFLA_LINK, dev->iflink)) ||
>  	    (upper_dev &&
>  	     nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex)) ||
> 

Hmm, not enough.

We probably need to keep a pointer to iflink net structure.

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Nicolas Dichtel]

Le 11/02/2014 05:15, Eric Dumazet a écrit :
> On Tue, 2014-02-11 at 03:40 +0100, Hannes Frederic Sowa wrote:
>
>> Setting up a macvlan and moving it into another namespace without moving
>> the parent device is a nice feature. I am not an administrator, so I don't
>> use that stuff often, but given you can easily spawn namespaces and put
>> applications into them, one of the easiest things to connect those to
>> local network without routing over veth and such is the macvlan interface.
>
> Exactly.

I also agree.

Re: [Patch net] macvlan: add NETIF_F_NETNS_LOCAL flag

[Nicolas Dichtel]

Le 11/02/2014 06:38, Eric Dumazet a écrit :
> On Mon, 2014-02-10 at 21:37 -0800, Eric Dumazet wrote:
>
>>
>> Simple patch would be :
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 048dc8d183aa..31bbba34fd1e 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -963,6 +963,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>>   	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES, dev->num_rx_queues) ||
>>   #endif
>>   	    (dev->ifindex != dev->iflink &&
>> +	     __dev_get_by_index(dev_net(dev), dev->iflink) &&
>>   	     nla_put_u32(skb, IFLA_LINK, dev->iflink)) ||
>>   	    (upper_dev &&
>>   	     nla_put_u32(skb, IFLA_MASTER, upper_dev->ifindex)) ||
>>
>
> Hmm, not enough.
>
> We probably need to keep a pointer to iflink net structure.
This is also used in ip tunnels, but when i/o device is not in the same netns
that the tunnel device, the userland can not interpret that IFLA_LINK attribute
(userland don't have necessary information to access another netns, maybe they
will be able to do it in the future ;-)).

The goal of your patch was to avoid filling this attribute when iflink is
into a netns != from dev_net(dev)?

If yes, I agree that we need to keep a pointer to the net structure of iflink.
This information is already available in ip tunnels (struct ip_tunnel->net,
struct ip6_tnl->net) but is not generic. Maybe we can move it to struct
net_device?