[PATCH net 0/1 v2] rtnetlink: require unique netns identifier

[PATCH net 0/1 v2] rtnetlink: require unique netns identifier

[Christian Brauner]

Hey,

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing. For legacy reasons the kernel will
pick the IFLA_NET_NS_PID property first and then look for the
IFLA_NET_NS_FD property but there is no reason to extend this type of
behavior to network namespace ids. The regression potential is quite
minimal since the rtnetlink requests in question either won't allow
IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.

We obviously cannot prevent users from passing both IFLA_NET_NS_PID and
IFLA_NET_NS_FD since we have supported this somehow for a long time. So
the check I'm proposing is to only fail when both IFLA_IF_NETNSID, and
IFLA_NET_NS_PID or IFLA_NET_NS_FD are passed and they refer to different
network namespaces.

Thanks!
Christian

ChangeLog v1->v2:
* return errno when the specified network namespace id is invalid
* fill in struct netlink_ext_ack if the network namespace id is invalid
* rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
  indicate that a request without any network namespace identifying attributes
  is also considered valid.

ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace

Christian Brauner (1):
  rtnetlink: require unique netns identifier

 net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

-- 
2.14.1

[PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Christian Brauner]

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing. For legacy reasons the kernel will
pick the IFLA_NET_NS_PID property first and then look for the
IFLA_NET_NS_FD property but there is no reason to extend this type of
behavior to network namespace ids. The regression potential is quite
minimal since the rtnetlink requests in question either won't allow
IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
ChangeLog v1->v2:
* return errno when the specified network namespace id is invalid
* fill in struct netlink_ext_ack if the network namespace id is invalid
* rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
  indicate that a request without any network namespace identifying attributes
  is also considered valid.

ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace
---
 net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 56af8e41abfc..c096c4ff9a00 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
 	return net;
 }
 
+/* Verify that rtnetlink requests supporting network namespace ids
+ * do not pass additional properties referring to different network
+ * namespaces.
+ */
+static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
+				    struct netlink_ext_ack *extack)
+{
+	int ret = -EINVAL;
+	struct net *net = NULL, *unique_net = NULL;
+
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	/* Caller operates on the current network namespace. */
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;
+
+	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
+	if (!unique_net) {
+		NL_SET_ERR_MSG(extack, "invalid network namespace id");
+		return ret;
+	}
+
+	if (tb[IFLA_NET_NS_PID]) {
+		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
+		if (net != unique_net)
+			goto on_error;
+	}
+
+	if (tb[IFLA_NET_NS_FD]) {
+		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
+		if (net != unique_net)
+			goto on_error;
+	}
+
+	ret = 0;
+
+on_error:
+	put_net(unique_net);
+
+	if (net && !IS_ERR(net))
+		put_net(net);
+
+	if (ret != 0)
+		NL_SET_ERR_MSG(extack, "multiple network namespaces specified");
+
+	return ret;
+}
+
 static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 {
 	if (dev) {
@@ -2553,6 +2606,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
+	err = rtnl_ensure_unique_netns(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		goto errout;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -2649,6 +2706,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 
@@ -2802,6 +2863,10 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -3045,6 +3110,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(NETLINK_CB(skb).sk, tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IF_NETNSID]) {
 		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
 		tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);
-- 
2.14.1

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[David Ahern]

On 2/5/18 8:55 AM, Christian Brauner wrote:
> Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
> it is possible for userspace to send us requests with three different
> properties to identify a target network namespace. This affects at least
> RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
> network namespace which is confusing. For legacy reasons the kernel will
> pick the IFLA_NET_NS_PID property first and then look for the
> IFLA_NET_NS_FD property but there is no reason to extend this type of
> behavior to network namespace ids. The regression potential is quite
> minimal since the rtnetlink requests in question either won't allow
> IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
> support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> ChangeLog v1->v2:
> * return errno when the specified network namespace id is invalid
> * fill in struct netlink_ext_ack if the network namespace id is invalid
> * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
>   indicate that a request without any network namespace identifying attributes
>   is also considered valid.
> 
> ChangeLog v0->v1:
> * report a descriptive error to userspace via struct netlink_ext_ack
> * do not fail when multiple properties specifiy the same network namespace
> ---
>  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)

LGTM.

Acked-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Kirill Tkhai]

