new link failing on duplicate names in different namespaces

Hello,

rtnetlink's RTM_NEWLINK allows for specifying the network namespace in
which the link is to be created via IFLA_NET_NS_PID or IFLA_NET_NS_FD.
This not only saves a user a call to move the link into the target
namespace, it can avoid a potential ifname conflict. For example, if
creating eth0 in another namespace and the current one already has
eth0.

Unfortunately, this is not the current behavior. If the user specifies
IFLA_IFNAME, leaves ifinfomsg.ifi_index unspecified and sets
NLM_F_EXCL flag, as in the case of creating a new link, the call will
fail with EEXIST in cases where there's a name conflict.

rtnl_newlink() will:

if (ifname[0])
    dev = __dev_get_by_name(net, ifname);

and later:

if (dev) {
    int status = 0;

    if (nlh->nlmsg_flags & NLM_F_EXCL)
        return -EEXIST;
...
}

I am not sure if this has been discussed before or if it's viewed as a
real problem. Fixing this however, would make it easier to work with
interfaces in different network namespaces. I don't have a good
solution in mind though and so I can't propose a patch at this time.

Thanks,
Eugene

Le 25/02/2015 01:48, Eugene Yakubovich a écrit :

Hello,

rtnetlink's RTM_NEWLINK allows for specifying the network namespace in
which the link is to be created via IFLA_NET_NS_PID or IFLA_NET_NS_FD.
This not only saves a user a call to move the link into the target
namespace, it can avoid a potential ifname conflict. For example, if
creating eth0 in another namespace and the current one already has
eth0.

Unfortunately, this is not the current behavior. If the user specifies
IFLA_IFNAME, leaves ifinfomsg.ifi_index unspecified and sets
NLM_F_EXCL flag, as in the case of creating a new link, the call will
fail with EEXIST in cases where there's a name conflict.

rtnl_newlink() will:

if (ifname[0])
     dev = __dev_get_by_name(net, ifname);

Yes, it seems that this should be done in "dest_net" or "link_net".

Nicolas Dichtel [off-list ref] writes:

Le 25/02/2015 01:48, Eugene Yakubovich a écrit :

quoted

Hello,

rtnetlink's RTM_NEWLINK allows for specifying the network namespace in
which the link is to be created via IFLA_NET_NS_PID or IFLA_NET_NS_FD.
This not only saves a user a call to move the link into the target
namespace, it can avoid a potential ifname conflict. For example, if
creating eth0 in another namespace and the current one already has
eth0.

Unfortunately, this is not the current behavior. If the user specifies
IFLA_IFNAME, leaves ifinfomsg.ifi_index unspecified and sets
NLM_F_EXCL flag, as in the case of creating a new link, the call will
fail with EEXIST in cases where there's a name conflict.

rtnl_newlink() will:

if (ifname[0])
     dev = __dev_get_by_name(net, ifname);

Yes, it seems that this should be done in "dest_net" or "link_net".

Ugh.  Looking at that code I think the link_net is at best extremely
confusing most likely semantically broken the way it is being
interpreted in rtnl_newlink.  More in another email message.

The original semantics and what seems mostly reasonable because the
network device in some sense lives in multiple network namespaces
as it is being created is that the network device name be unique
in both namespaces.

The code that creates a network devices fundamentally needs to know
about both namespaces network devices like the macvlan driver need
to find their lower devices in the original network namespace.

I think we could possibly remove the restriction for the names of newly
created network devices being unique in both network namespaces but I
don't think the code changes will be trivial.

Eric

Reading through the code of rtnl_newlink I am perplexed with what your
intended semantics of IFLA_LINK_NETNSID are supposed to be.

My expectation was something with the same semantics IFLA_NET_NS_PID and
IFLA_NET_NS_FD just a different data representation and something that
could be used in more netlink messages, so you could report the network
namespace where the other end of a tunnel or the the network namespace
of an underlying device is.

Being very slow I would expect that ILFA_LINK_NETNSID would replace
dest_net or possibly net in rtnl_newlink but it does not replace either
of those completely.  Which causes me to think that the implementation
of IFLA_LINK_NETNSID in rtnl_newlink is broken.

I suspect the correct fix for rtnl_newlink is to just use
IFLA_LINK_NETNSID in rtnl_link_get_net and have it be an alternative way
of setting dest_net.  But you may intend some different semantics that
I don't understand.

Eric

On Wed, Feb 25, 2015 at 11:03 AM, Eric W. Biederman
[off-list ref] wrote:

