diff mbox

[net,0/2] netns: audit netdevice creation with IFLA_NET_NS_[PID|FD]

Message ID 54C7694C.2060709@6wind.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Nicolas Dichtel Jan. 27, 2015, 10:32 a.m. UTC 
Le 27/01/2015 10:34, Alexander Aring a écrit :
> Hi,
>
> On Mon, Jan 26, 2015 at 10:28:12PM +0100, Nicolas Dichtel wrote:
>>
[snip]
>> - ieee802154 uses also src_net and does not have NETIF_F_NETNS_LOCAL. Same
>>    question: does this netdevice really supports x-netns?
>
> I am not sure if I understand exactly what you mean. First of all, I
> didn't test anything about net namespaces for the ieee802154 branch.
> In 802.15.4 branch we have two interfaces: wpan and 6LoWPAN.
>
> After running "grep -r "src_net" net" I found this is used in:
>
> net/ieee802154/6lowpan/core.c [0]
Yes, I was talking about this.

>
> This file handles the IEEE 802.15.4 6LoWPAN interface to offering a
> IPv6 interface with an IEEE 802.15.4 6LoWPAN adaption layer.
>
> To the codeline "dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));".
> By calling "ip link add link wpan0 name lowpan0 type lowpan" the
> lowpan_newlink function will be called and we need to find the wpan interface
> (returned as real_dev in this case).
>
> Namespace setting in wpan interface:
>
> Currently we don't use any net namespace settings there, also we don't
> change the net namespace. The default net namespace for a wpan shoule be
> "init_net".
Ok. After grepping for init_net, it seems to be used a lot in net/ieee802154/.

>
> So this line could be also written as (I found also some others code which search
> the wpan interface in &init_net):
>
> diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
> index 9dbe0d69..495c6ad 100644
> --- a/net/ieee802154/6lowpan/core.c
> +++ b/net/ieee802154/6lowpan/core.c
> @@ -151,7 +151,7 @@ static int lowpan_newlink(struct net *src_net, struct net_device *dev,
>          if (!tb[IFLA_LINK])
>                  return -EINVAL;
>          /* find and hold real wpan device */
> -       real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
> +       real_dev = dev_get_by_index(&init_net, nla_get_u32(tb[IFLA_LINK]));
>          if (!real_dev)
>                  return -ENODEV;
>          if (real_dev->type != ARPHRD_IEEE802154) {
>
>
>
> The above code is for finding the wpan interface (the real 802.15.4 L2 interface).
> For the IEEE 802.15.4 6LoWPAN interface the whole IPv6 implementation is
> used. This interface will be created inside function "newlink".
>
> Running "grep -r "src_net" net/ipv6" reports me alot uses of "src_net".
> Don't know if this information is really necessary.
>
> Should I set now the NETIF_F_NETNS_LOCAL for both interface types?
I think yes. If it's not set, a user may do:
$ ip link add link wpan0 name lowpan0 type lowpan
$ ip netns add foo
$ ip link set lowpan0 netns foo

The flag forbids the last command.

Instead of your patch, what about this one:

 From d9a9cd22d5e1db1417b3ffb53cc020481dc761b2 Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 27 Jan 2015 11:26:20 +0100
Subject: [PATCH] ieee802154: forbid to create an iface in a netns != init_net

6LoWPAN currently doesn't supports netns.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
  net/ieee802154/6lowpan/core.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

  	real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));

Comments

Alexander Aring Jan. 27, 2015, 12:23 p.m. UTC | #1 
Hi,

(removing the bounced mail address).

On Tue, Jan 27, 2015 at 11:32:44AM +0100, Nicolas Dichtel wrote:
> Le 27/01/2015 10:34, Alexander Aring a écrit :
> >Hi,
> >
> >On Mon, Jan 26, 2015 at 10:28:12PM +0100, Nicolas Dichtel wrote:
> >>
> [snip]
> >>- ieee802154 uses also src_net and does not have NETIF_F_NETNS_LOCAL. Same
> >>   question: does this netdevice really supports x-netns?
> >
> >I am not sure if I understand exactly what you mean. First of all, I
> >didn't test anything about net namespaces for the ieee802154 branch.
> >In 802.15.4 branch we have two interfaces: wpan and 6LoWPAN.
> >
> >After running "grep -r "src_net" net" I found this is used in:
> >
> >net/ieee802154/6lowpan/core.c [0]
> Yes, I was talking about this.
> 