On 05.02.2018 18:55, Christian Brauner wrote:
> Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
> it is possible for userspace to send us requests with three different
> properties to identify a target network namespace. This affects at least
> RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
> network namespace which is confusing. For legacy reasons the kernel will
> pick the IFLA_NET_NS_PID property first and then look for the
> IFLA_NET_NS_FD property but there is no reason to extend this type of
> behavior to network namespace ids. The regression potential is quite
> minimal since the rtnetlink requests in question either won't allow
> IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
> support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> ChangeLog v1->v2:
> * return errno when the specified network namespace id is invalid
> * fill in struct netlink_ext_ack if the network namespace id is invalid
> * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
>   indicate that a request without any network namespace identifying attributes
>   is also considered valid.
> 
> ChangeLog v0->v1:
> * report a descriptive error to userspace via struct netlink_ext_ack
> * do not fail when multiple properties specifiy the same network namespace
> ---
>  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 56af8e41abfc..c096c4ff9a00 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
>  	return net;
>  }
>  
> +/* Verify that rtnetlink requests supporting network namespace ids
> + * do not pass additional properties referring to different network
> + * namespaces.
> + */
> +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
> +				    struct netlink_ext_ack *extack)
> +{
> +	int ret = -EINVAL;
> +	struct net *net = NULL, *unique_net = NULL;
> +
> +	/* Requests without network namespace ids have been able to specify
> +	 * multiple properties referring to different network namespaces so
> +	 * don't regress them.
> +	 */
> +	if (!tb[IFLA_IF_NETNSID])
> +		return 0;
> +
> +	/* Caller operates on the current network namespace. */
> +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> +		return 0;
> +
> +	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
> +	if (!unique_net) {
> +		NL_SET_ERR_MSG(extack, "invalid network namespace id");
> +		return ret;
> +	}
> +
> +	if (tb[IFLA_NET_NS_PID]) {
> +		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
> +		if (net != unique_net)
> +			goto on_error;
> +	}
> +
> +	if (tb[IFLA_NET_NS_FD]) {
> +		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
> +		if (net != unique_net)
> +			goto on_error;
> +	}
> +
> +	ret = 0;
> +
> +on_error:
> +	put_net(unique_net);
> +
> +	if (net && !IS_ERR(net))
> +		put_net(net);

1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing
to the same net, this function increments net::count in get_net_ns_by_pid() and
in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called.
So, after this function net::count will be incremented by 1, and it never will
die.

2)The whole approach does not seem good for me. The first reason is it's racy.
Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that
tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later,
as the pid may die or do setns(). Racy check is worse than no check at all.

The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/
get_net_ns_by_fd() will be called twice: the first time is in your check
and the second time is where they are actually used. This is not good for
performance.

What is the problem people pass several different tb[xxx] in one call? We
may just describe the order of tb[xxx] in man page and their priorities,
and ignore the rest after the first not zero tb[xxx] is found, and do that
in the place, where net from tb[xxx] in actually used. This is the thing
we already do.

Comparing to classic Linux interface such as syscalls, it's usual behavior
for them to ignore one argument, when another is set. Nobody confuses.

Kirill

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Christian Brauner]

