new link failing on duplicate names in different namespaces

new link failing on duplicate names in different namespaces

[Eugene Yakubovich]

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

Re: new link failing on duplicate names in different namespaces

[Nicolas Dichtel]

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".

Re: new link failing on duplicate names in different namespaces

[Eric W. Biederman]

Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:

> 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".

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

What are the intended semantics of IFLA_LINK_NETNSID?

[Eric W. Biederman]

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

Re: What are the intended semantics of IFLA_LINK_NETNSID?

[Cong Wang]

On Wed, Feb 25, 2015 at 11:03 AM, Eric W. Biederman
<ebiederm@xmission.com> 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.

Re: new link failing on duplicate names in different namespaces

[Cong Wang]

On Wed, Feb 25, 2015 at 9:44 AM, Eric W. Biederman
<ebiederm@xmission.com> 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.

Re: new link failing on duplicate names in different namespaces

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang <cwang@twopensource.com> wrote:
> On Wed, Feb 25, 2015 at 9:44 AM, Eric W. Biederman
> <ebiederm@xmission.com> 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?).

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.

[-- Attachment #2: rtnetlink.patch --]
[-- Type: text/x-patch, Size: 6028 bytes --]

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1385de0..ff6e98c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1961,6 +1961,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct nlattr *tb[IFLA_MAX+1];
 	struct nlattr *linkinfo[IFLA_INFO_MAX+1];
 	unsigned char name_assign_type = NET_NAME_USER;
+	struct net *dest_net, *link_net = NULL;
 	int err;
 
 #ifdef CONFIG_MODULES
@@ -1976,11 +1977,26 @@ replay:
 		ifname[0] = '\0';
 
 	ifm = nlmsg_data(nlh);
+
+	dest_net = rtnl_link_get_net(net, tb);
+	if (IS_ERR(dest_net))
+		return PTR_ERR(dest_net);
+
+	if (tb[IFLA_LINK_NETNSID]) {
+		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
+
+		link_net = get_net_ns_by_id(dest_net, id);
+		if (!link_net) {
+			err =  -EINVAL;
+			goto out;
+		}
+	}
+
 	if (ifm->ifi_index > 0)
-		dev = __dev_get_by_index(net, ifm->ifi_index);
+		dev = __dev_get_by_index(link_net ?: dest_net, ifm->ifi_index);
 	else {
 		if (ifname[0])
-			dev = __dev_get_by_name(net, ifname);
+			dev = __dev_get_by_name(link_net ?: dest_net, ifname);
 		else
 			dev = NULL;
 	}
@@ -1993,13 +2009,13 @@ replay:
 
 	err = validate_linkmsg(dev, tb);
 	if (err < 0)
-		return err;
+		goto err;
 
 	if (tb[IFLA_LINKINFO]) {
 		err = nla_parse_nested(linkinfo, IFLA_INFO_MAX,
 				       tb[IFLA_LINKINFO], ifla_info_policy);
 		if (err < 0)
-			return err;
+			goto err;
 	} else
 		memset(linkinfo, 0, sizeof(linkinfo));
 
@@ -2016,7 +2032,6 @@ replay:
 		struct nlattr *slave_attr[m_ops ? m_ops->slave_maxtype + 1 : 1];
 		struct nlattr **data = NULL;
 		struct nlattr **slave_data = NULL;
-		struct net *dest_net, *link_net = NULL;
 
 		if (ops) {
 			if (ops->maxtype && linkinfo[IFLA_INFO_DATA]) {
@@ -2024,13 +2039,13 @@ replay:
 						       linkinfo[IFLA_INFO_DATA],
 						       ops->policy);
 				if (err < 0)
-					return err;
+					goto err;
 				data = attr;
 			}
 			if (ops->validate) {
 				err = ops->validate(tb, data);
 				if (err < 0)
-					return err;
+					goto err;
 			}
 		}
 