ok.

> >
> >This file handles the IEEE 802.15.4 6LoWPAN interface to offering a
> >IPv6 interface with an IEEE 802.15.4 6LoWPAN adaption layer.
> >
> >To the codeline "dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));".
> >By calling "ip link add link wpan0 name lowpan0 type lowpan" the
> >lowpan_newlink function will be called and we need to find the wpan interface
> >(returned as real_dev in this case).
> >
> >Namespace setting in wpan interface:
> >
> >Currently we don't use any net namespace settings there, also we don't
> >change the net namespace. The default net namespace for a wpan shoule be
> >"init_net".
> Ok. After grepping for init_net, it seems to be used a lot in net/ieee802154/.
> 

Yes, but the code in net/ieee802154 (except net/ieee802154/6lowpan) is only for
the WPAN interface. Currently the WPAN interface is created in mac802154
implementation only [0].

> >
> >So this line could be also written as (I found also some others code which search
> >the wpan interface in &init_net):
> >
> >diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
> >index 9dbe0d69..495c6ad 100644
> >--- a/net/ieee802154/6lowpan/core.c
> >+++ b/net/ieee802154/6lowpan/core.c
> >@@ -151,7 +151,7 @@ static int lowpan_newlink(struct net *src_net, struct net_device *dev,
> >         if (!tb[IFLA_LINK])
> >                 return -EINVAL;
> >         /* find and hold real wpan device */
> >-       real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
> >+       real_dev = dev_get_by_index(&init_net, nla_get_u32(tb[IFLA_LINK]));
> >         if (!real_dev)
> >                 return -ENODEV;
> >         if (real_dev->type != ARPHRD_IEEE802154) {
> >
> >
> >
> >The above code is for finding the wpan interface (the real 802.15.4 L2 interface).
> >For the IEEE 802.15.4 6LoWPAN interface the whole IPv6 implementation is
> >used. This interface will be created inside function "newlink".
> >
> >Running "grep -r "src_net" net/ipv6" reports me alot uses of "src_net".
> >Don't know if this information is really necessary.
> >
> >Should I set now the NETIF_F_NETNS_LOCAL for both interface types?
> I think yes. If it's not set, a user may do:
> $ ip link add link wpan0 name lowpan0 type lowpan
> $ ip netns add foo
> $ ip link set lowpan0 netns foo
> 

We should forbid that for the wpan interface. The code line:

real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));

searches for the "wpan0" interface which is given by:
$ ip link add link wpan0 name lowpan0 type lowpan

The returned real_dev netdevice pointer is the wpan interface. The given
"lowpan0" interface is a virtual interface for making IPv6 stuff.



For 6LoWPAN:

This interface is created in 6lowpan/core.c file and is used in the
whole IPv6 stack, because we set the skb->protocol to htons(ETH_P_IPV6). [1]

The IPv6 stack uses alot of "src_net".

> The flag forbids the last command.
> 
> Instead of your patch, what about this one:
> 
> From d9a9cd22d5e1db1417b3ffb53cc020481dc761b2 Mon Sep 17 00:00:00 2001
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Tue, 27 Jan 2015 11:26:20 +0100
> Subject: [PATCH] ieee802154: forbid to create an iface in a netns != init_net
> 
> 6LoWPAN currently doesn't supports netns.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/ieee802154/6lowpan/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
> index 055fbb71ba6f..fe8fd022042e 100644
> --- a/net/ieee802154/6lowpan/core.c
> +++ b/net/ieee802154/6lowpan/core.c
> @@ -126,6 +126,7 @@ static void lowpan_setup(struct net_device *dev)
>  	dev->header_ops		= &lowpan_header_ops;
>  	dev->ml_priv		= &lowpan_mlme;
>  	dev->destructor		= free_netdev;
> +	dev->features		|= NETIF_F_NETNS_LOCAL;
>  }
> 
>  static int lowpan_validate(struct nlattr *tb[], struct nlattr *data[])
> @@ -148,7 +149,9 @@ static int lowpan_newlink(struct net *src_net, struct
> net_device *dev,
> 
>  	pr_debug("adding new link\n");
> 
> -	if (!tb[IFLA_LINK])
> +	if (!tb[IFLA_LINK] ||
> +	    !net_eq(src_net, &init_net) ||
> +	    !net_eq(dev_net(dev), &init_net))
>  		return -EINVAL;
>  	/* find and hold real wpan device */
>  	real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));