On Tue, Feb 06, 2018 at 12:47:46AM +0300, Kirill Tkhai wrote:
> On 05.02.2018 18:55, Christian Brauner wrote:
> > Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
> > it is possible for userspace to send us requests with three different
> > properties to identify a target network namespace. This affects at least
> > RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
> > network namespace which is confusing. For legacy reasons the kernel will
> > pick the IFLA_NET_NS_PID property first and then look for the
> > IFLA_NET_NS_FD property but there is no reason to extend this type of
> > behavior to network namespace ids. The regression potential is quite
> > minimal since the rtnetlink requests in question either won't allow
> > IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
> > support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
> >> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > ChangeLog v1->v2:
> > * return errno when the specified network namespace id is invalid
> > * fill in struct netlink_ext_ack if the network namespace id is invalid
> > * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
> >   indicate that a request without any network namespace identifying attributes
> >   is also considered valid.
> > 
> > ChangeLog v0->v1:
> > * report a descriptive error to userspace via struct netlink_ext_ack
> > * do not fail when multiple properties specifiy the same network namespace
> > ---
> >  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 56af8e41abfc..c096c4ff9a00 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
> >  	return net;
> >  }
> >  
> > +/* Verify that rtnetlink requests supporting network namespace ids
> > + * do not pass additional properties referring to different network
> > + * namespaces.
> > + */
> > +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
> > +				    struct netlink_ext_ack *extack)
> > +{
> > +	int ret = -EINVAL;
> > +	struct net *net = NULL, *unique_net = NULL;
> > +
> > +	/* Requests without network namespace ids have been able to specify
> > +	 * multiple properties referring to different network namespaces so
> > +	 * don't regress them.
> > +	 */
> > +	if (!tb[IFLA_IF_NETNSID])
> > +		return 0;
> > +
> > +	/* Caller operates on the current network namespace. */
> > +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> > +		return 0;
> > +
> > +	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
> > +	if (!unique_net) {
> > +		NL_SET_ERR_MSG(extack, "invalid network namespace id");
> > +		return ret;
> > +	}
> > +
> > +	if (tb[IFLA_NET_NS_PID]) {
> > +		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
> > +		if (net != unique_net)
> > +			goto on_error;
> > +	}
> > +
> > +	if (tb[IFLA_NET_NS_FD]) {
> > +		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
> > +		if (net != unique_net)
> > +			goto on_error;
> > +	}
> > +
> > +	ret = 0;
> > +
> > +on_error:
> > +	put_net(unique_net);
> > +
> > +	if (net && !IS_ERR(net))
> > +		put_net(net);
> 
> 1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing
> to the same net, this function increments net::count in get_net_ns_by_pid() and
> in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called.
> So, after this function net::count will be incremented by 1, and it never will
> die.

Thanks for spotting this, Kirill.

> 
> 2)The whole approach does not seem good for me. The first reason is it's racy.
> Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that
> tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later,
> as the pid may die or do setns(). Racy check is worse than no check at all.
> 
> The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/
> get_net_ns_by_fd() will be called twice: the first time is in your check
> and the second time is where they are actually used. This is not good for
> performance.

If this is really a performance problem we can simply fix this by
performing the check when the target network namespace is retrieved in
each request. The intention for doing it in one function at the
beginning of each request was to make it generic and easily
understandable.

> 
> What is the problem people pass several different tb[xxx] in one call? We
> may just describe the order of tb[xxx] in man page and their priorities,
> and ignore the rest after the first not zero tb[xxx] is found, and do that
> in the place, where net from tb[xxx] in actually used. This is the thing
> we already do.
> 
> Comparing to classic Linux interface such as syscalls, it's usual behavior
> for them to ignore one argument, when another is set. Nobody confuses.

>From what I gather from recent discussions I had here using pids and
fds to perform operations on network namespaces in netlink requests is
not the future. Specifically, using pids and fds will not be extended to
existing or future requests that do not already support it.

It also very much smells like a security liability if what you've
outlined above is true: a user sends a request with a pid and the task
dies and the pid gets recycled. Now, we can't easily fix this by simply
ignoring pids and fds from here on since this would likely break a bunch
of userspace programs but we can ensure that if a network namespace
identifier is passed that no other way of retrieving the target network
namespace is passed. Especially with requests that already support pids
and fds. It's either that or reversing the order meaning that if a
network namespace identifier is passed then it should take precedence
over the other identifiers. Furthermore, this would also clearly
indicate that netns ids are the preferred way to perform operations on
network namespaces via netlink requests.

I also do not think that your suggestion of making guarantees in what
order additional netlink properties are evaluated is a good one. I don't
think we want to give userspace the impression that sticking a pid, fd,
and netnsid into the same netlink request is something that we actively
support.

What is certainly a good point is that if pids and fds are as you said
inherently racy then we shouldn't perform the check but do what my
original patch did and simply refuse to combine netns ids with pids
and/or fds.

Thanks for the comments!
Christian

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Kirill Tkhai]

Hi, Christian,