@@ -2042,59 +2057,71 @@ replay:
 						       linkinfo[IFLA_INFO_SLAVE_DATA],
 						       m_ops->slave_policy);
 				if (err < 0)
-					return err;
+					goto err;
 				slave_data = slave_attr;
 			}
 			if (m_ops->slave_validate) {
 				err = m_ops->slave_validate(tb, slave_data);
 				if (err < 0)
-					return err;
+					goto err;
 			}
 		}
 
 		if (dev) {
 			int status = 0;
 
-			if (nlh->nlmsg_flags & NLM_F_EXCL)
-				return -EEXIST;
-			if (nlh->nlmsg_flags & NLM_F_REPLACE)
-				return -EOPNOTSUPP;
+			if (nlh->nlmsg_flags & NLM_F_EXCL) {
+				err = -EEXIST;
+				goto err;
+			}
+			if (nlh->nlmsg_flags & NLM_F_REPLACE) {
+				err = -EOPNOTSUPP;
+				goto err;
+			}
 
 			if (linkinfo[IFLA_INFO_DATA]) {
 				if (!ops || ops != dev->rtnl_link_ops ||
-				    !ops->changelink)
-					return -EOPNOTSUPP;
+				    !ops->changelink) {
+					err = -EOPNOTSUPP;
+					goto err;
+				}
 
 				err = ops->changelink(dev, tb, data);
 				if (err < 0)
-					return err;
+					goto err;
 				status |= DO_SETLINK_NOTIFY;
 			}
 
 			if (linkinfo[IFLA_INFO_SLAVE_DATA]) {
-				if (!m_ops || !m_ops->slave_changelink)
-					return -EOPNOTSUPP;
+				if (!m_ops || !m_ops->slave_changelink) {
+					err = -EOPNOTSUPP;
+					goto err;
+				}
 
 				err = m_ops->slave_changelink(master_dev, dev,
 							      tb, slave_data);
 				if (err < 0)
-					return err;
+					goto err;
 				status |= DO_SETLINK_NOTIFY;
 			}
 
-			return do_setlink(skb, dev, ifm, tb, ifname, status);
+			err = do_setlink(skb, dev, ifm, tb, ifname, status);
+			goto err;
 		}
 
 		if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
+			err = -ENODEV;
 			if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
-				return rtnl_group_changelink(skb, net,
+				err = rtnl_group_changelink(skb, dest_net,
 						nla_get_u32(tb[IFLA_GROUP]),
 						ifm, tb);
-			return -ENODEV;
+			goto err;
 		}
 
-		if (tb[IFLA_MAP] || tb[IFLA_MASTER] || tb[IFLA_PROTINFO])
-			return -EOPNOTSUPP;
+		if (tb[IFLA_MAP] || tb[IFLA_MASTER] || tb[IFLA_PROTINFO]) {
+			err = -EOPNOTSUPP;
+			goto err;
+		}
 
 		if (!ops) {
 #ifdef CONFIG_MODULES
@@ -2103,40 +2130,33 @@ replay:
 				request_module("rtnl-link-%s", kind);
 				rtnl_lock();
 				ops = rtnl_link_ops_get(kind);
-				if (ops)
+				if (ops) {
+					if (link_net)
+						put_net(link_net);
+					put_net(dest_net);
 					goto replay;
+				}
 			}
 #endif
-			return -EOPNOTSUPP;
+			err = -EOPNOTSUPP;
+			goto err;
 		}
 
-		if (!ops->setup)
-			return -EOPNOTSUPP;
+		if (!ops->setup) {
+			err = -EOPNOTSUPP;
+			goto err;
+		}
 
 		if (!ifname[0]) {
 			snprintf(ifname, IFNAMSIZ, "%s%%d", ops->kind);
 			name_assign_type = NET_NAME_ENUM;
 		}
 