With the check of "!net_eq(src_net, &init_net)" we need to be sure
that the wpan interface is always in "init_net". This means we need
definitely a dev->features |= NETIF_F_NETNS_LOCAL; somewhere in [0].

To adding "dev->features |= NETIF_F_NETNS_LOCAL;" for a 6LoWPAN interface,
I am not sure about this. I didn't test it yet and it will not break
anything, but we will lost the support for making net namespaces stuff
inside the IPv6/(netfilter) stack.


Summarize:

I would add the dev->features |= NETIF_F_NETNS_LOCAL; while wpan
interface generation and add only the !net_eq(src_net, &init_net) check
above. I suppose that src_net is the net namespace from "underlaying"
interface wpan by calling:

$ ip link add link wpan0 name lowpan0 type lowpan

- Alex

[0] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/net/mac802154/iface.c#n532
[1] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/net/ieee802154/6lowpan/rx.c#n25
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel Jan. 27, 2015, 1:28 p.m. UTC | #2 
Le 27/01/2015 13:23, Alexander Aring a écrit :
> Hi,
>
> (removing the bounced mail address).
>
> On Tue, Jan 27, 2015 at 11:32:44AM +0100, Nicolas Dichtel wrote:
>> Le 27/01/2015 10:34, Alexander Aring a écrit :
>>> Hi,
>>>
>>> On Mon, Jan 26, 2015 at 10:28:12PM +0100, Nicolas Dichtel wrote:
>>>>
[snip]
>>>
>>> This file handles the IEEE 802.15.4 6LoWPAN interface to offering a
>>> IPv6 interface with an IEEE 802.15.4 6LoWPAN adaption layer.
>>>
>>> To the codeline "dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));".
>>> By calling "ip link add link wpan0 name lowpan0 type lowpan" the
>>> lowpan_newlink function will be called and we need to find the wpan interface
>>> (returned as real_dev in this case).
>>>
>>> Namespace setting in wpan interface:
>>>
>>> Currently we don't use any net namespace settings there, also we don't
>>> change the net namespace. The default net namespace for a wpan shoule be
>>> "init_net".
>> Ok. After grepping for init_net, it seems to be used a lot in net/ieee802154/.
>>
>
> Yes, but the code in net/ieee802154 (except net/ieee802154/6lowpan) is only for
> the WPAN interface. Currently the WPAN interface is created in mac802154
> implementation only [0].
Ok. How is this interface created?