On 06.02.2018 02:24, Christian Brauner wrote:
> On Tue, Feb 06, 2018 at 12:47:46AM +0300, Kirill Tkhai wrote:
>> On 05.02.2018 18:55, Christian Brauner wrote:
>>> Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
>>> it is possible for userspace to send us requests with three different
>>> properties to identify a target network namespace. This affects at least
>>> RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
>>> network namespace which is confusing. For legacy reasons the kernel will
>>> pick the IFLA_NET_NS_PID property first and then look for the
>>> IFLA_NET_NS_FD property but there is no reason to extend this type of
>>> behavior to network namespace ids. The regression potential is quite
>>> minimal since the rtnetlink requests in question either won't allow
>>> IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
>>> support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
>>>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>>> ---
>>> ChangeLog v1->v2:
>>> * return errno when the specified network namespace id is invalid
>>> * fill in struct netlink_ext_ack if the network namespace id is invalid
>>> * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
>>>   indicate that a request without any network namespace identifying attributes
>>>   is also considered valid.
>>>
>>> ChangeLog v0->v1:
>>> * report a descriptive error to userspace via struct netlink_ext_ack
>>> * do not fail when multiple properties specifiy the same network namespace
>>> ---
>>>  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 69 insertions(+)
>>>
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>>> index 56af8e41abfc..c096c4ff9a00 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
>>>  	return net;
>>>  }
>>>  
>>> +/* Verify that rtnetlink requests supporting network namespace ids
>>> + * do not pass additional properties referring to different network
>>> + * namespaces.
>>> + */
>>> +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
>>> +				    struct netlink_ext_ack *extack)
>>> +{
>>> +	int ret = -EINVAL;
>>> +	struct net *net = NULL, *unique_net = NULL;
>>> +
>>> +	/* Requests without network namespace ids have been able to specify
>>> +	 * multiple properties referring to different network namespaces so
>>> +	 * don't regress them.
>>> +	 */
>>> +	if (!tb[IFLA_IF_NETNSID])
>>> +		return 0;
>>> +
>>> +	/* Caller operates on the current network namespace. */
>>> +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
>>> +		return 0;
>>> +
>>> +	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
>>> +	if (!unique_net) {
>>> +		NL_SET_ERR_MSG(extack, "invalid network namespace id");
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (tb[IFLA_NET_NS_PID]) {
>>> +		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
>>> +		if (net != unique_net)
>>> +			goto on_error;
>>> +	}
>>> +
>>> +	if (tb[IFLA_NET_NS_FD]) {
>>> +		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
>>> +		if (net != unique_net)
>>> +			goto on_error;
>>> +	}
>>> +
>>> +	ret = 0;
>>> +
>>> +on_error:
>>> +	put_net(unique_net);
>>> +
>>> +	if (net && !IS_ERR(net))
>>> +		put_net(net);
>>
>> 1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing
>> to the same net, this function increments net::count in get_net_ns_by_pid() and
>> in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called.
>> So, after this function net::count will be incremented by 1, and it never will
>> die.
> 
> Thanks for spotting this, Kirill.
> 
>>
>> 2)The whole approach does not seem good for me. The first reason is it's racy.
>> Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that
>> tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later,
>> as the pid may die or do setns(). Racy check is worse than no check at all.
>>
>> The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/
>> get_net_ns_by_fd() will be called twice: the first time is in your check
>> and the second time is where they are actually used. This is not good for
>> performance.
> 
> If this is really a performance problem we can simply fix this by
> performing the check when the target network namespace is retrieved in
> each request. The intention for doing it in one function at the
> beginning of each request was to make it generic and easily
> understandable.

I haven't measured the performance with stopwatch, of course, but this is
additional operations, and we should not use them unless they are really need.
The approach with get_net()/put_net() is racy and it does not solve the
problem. So it does not seem good for me despite it is generic.

>>
>> What is the problem people pass several different tb[xxx] in one call? We
>> may just describe the order of tb[xxx] in man page and their priorities,
>> and ignore the rest after the first not zero tb[xxx] is found, and do that
>> in the place, where net from tb[xxx] in actually used. This is the thing
>> we already do.
>>
>> Comparing to classic Linux interface such as syscalls, it's usual behavior
>> for them to ignore one argument, when another is set. Nobody confuses.
> 
> From what I gather from recent discussions I had here using pids and
> fds to perform operations on network namespaces in netlink requests is
> not the future. Specifically, using pids and fds will not be extended to
> existing or future requests that do not already support it.
> 
> It also very much smells like a security liability if what you've
> outlined above is true: a user sends a request with a pid and the task
> dies and the pid gets recycled. Now, we can't easily fix this by simply
> ignoring pids and fds from here on since this would likely break a bunch
> of userspace programs but we can ensure that if a network namespace
> identifier is passed that no other way of retrieving the target network
> namespace is passed. Especially with requests that already support pids
> and fds. It's either that or reversing the order meaning that if a
> network namespace identifier is passed then it should take precedence
> over the other identifiers. Furthermore, this would also clearly
> indicate that netns ids are the preferred way to perform operations on
> network namespaces via netlink requests.

If we really need this, can't we simply zero excess identifiers instead?

void rtnl_kill_them_all(struct nlattr *tb[])
{
	if (!tb[IFLA_IF_NETNSID])
		return;
	tb[IFLA_NET_NS_PID] = tb[IFLA_NET_NS_FD] = NULL;
}

It's not racy and solves the problem you are solving.

> I also do not think that your suggestion of making guarantees in what
> order additional netlink properties are evaluated is a good one. I don't
> think we want to give userspace the impression that sticking a pid, fd,
> and netnsid into the same netlink request is something that we actively
> support.
> 
> What is certainly a good point is that if pids and fds are as you said
> inherently racy then we shouldn't perform the check but do what my
> original patch did and simply refuse to combine netns ids with pids
> and/or fds.

Thanks,
Kirill

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Christian Brauner]