-		dest_net = rtnl_link_get_net(net, tb);
-		if (IS_ERR(dest_net))
-			return PTR_ERR(dest_net);
-
-		if (tb[IFLA_LINK_NETNSID]) {
-			int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
-
-			link_net = get_net_ns_by_id(dest_net, id);
-			if (!link_net) {
-				err =  -EINVAL;
-				goto out;
-			}
-		}
-
 		dev = rtnl_create_link(link_net ? : dest_net, ifname,
 				       name_assign_type, ops, tb);
 		if (IS_ERR(dev)) {
 			err = PTR_ERR(dev);
-			goto out;
+			goto err;
 		}
 
 		dev->ifindex = ifm->ifi_index;
@@ -2151,13 +2171,13 @@ replay:
 				/* If device is not registered at all, free it now */
 				if (dev->reg_state == NETREG_UNINITIALIZED)
 					free_netdev(dev);
-				goto out;
+				goto err;
 			}
 		} else {
 			err = register_netdevice(dev);
 			if (err < 0) {
 				free_netdev(dev);
-				goto out;
+				goto err;
 			}
 		}
 		err = rtnl_configure_link(dev, ifm);
@@ -2170,7 +2190,7 @@ replay:
 			} else {
 				unregister_netdevice(dev);
 			}
-			goto out;
+			goto err;
 		}
 
 		if (link_net) {
@@ -2178,12 +2198,14 @@ replay:
 			if (err < 0)
 				unregister_netdevice(dev);
 		}
-out:
-		if (link_net)
-			put_net(link_net);
-		put_net(dest_net);
-		return err;
 	}
+
+err:
+	if (link_net)
+		put_net(link_net);
+out:
+	put_net(dest_net);
+	return err;
 }
 
 static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh)

Re: What are the intended semantics of IFLA_LINK_NETNSID?

[Nicolas Dichtel]

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

Re: new link failing on duplicate names in different namespaces

[Nicolas Dichtel]

Le 26/02/2015 06:56, Cong Wang a écrit :
> On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang <cwang@twopensource.com> 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.

Re: What are the intended semantics of IFLA_LINK_NETNSID?

[Eric W. Biederman]

Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:

> 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.

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

Re: new link failing on duplicate names in different namespaces

[Eric W. Biederman]

Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:

> Le 26/02/2015 06:56, Cong Wang a écrit :
>> On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang <cwang@twopensource.com> 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.

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

Re: new link failing on duplicate names in different namespaces

[Nicolas Dichtel]

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).

Re: What are the intended semantics of IFLA_LINK_NETNSID?

[Nicolas Dichtel]

Le 26/02/2015 14:48, Eric W. Biederman a écrit :
> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
[snip]
>> 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

[PATCH net 1/2] net: Verify permission to dest_net in newlink

[Eric W. Biederman]

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" <ebiederm@xmission.com>
---
 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

[PATCH net 2/2] net: Verify permission to link_net in newlink

[Eric W. Biederman]

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" <ebiederm@xmission.com>
---
 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

Re: new link failing on duplicate names in different namespaces

[Cong Wang]

On Thu, Feb 26, 2015 at 1:14 AM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 26/02/2015 06:56, Cong Wang a écrit :
>>
>> On Wed, Feb 25, 2015 at 9:29 PM, Cong Wang <cwang@twopensource.com> 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).
>


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!

Re: [PATCH net 1/2] net: Verify permission to dest_net in newlink

[Nicolas Dichtel]

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 <nicolas.dichtel@6wind.com>

Re: [PATCH net 2/2] net: Verify permission to link_net in newlink

[Nicolas Dichtel]

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 <nicolas.dichtel@6wind.com>

Re: [PATCH net 1/2] net: Verify permission to dest_net in newlink

[David Miller]

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" <ebiederm@xmission.com>

Applied.

Re: [PATCH net 2/2] net: Verify permission to link_net in newlink

[David Miller]

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" <ebiederm@xmission.com>

Applied.