I suspect the correct fix for rtnl_newlink is to just use
IFLA_LINK_NETNSID in rtnl_link_get_net and have it be an alternative way
of setting dest_net.  But you may intend some different semantics that
I don't understand.

Looking at the example provided in iproute2, link-netnsid
should refer the the netns, by the given ID, where the underlying
link is. The difference is that this ID is only unique in a given netns,
and refers to another peer netns.

On Wed, Feb 25, 2015 at 9:44 AM, Eric W. Biederman
[off-list ref] wrote:

The code that creates a network devices fundamentally needs to know
about both namespaces network devices like the macvlan driver need
to find their lower devices in the original network namespace.

I think we could possibly remove the restriction for the names of newly
created network devices being unique in both network namespaces but I
don't think the code changes will be trivial.

We should add a flag to network devices, so that some devices which
don't have an underlying device, like veth, are allowed to be create right
in the given netns. Or, of course, just modify rtnl_newlink() if we have
some other clue (tb[IFLA_LINK] == NULL?).

I am working on a patch.

On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang [off-list ref] wrote:

On Wed, Feb 25, 2015 at 9:44 AM, Eric W. Biederman
[off-list ref] wrote:

quoted

The code that creates a network devices fundamentally needs to know
about both namespaces network devices like the macvlan driver need
to find their lower devices in the original network namespace.

I think we could possibly remove the restriction for the names of newly
created network devices being unique in both network namespaces but I
don't think the code changes will be trivial.

We should add a flag to network devices, so that some devices which
don't have an underlying device, like veth, are allowed to be create right
in the given netns. Or, of course, just modify rtnl_newlink() if we have
some other clue (tb[IFLA_LINK] == NULL?).

Never mind, seems we only need to move the dest_net and link_net
up before checking duplicated.

Please give the attached patch a try.

Le 25/02/2015 20:03, Eric W. Biederman a écrit :

Reading through the code of rtnl_newlink I am perplexed with what your
intended semantics of IFLA_LINK_NETNSID are supposed to be.

My expectation was something with the same semantics IFLA_NET_NS_PID and
IFLA_NET_NS_FD just a different data representation and something that
could be used in more netlink messages, so you could report the network
namespace where the other end of a tunnel or the the network namespace
of an underlying device is.

Being very slow I would expect that ILFA_LINK_NETNSID would replace
dest_net or possibly net in rtnl_newlink but it does not replace either
of those completely.  Which causes me to think that the implementation
of IFLA_LINK_NETNSID in rtnl_newlink is broken.

I suspect the correct fix for rtnl_newlink is to just use
IFLA_LINK_NETNSID in rtnl_link_get_net and have it be an alternative way
of setting dest_net.  But you may intend some different semantics that
I don't understand.

ILFA_LINK_NETNSID is used to point to the i/o netns of the interface, ie the
opposite netns of dest_net.
The interface is first created in link_net and moved at the end in dest_net.

IP tunnels interfaces (ipip, sit, ip6_tunnels, gre[v6]) does not use src_net,
thus when you create an ipip interface by specifying an attribute
IFLA_NET_NS_PID it will result to an interface which is not across two netns but
only in the netns pointed by IFLA_NET_NS_PID. If you use IFLA_LINK_NETNSID, it
allows you to create this kind of x-netns interfaces.
In fact, the goal of this attribute is to replace the two following command by
only one:
ip link add foo ...
ip link set foo netns bar
=> ip link add foo link-netnsid barID


Regards,
Nicolas

Le 26/02/2015 06:56, Cong Wang a écrit :

On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang [off-list ref] wrote:

[snip]

Please give the attached patch a try.

It's hard to comment a patch which is sent in attachment
(see http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n333).