On Tue, Feb 06, 2018 at 01:49:10PM +0300, Kirill Tkhai wrote:
> Hi, Christian,
> 
> On 06.02.2018 02:24, Christian Brauner wrote:
> > On Tue, Feb 06, 2018 at 12:47:46AM +0300, Kirill Tkhai wrote:
> >> On 05.02.2018 18:55, Christian Brauner wrote:
> >>> Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
> >>> it is possible for userspace to send us requests with three different
> >>> properties to identify a target network namespace. This affects at least
> >>> RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
> >>> network namespace which is confusing. For legacy reasons the kernel will
> >>> pick the IFLA_NET_NS_PID property first and then look for the
> >>> IFLA_NET_NS_FD property but there is no reason to extend this type of
> >>> behavior to network namespace ids. The regression potential is quite
> >>> minimal since the rtnetlink requests in question either won't allow
> >>> IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
> >>> support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
> >>>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >>> ---
> >>> ChangeLog v1->v2:
> >>> * return errno when the specified network namespace id is invalid
> >>> * fill in struct netlink_ext_ack if the network namespace id is invalid
> >>> * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
> >>>   indicate that a request without any network namespace identifying attributes
> >>>   is also considered valid.
> >>>
> >>> ChangeLog v0->v1:
> >>> * report a descriptive error to userspace via struct netlink_ext_ack
> >>> * do not fail when multiple properties specifiy the same network namespace
> >>> ---
> >>>  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 69 insertions(+)
> >>>
> >>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> >>> index 56af8e41abfc..c096c4ff9a00 100644
> >>> --- a/net/core/rtnetlink.c
> >>> +++ b/net/core/rtnetlink.c
> >>> @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
> >>>  	return net;
> >>>  }
> >>>  
> >>> +/* Verify that rtnetlink requests supporting network namespace ids
> >>> + * do not pass additional properties referring to different network
> >>> + * namespaces.
> >>> + */
> >>> +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
> >>> +				    struct netlink_ext_ack *extack)
> >>> +{
> >>> +	int ret = -EINVAL;
> >>> +	struct net *net = NULL, *unique_net = NULL;
> >>> +
> >>> +	/* Requests without network namespace ids have been able to specify
> >>> +	 * multiple properties referring to different network namespaces so
> >>> +	 * don't regress them.
> >>> +	 */
> >>> +	if (!tb[IFLA_IF_NETNSID])
> >>> +		return 0;
> >>> +
> >>> +	/* Caller operates on the current network namespace. */
> >>> +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> >>> +		return 0;
> >>> +
> >>> +	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
> >>> +	if (!unique_net) {
> >>> +		NL_SET_ERR_MSG(extack, "invalid network namespace id");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	if (tb[IFLA_NET_NS_PID]) {
> >>> +		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
> >>> +		if (net != unique_net)
> >>> +			goto on_error;
> >>> +	}
> >>> +
> >>> +	if (tb[IFLA_NET_NS_FD]) {
> >>> +		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
> >>> +		if (net != unique_net)
> >>> +			goto on_error;
> >>> +	}
> >>> +
> >>> +	ret = 0;
> >>> +
> >>> +on_error:
> >>> +	put_net(unique_net);
> >>> +
> >>> +	if (net && !IS_ERR(net))
> >>> +		put_net(net);
> >>
> >> 1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing
> >> to the same net, this function increments net::count in get_net_ns_by_pid() and
> >> in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called.
> >> So, after this function net::count will be incremented by 1, and it never will
> >> die.
> > 
> > Thanks for spotting this, Kirill.
> > 
> >>
> >> 2)The whole approach does not seem good for me. The first reason is it's racy.
> >> Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that
> >> tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later,
> >> as the pid may die or do setns(). Racy check is worse than no check at all.
> >>
> >> The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/
> >> get_net_ns_by_fd() will be called twice: the first time is in your check
> >> and the second time is where they are actually used. This is not good for
> >> performance.
> > 
> > If this is really a performance problem we can simply fix this by
> > performing the check when the target network namespace is retrieved in
> > each request. The intention for doing it in one function at the
> > beginning of each request was to make it generic and easily
> > understandable.
> 
> I haven't measured the performance with stopwatch, of course, but this is
> additional operations, and we should not use them unless they are really need.
> The approach with get_net()/put_net() is racy and it does not solve the
> problem. So it does not seem good for me despite it is generic.
> 
> >>
> >> What is the problem people pass several different tb[xxx] in one call? We
> >> may just describe the order of tb[xxx] in man page and their priorities,
> >> and ignore the rest after the first not zero tb[xxx] is found, and do that
> >> in the place, where net from tb[xxx] in actually used. This is the thing
> >> we already do.
> >>
> >> Comparing to classic Linux interface such as syscalls, it's usual behavior
> >> for them to ignore one argument, when another is set. Nobody confuses.
> > 
> > From what I gather from recent discussions I had here using pids and
> > fds to perform operations on network namespaces in netlink requests is
> > not the future. Specifically, using pids and fds will not be extended to
> > existing or future requests that do not already support it.
> > 
> > It also very much smells like a security liability if what you've
> > outlined above is true: a user sends a request with a pid and the task
> > dies and the pid gets recycled. Now, we can't easily fix this by simply
> > ignoring pids and fds from here on since this would likely break a bunch
> > of userspace programs but we can ensure that if a network namespace
> > identifier is passed that no other way of retrieving the target network
> > namespace is passed. Especially with requests that already support pids
> > and fds. It's either that or reversing the order meaning that if a
> > network namespace identifier is passed then it should take precedence
> > over the other identifiers. Furthermore, this would also clearly
> > indicate that netns ids are the preferred way to perform operations on
> > network namespaces via netlink requests.
> 
> If we really need this, can't we simply zero excess identifiers instead?
> 
> void rtnl_kill_them_all(struct nlattr *tb[])
> {
> 	if (!tb[IFLA_IF_NETNSID])
> 		return;
> 	tb[IFLA_NET_NS_PID] = tb[IFLA_NET_NS_FD] = NULL;
> }
> 
> It's not racy and solves the problem you are solving.

I take it that you haven't seen the first version of my patch which does
exactly that: https://patchwork.kernel.org/patch/10196409/ :)
I'm going to resend this one with the extack fixes added.

Thanks!
Christian

> 
> > I also do not think that your suggestion of making guarantees in what
> > order additional netlink properties are evaluated is a good one. I don't
> > think we want to give userspace the impression that sticking a pid, fd,
> > and netnsid into the same netlink request is something that we actively
> > support.
> > 
> > What is certainly a good point is that if pids and fds are as you said
> > inherently racy then we shouldn't perform the check but do what my
> > original patch did and simply refuse to combine netns ids with pids
> > and/or fds.
> 
> Thanks,
> Kirill

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Eric W. Biederman]