>
>>>
>>> So this line could be also written as (I found also some others code which search
>>> the wpan interface in &init_net):
>>>
>>> diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
>>> index 9dbe0d69..495c6ad 100644
>>> --- a/net/ieee802154/6lowpan/core.c
>>> +++ b/net/ieee802154/6lowpan/core.c
>>> @@ -151,7 +151,7 @@ static int lowpan_newlink(struct net *src_net, struct net_device *dev,
>>>          if (!tb[IFLA_LINK])
>>>                  return -EINVAL;
>>>          /* find and hold real wpan device */
>>> -       real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
>>> +       real_dev = dev_get_by_index(&init_net, nla_get_u32(tb[IFLA_LINK]));
>>>          if (!real_dev)
>>>                  return -ENODEV;
>>>          if (real_dev->type != ARPHRD_IEEE802154) {
>>>
>>>
>>>
>>> The above code is for finding the wpan interface (the real 802.15.4 L2 interface).
>>> For the IEEE 802.15.4 6LoWPAN interface the whole IPv6 implementation is
>>> used. This interface will be created inside function "newlink".
>>>
>>> Running "grep -r "src_net" net/ipv6" reports me alot uses of "src_net".
>>> Don't know if this information is really necessary.
>>>
>>> Should I set now the NETIF_F_NETNS_LOCAL for both interface types?
>> I think yes. If it's not set, a user may do:
>> $ ip link add link wpan0 name lowpan0 type lowpan
>> $ ip netns add foo
>> $ ip link set lowpan0 netns foo
>>
>
> We should forbid that for the wpan interface. The code line:
>
> real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
>
> searches for the "wpan0" interface which is given by:
> $ ip link add link wpan0 name lowpan0 type lowpan
>
> The returned real_dev netdevice pointer is the wpan interface. The given
> "lowpan0" interface is a virtual interface for making IPv6 stuff.
Yes. But with the current code, it seems that you can also do (not tested):
$ ip link add link wpan0 netns foo name lowpan0 type lowpan
With this cmd, src_net points to the netns where the ip cmd is launched (let's
say init_net in this case) but the interface lowpan0 will be created in the
netns foo. To summarize: wpan0 is in init_net and lowpan0 in foo.

Now, if you use this line:
real_dev = dev_get_by_index(dev_net(dev), nla_get_u32(tb[IFLA_LINK]));
With the same command, dev_net(dev) will point to foo.

>
>
>
> For 6LoWPAN:
>
> This interface is created in 6lowpan/core.c file and is used in the
> whole IPv6 stack, because we set the skb->protocol to htons(ETH_P_IPV6). [1]
>
> The IPv6 stack uses alot of "src_net".
In fact, src_net is used by IP tunnels (sit, ip6gre, ip6_tunnels, ip6_vti). But
these interfaces ignore this parameter.

The IPv6 stack is fully netns aware, thus it will use correctly the netns from
the netdevice and/or from the socket.

>
>> The flag forbids the last command.
>>
>> Instead of your patch, what about this one:
>>
>>  From d9a9cd22d5e1db1417b3ffb53cc020481dc761b2 Mon Sep 17 00:00:00 2001
>> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> Date: Tue, 27 Jan 2015 11:26:20 +0100
>> Subject: [PATCH] ieee802154: forbid to create an iface in a netns != init_net
>>
>> 6LoWPAN currently doesn't supports netns.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   net/ieee802154/6lowpan/core.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
>> index 055fbb71ba6f..fe8fd022042e 100644
>> --- a/net/ieee802154/6lowpan/core.c
>> +++ b/net/ieee802154/6lowpan/core.c
>> @@ -126,6 +126,7 @@ static void lowpan_setup(struct net_device *dev)
>>   	dev->header_ops		= &lowpan_header_ops;
>>   	dev->ml_priv		= &lowpan_mlme;
>>   	dev->destructor		= free_netdev;
>> +	dev->features		|= NETIF_F_NETNS_LOCAL;
>>   }
>>
>>   static int lowpan_validate(struct nlattr *tb[], struct nlattr *data[])
>> @@ -148,7 +149,9 @@ static int lowpan_newlink(struct net *src_net, struct
>> net_device *dev,
>>
>>   	pr_debug("adding new link\n");
>>
>> -	if (!tb[IFLA_LINK])
>> +	if (!tb[IFLA_LINK] ||
>> +	    !net_eq(src_net, &init_net) ||
>> +	    !net_eq(dev_net(dev), &init_net))
>>   		return -EINVAL;
>>   	/* find and hold real wpan device */
>>   	real_dev = dev_get_by_index(src_net, nla_get_u32(tb[IFLA_LINK]));
>
> With the check of "!net_eq(src_net, &init_net)" we need to be sure
> that the wpan interface is always in "init_net". This means we need
> definitely a dev->features |= NETIF_F_NETNS_LOCAL; somewhere in [0].
>
> To adding "dev->features |= NETIF_F_NETNS_LOCAL;" for a 6LoWPAN interface,
> I am not sure about this. I didn't test it yet and it will not break
> anything, but we will lost the support for making net namespaces stuff
> inside the IPv6/(netfilter) stack.
Adding NETIF_F_NETNS_LOCAL does not mean that the netdevice can be used only
in init_net, this flag means that the netdevice cannot be moved to another
netns. You can still create a netdevice in another netns (if wpan0 is in netns
foo):

$ ip netns exec foo ip link add link wpan0 name lowpan0 type lowpan

I don't know how wpan0 is created and if this interface can be created directly
in another netns than init_net.

>
>
> Summarize:
>
> I would add the dev->features |= NETIF_F_NETNS_LOCAL; while wpan
> interface generation and add only the !net_eq(src_net, &init_net) check
> above. I suppose that src_net is the net namespace from "underlaying"
> interface wpan by calling:
>
> $ ip link add link wpan0 name lowpan0 type lowpan
No. src_net is the netns where the ip command is launched. With this patch, my
previous cmd (ip link add link wpan0 netns foo name lowpan0 type lowpan) will
still work and lowpan0 will be in netns foo.
Note: I just saw your patch proposal in another reply [0], with it, my sentence
is still true I think ;-)

But:
$ git grep "net_eq.*init_net" net/ieee802154/' | wc -l
7
So I wonder if it's really possible to use lowpan0 in another netns than
init_net (my proposal was based on this).

Maybe this can help you to understand my point.
A virtual interface can be across two netns:
  - the link netns (the interface receives and sends packets to/from the link
    layer into this netns)
  - the netns where the interface is visible (the user receives and sends
    packets to/from the interface into this netns)

[0] http://patchwork.ozlabs.org/patch/433467/

Regards,
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Aring Jan. 27, 2015, 2:06 p.m. UTC | #3 
Hi,

On Tue, Jan 27, 2015 at 02:28:47PM +0100, Nicolas Dichtel wrote:
...
> >With the check of "!net_eq(src_net, &init_net)" we need to be sure
> >that the wpan interface is always in "init_net". This means we need
> >definitely a dev->features |= NETIF_F_NETNS_LOCAL; somewhere in [0].
> >
> >To adding "dev->features |= NETIF_F_NETNS_LOCAL;" for a 6LoWPAN interface,
> >I am not sure about this. I didn't test it yet and it will not break
> >anything, but we will lost the support for making net namespaces stuff
> >inside the IPv6/(netfilter) stack.
> Adding NETIF_F_NETNS_LOCAL does not mean that the netdevice can be used only
> in init_net, this flag means that the netdevice cannot be moved to another
> netns. You can still create a netdevice in another netns (if wpan0 is in netns
> foo):
> 
> $ ip netns exec foo ip link add link wpan0 name lowpan0 type lowpan
> 
> I don't know how wpan0 is created and if this interface can be created directly
> in another netns than init_net.
> 

no it can't. The wpan0 interface can be created via the 802.15.4
userspace tools and we don't have such option for namespaces. It
should be always to init_net while creation.

> >
> >
> >Summarize:
> >
> >I would add the dev->features |= NETIF_F_NETNS_LOCAL; while wpan
> >interface generation and add only the !net_eq(src_net, &init_net) check
> >above. I suppose that src_net is the net namespace from "underlaying"
> >interface wpan by calling:
> >
> >$ ip link add link wpan0 name lowpan0 type lowpan
> No. src_net is the netns where the ip command is launched. With this patch, my

ah, and when no "ip netns" is given it's default to init_net?


Okay, then I agree with that both interfaces should be set

dev->features |= NETIF_F_NETNS_LOCAL

because both interfaces should started with "init_net" as default
namespace. For wpan interface this should always be in "init_net",
because we don't set anything while creation.

For 6LoWPAN interface this should also always in the same namespace like
the wpan interface and not diffrent namespace between link (wpan) and
virtual (6LoWPAN) interface.

Do you agree with that?

- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ieee802154/6lowpan/core.c b/net/ieee802154/6lowpan/core.c
index 055fbb71ba6f..fe8fd022042e 100644
--- a/net/ieee802154/6lowpan/core.c
+++ b/net/ieee802154/6lowpan/core.c
@@ -126,6 +126,7 @@  static void lowpan_setup(struct net_device *dev)
  	dev->header_ops		= &lowpan_header_ops;
  	dev->ml_priv		= &lowpan_mlme;
  	dev->destructor		= free_netdev;
+	dev->features		|= NETIF_F_NETNS_LOCAL;
  }

  static int lowpan_validate(struct nlattr *tb[], struct nlattr *data[])
@@ -148,7 +149,9 @@  static int lowpan_newlink(struct net *src_net, struct 
net_device *dev,

  	pr_debug("adding new link\n");

-	if (!tb[IFLA_LINK])
+	if (!tb[IFLA_LINK] ||
+	    !net_eq(src_net, &init_net) ||
+	    !net_eq(dev_net(dev), &init_net))
  		return -EINVAL;
  	/* find and hold real wpan device */