+				err = rtnl_group_changelink(skb, dest_net,
It should probably be something like 'link_net ? : dest_net' instead of
dest_net.

I also don't understand why you need two label ('out' and 'err'). I think one is enough (link_net is initialized to NULL for this purpose).
And why not keeping the same name as before, ie 'out'? It will minimize the
patch.

Nicolas Dichtel [off-list ref] writes:

Le 25/02/2015 20:03, Eric W. Biederman a écrit :

quoted

Reading through the code of rtnl_newlink I am perplexed with what your
intended semantics of IFLA_LINK_NETNSID are supposed to be.

My expectation was something with the same semantics IFLA_NET_NS_PID and
IFLA_NET_NS_FD just a different data representation and something that
could be used in more netlink messages, so you could report the network
namespace where the other end of a tunnel or the the network namespace
of an underlying device is.

Being very slow I would expect that ILFA_LINK_NETNSID would replace
dest_net or possibly net in rtnl_newlink but it does not replace either
of those completely.  Which causes me to think that the implementation
of IFLA_LINK_NETNSID in rtnl_newlink is broken.

I suspect the correct fix for rtnl_newlink is to just use
IFLA_LINK_NETNSID in rtnl_link_get_net and have it be an alternative way
of setting dest_net.  But you may intend some different semantics that
I don't understand.

ILFA_LINK_NETNSID is used to point to the i/o netns of the interface, ie the
opposite netns of dest_net.

Fair enough.  That makes sense given how it is used in the reporting.
With those semantics the code may arguably be correct.

The interface is first created in link_net and moved at the end in dest_net.

IP tunnels interfaces (ipip, sit, ip6_tunnels, gre[v6]) does not use src_net,

I am pretty certain that is simply something that was overlooked when
cross network namespace support was added to those network device types.
Right now I would be surprised if anything in userspace cares, so we can
probably just change those device types to look at src_net from newlink.

Certainly for our sanity in maintaining rtnl_newlink finding a way to
make that change would be preferable.  Even if we ultimately have to add
a flag that says only make src_net different from dev->net when
IFLA_LINK_NETNSID is passed.

As making that change allows much more consistency in the code and
allows us to get rid of an unnecessary dev_change_net_namespace.

thus when you create an ipip interface by specifying an attribute
IFLA_NET_NS_PID it will result to an interface which is not across two netns but
only in the netns pointed by IFLA_NET_NS_PID. If you use IFLA_LINK_NETNSID, it
allows you to create this kind of x-netns interfaces.
In fact, the goal of this attribute is to replace the two following command by
only one:
ip link add foo ...
ip link set foo netns bar
=> ip link add foo link-netnsid barID

Eric

Nicolas Dichtel [off-list ref] writes:

Le 26/02/2015 06:56, Cong Wang a écrit :

quoted

On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang [off-list ref] wrote:

[snip]

quoted

Please give the attached patch a try.

It's hard to comment a patch which is sent in attachment
(see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n333).

+				err = rtnl_group_changelink(skb, dest_net,
It should probably be something like 'link_net ? : dest_net' instead of
dest_net.

I also don't understand why you need two label ('out' and 'err'). I think one is
enough (link_net is initialized to NULL for this purpose).
And why not keeping the same name as before, ie 'out'? It will minimize the
patch.

Sigh.

We can not give the guarantee that a new network device will only live
in and have a unique name in a single network namespace until we get
rid of dev_change_net_namespace in the code.

Cong your patch does not get rid of that and so is insufficient to solve
this problem and buggy.

Eric

Le 26/02/2015 14:55, Eric W. Biederman a écrit :
[snip]

We can not give the guarantee that a new network device will only live
in and have a unique name in a single network namespace until we get
rid of dev_change_net_namespace in the code.

In fact in case of IFLA_LINK_NETNSID, with Cong's patch, we will first check
that the name is unique in link_net and after dev_change_net_namespace() will
check that the name is also unique in the final net, else the command will fail
(and the device is not created).

Le 26/02/2015 14:48, Eric W. Biederman a écrit :

Nicolas Dichtel [off-list ref] writes:

[snip]

quoted

The interface is first created in link_net and moved at the end in dest_net.

IP tunnels interfaces (ipip, sit, ip6_tunnels, gre[v6]) does not use src_net,

I am pretty certain that is simply something that was overlooked when
cross network namespace support was added to those network device types.
Right now I would be surprised if anything in userspace cares, so we can
probably just change those device types to look at src_net from newlink.

At least, depending of the interface type, the behavior is not the same. Thus,
making all interfaces consistent seems good.

Certainly for our sanity in maintaining rtnl_newlink finding a way to
make that change would be preferable.  Even if we ultimately have to add
a flag that says only make src_net different from dev->net when
IFLA_LINK_NETNSID is passed.

A flag is probably not needed, it's just a matter of passing src_net through
the functions that create the tunnel device.

As making that change allows much more consistency in the code and
allows us to get rid of an unnecessary dev_change_net_namespace.

Yes, I add this dev_change_net_namespace() to be independant of the interface
implementation (and thus beeing consistent whatever the interface type is used).


Nicolas

When applicable verify that the caller has permision to create a
network device in another network namespace.  This check is already
present when moving a network device between network namespaces in
setlink so all that is needed is to duplicate that check in newlink.

This change almost backports cleanly, but there are context conflicts
as the code that follows was added in v4.0-rc1

Fixes: b51642f6d77b131dc85d1d71029c3cbb5b07c262 net: Enable a userns root rtnl calls that are safe for unprivilged users
Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <redacted>
---
 net/core/rtnetlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ab293a3066b3..155e675f656c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -2122,6 +2122,10 @@ replay:
 		if (IS_ERR(dest_net))
 			return PTR_ERR(dest_net);
 
+		err = -EPERM;
+		if (!netlink_ns_capable(skb, dest_net->user_ns, CAP_NET_ADMIN))
+			goto out;
+
 		if (tb[IFLA_LINK_NETNSID]) {
 			int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
 

-- 
2.2.1

When applicable verify that the caller has permisson to the underlying
network namespace for a newly created network device.

Similary checks exist for the network namespace a network device will
be created in.

Fixes: v4.0-rc1
Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <redacted>
---
 net/core/rtnetlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 155e675f656c..0a0b1c081d68 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -2134,6 +2134,9 @@ replay:
 				err =  -EINVAL;
 				goto out;
 			}
+			err = -EPERM;
+			if (!netlink_ns_capable(skb, link_net->user_ns, CAP_NET_ADMIN))
+				goto out;
 		}
 
 		dev = rtnl_create_link(link_net ? : dest_net, ifname,

-- 
2.2.1

On Thu, Feb 26, 2015 at 1:14 AM, Nicolas Dichtel
[off-list ref] wrote:

Le 26/02/2015 06:56, Cong Wang a écrit :

quoted

On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang [off-list ref] wrote:

[snip]

quoted

Please give the attached patch a try.

It's hard to comment a patch which is sent in attachment
(see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n333).

I knew, it is just not yet ready to submit formally. :)

+                               err = rtnl_group_changelink(skb, dest_net,
It should probably be something like 'link_net ? : dest_net' instead of
dest_net.

I think it should be "link_net ?: net", since it is the source net.
And yes we need to pass the dest_net to this function as well as
do_setlink() too.

I also don't understand why you need two label ('out' and 'err'). I think
one is enough (link_net is initialized to NULL for this purpose).
And why not keeping the same name as before, ie 'out'? It will minimize the
patch.

Ah, right, I missed it.

Thanks!

Le 26/02/2015 23:19, Eric W. Biederman a écrit :

When applicable verify that the caller has permision to create a
network device in another network namespace.  This check is already
present when moving a network device between network namespaces in
setlink so all that is needed is to duplicate that check in newlink.

This change almost backports cleanly, but there are context conflicts
as the code that follows was added in v4.0-rc1

Fixes: b51642f6d77b131dc85d1d71029c3cbb5b07c262 net: Enable a userns root rtnl calls that are safe for unprivilged users

12 digits is enough for the sha1 and commit title should be formatted like this
("commit title")
See http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n187

Fixes: b51642f6d77b ("net: Enable a userns root rtnl calls that are safe for unprivilged users")
Acked-by: Nicolas Dichtel <redacted>

Le 26/02/2015 23:20, Eric W. Biederman a écrit :

When applicable verify that the caller has permisson to the underlying
network namespace for a newly created network device.

Similary checks exist for the network namespace a network device will
be created in.

Fixes: v4.0-rc1

Fixes: 317f4810e45e ("rtnl: allow to create device with IFLA_LINK_NETNSID set")
Acked-by: Nicolas Dichtel <redacted>

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 26 Feb 2015 16:19:00 -0600

When applicable verify that the caller has permision to create a
network device in another network namespace.  This check is already
present when moving a network device between network namespaces in
setlink so all that is needed is to duplicate that check in newlink.

This change almost backports cleanly, but there are context conflicts
as the code that follows was added in v4.0-rc1

Fixes: b51642f6d77b131dc85d1d71029c3cbb5b07c262 net: Enable a userns root rtnl calls that are safe for unprivilged users
Signed-off-by: "Eric W. Biederman" <redacted>

Applied.

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 26 Feb 2015 16:20:07 -0600

When applicable verify that the caller has permisson to the underlying
network namespace for a newly created network device.

Similary checks exist for the network namespace a network device will
be created in.

Fixes: v4.0-rc1
Signed-off-by: "Eric W. Biederman" <redacted>

Applied.