Christian Brauner <christian.brauner@canonical.com> writes:

> On Tue, Feb 06, 2018 at 12:47:46AM +0300, Kirill Tkhai wrote:
>> On 05.02.2018 18:55, Christian Brauner wrote:
>> > Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
>> > it is possible for userspace to send us requests with three different
>> > properties to identify a target network namespace. This affects at least
>> > RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
>> > network namespace which is confusing. For legacy reasons the kernel will
>> > pick the IFLA_NET_NS_PID property first and then look for the
>> > IFLA_NET_NS_FD property but there is no reason to extend this type of
>> > behavior to network namespace ids. The regression potential is quite
>> > minimal since the rtnetlink requests in question either won't allow
>> > IFLA_IF_NETNSID requests before 4.16 is out (RTM_{NEW,SET}LINK) or don't
>> > support IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.
>> >> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>> > ---
>> > ChangeLog v1->v2:
>> > * return errno when the specified network namespace id is invalid
>> > * fill in struct netlink_ext_ack if the network namespace id is invalid
>> > * rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
>> >   indicate that a request without any network namespace identifying attributes
>> >   is also considered valid.
>> > 
>> > ChangeLog v0->v1:
>> > * report a descriptive error to userspace via struct netlink_ext_ack
>> > * do not fail when multiple properties specifiy the same network namespace
>> > ---
>> >  net/core/rtnetlink.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 69 insertions(+)
>> > 
>> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> > index 56af8e41abfc..c096c4ff9a00 100644
>> > --- a/net/core/rtnetlink.c
>> > +++ b/net/core/rtnetlink.c
>> > @@ -1951,6 +1951,59 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
>> >  	return net;
>> >  }
>> >  
>> > +/* Verify that rtnetlink requests supporting network namespace ids
>> > + * do not pass additional properties referring to different network
>> > + * namespaces.
>> > + */
>> > +static int rtnl_ensure_unique_netns(const struct sock *sk, struct nlattr *tb[],
>> > +				    struct netlink_ext_ack *extack)
>> > +{
>> > +	int ret = -EINVAL;
>> > +	struct net *net = NULL, *unique_net = NULL;
>> > +
>> > +	/* Requests without network namespace ids have been able to specify
>> > +	 * multiple properties referring to different network namespaces so
>> > +	 * don't regress them.
>> > +	 */
>> > +	if (!tb[IFLA_IF_NETNSID])
>> > +		return 0;
>> > +
>> > +	/* Caller operates on the current network namespace. */
>> > +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
>> > +		return 0;
>> > +
>> > +	unique_net = get_net_ns_by_id(sock_net(sk), nla_get_s32(tb[IFLA_IF_NETNSID]));
>> > +	if (!unique_net) {
>> > +		NL_SET_ERR_MSG(extack, "invalid network namespace id");
>> > +		return ret;
>> > +	}
>> > +
>> > +	if (tb[IFLA_NET_NS_PID]) {
>> > +		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
>> > +		if (net != unique_net)
>> > +			goto on_error;
>> > +	}
>> > +
>> > +	if (tb[IFLA_NET_NS_FD]) {
>> > +		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
>> > +		if (net != unique_net)
>> > +			goto on_error;
>> > +	}
>> > +
>> > +	ret = 0;
>> > +
>> > +on_error:
>> > +	put_net(unique_net);
>> > +
>> > +	if (net && !IS_ERR(net))
>> > +		put_net(net);
>> 
>> 1)When we have tb[IFLA_NET_NS_PID and tb[IFLA_NET_NS_FD] both set and pointing
>> to the same net, this function increments net::count in get_net_ns_by_pid() and
>> in get_net_ns_by_fd(), i.e. twice. But only single put_net(net) will be called.
>> So, after this function net::count will be incremented by 1, and it never will
>> die.
>
> Thanks for spotting this, Kirill.
>
>> 
>> 2)The whole approach does not seem good for me. The first reason is it's racy.
>> Even if rtnl_ensure_unique_netns() returns 0, this does not guarantees that
>> tb[IFLA_IF_NETNSID] and tb[IFLA_NET_NS_PID] will be point the same net later,
>> as the pid may die or do setns(). Racy check is worse than no check at all.
>> 
>> The second reason is after this patch get_net_ns_by_id/get_net_ns_by_pid()/
>> get_net_ns_by_fd() will be called twice: the first time is in your check
>> and the second time is where they are actually used. This is not good for
>> performance.
>
> If this is really a performance problem we can simply fix this by
> performing the check when the target network namespace is retrieved in
> each request. The intention for doing it in one function at the
> beginning of each request was to make it generic and easily
> understandable.
>
>> 
>> What is the problem people pass several different tb[xxx] in one call? We
>> may just describe the order of tb[xxx] in man page and their priorities,
>> and ignore the rest after the first not zero tb[xxx] is found, and do that
>> in the place, where net from tb[xxx] in actually used. This is the thing
>> we already do.
>> 
>> Comparing to classic Linux interface such as syscalls, it's usual behavior
>> for them to ignore one argument, when another is set. Nobody confuses.
>
> From what I gather from recent discussions I had here using pids and
> fds to perform operations on network namespaces in netlink requests is
> not the future. Specifically, using pids and fds will not be extended to
> existing or future requests that do not already support it.

Pids are essentially deprecated in that fashion.  At one point they were
the best we had.  File descriptors will not be.

The use case for a netnsid and for a fd to identify network namespaces
are very different.

With netnsid's you have a strongly related set of network namespaces
that are cooperating in some way.  File descriptors don't need to assume
any kind of association between network namespaces.

Plus there are some very real costs to netnsid's that file descriptor's
don't have.

> It also very much smells like a security liability if what you've
> outlined above is true: a user sends a request with a pid and the task
> dies and the pid gets recycled. Now, we can't easily fix this by simply
> ignoring pids and fds from here on since this would likely break a bunch
> of userspace programs but we can ensure that if a network namespace
> identifier is passed that no other way of retrieving the target network
> namespace is passed. Especially with requests that already support pids
> and fds. It's either that or reversing the order meaning that if a
> network namespace identifier is passed then it should take precedence
> over the other identifiers. Furthermore, this would also clearly
> indicate that netns ids are the preferred way to perform operations on
> network namespaces via netlink requests.

Frankly.  If we are talking precedence it should be:
fds
netnsids
pids


I do think it makes a lot of sense to error if someone passes in
duplicate arguments.  AKA multiple attribute that could select for
the same thing.   No one will do that deliberately.  It doesn't make
sense.  So it is just a nonsense case we have to handle gracefully,
and correctly.  With correctness being the most important as otherwise
people might just send in nonsense to exploit bugs.

Typically sending errors can be done statelessly.  So it has the lowest
risk.

> What is certainly a good point is that if pids and fds are as you said
> inherently racy then we shouldn't perform the check but do what my
> original patch did and simply refuse to combine netns ids with pids
> and/or fds.

I agree refusing to combine multiple attributes for the same thing
sounds the most sensible course.

Eric

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[kbuild test robot]

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

Hi Christian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Christian-Brauner/rtnetlink-require-unique-netns-identifier/20180207-064207
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817de851:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817de85f:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817decc2:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817decf1:	0f ff c3             	ud0    %ebx,%eax
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817def6c:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817df332:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e1947:	0f ff 44 8b ad       	ud0    -0x53(%rbx,%rcx,4),%eax
   arch/x86/tools/insn_decoder_test: warning: objdump says 5 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e2552:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e2585:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e26d8:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e2752:	0f ff 48 8d          	ud0    -0x73(%rax),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e2801:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e305e:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e3559:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e3fd8:	0f ff 48 8b          	ud0    -0x75(%rax),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e4021:	0f ff 48 89          	ud0    -0x77(%rax),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e4e15:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e6571:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e67a8:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e6b25:	0f ff 4c 89 e6       	ud0    -0x1a(%rcx,%rcx,4),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 5 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e6dab:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e74f1:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e75b5:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e7b51:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e957d:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e9b18:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817e9f9b:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817ea42a:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817eb29f:	0f ff 4d 85          	ud0    -0x7b(%rbp),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817eb2b7:	0f ff 4c 89 20       	ud0    0x20(%rcx,%rcx,4),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 5 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817eb2cf:	0f ff 48 89          	ud0    -0x77(%rax),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817ebaf3:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
   arch/x86/tools/insn_decoder_test: warning: ffffffff817ebd58:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ed1a3:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee01c:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee061:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee12f:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee18d:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee3ee:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ee5c1:	0f ff 49 89          	ud0    -0x77(%rcx),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817eeb54:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817eeb5c:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817eeb7c:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817ef97c:	0f ff 8b 0c 24 e9 57 	ud0    0x57e9240c(%rbx),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 7 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efa01:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efa0f:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efa58:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efa66:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efa71:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817efc8e:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817f015d:	0f ff 48 89          	ud0    -0x77(%rax),%ecx
   arch/x86/tools/insn_decoder_test: warning: objdump says 4 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817f04fc:	0f ff e9             	ud0    %ecx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.
>> arch/x86/tools/insn_decoder_test: warning: ffffffff817f11cc:	0f ff eb             	ud0    %ebx,%ebp
   arch/x86/tools/insn_decoder_test: warning: objdump says 3 bytes, but insn_get_length() says 2
   arch/x86/tools/insn_decoder_test: warning: Found an x86 instruction decoder bug, please report this.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40655 bytes --]

Re: [PATCH net 1/1 v2] rtnetlink: require unique netns identifier

[Jiri Benc]

On Tue, 06 Feb 2018 16:31:29 -0600, Eric W. Biederman wrote:
> Frankly.  If we are talking precedence it should be:
> fds
> netnsids
> pids

The current order is 1. pids, 2. fds, though. Not that it matters much,
see below.

> I do think it makes a lot of sense to error if someone passes in
> duplicate arguments.  AKA multiple attribute that could select for
> the same thing.   No one will do that deliberately.  It doesn't make
> sense.  So it is just a nonsense case we have to handle gracefully,
> and correctly.  With correctness being the most important as otherwise
> people might just send in nonsense to exploit bugs.

Completely agreed. Let's just start returning error if more than one of
the pid/fs/netnsid attributes is specified. I don't think this is going
to break any user. And we'll not have to care about the order.

> I agree refusing to combine multiple attributes for the same thing
> sounds the most sensible course.

Yes, please.

Thanks!

 Jiri