Message ID | 20180928192841.20410-1-dsahern@kernel.org |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net] rtnetlink: Fail dump if target netnsid is invalid | expand |
From: David Ahern <dsahern@kernel.org> Date: Fri, 28 Sep 2018 12:28:41 -0700 > From: David Ahern <dsahern@gmail.com> > > Link dumps can return results from a target namespace. If the namespace id > is invalid, then the dump request should fail if get_target_net fails > rather than continuing with a dump of the current namespace. > > Fixes: 79e1ad148c844 ("rtnetlink: use netnsid to query interface") > Signed-off-by: David Ahern <dsahern@gmail.com> Applied and queued up for -stable, thanks.
On Fri, 28 Sep 2018 12:28:41 -0700, David Ahern wrote: > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1898,10 +1898,8 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > if (tb[IFLA_IF_NETNSID]) { > netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]); > tgt_net = get_target_net(skb->sk, netnsid); > - if (IS_ERR(tgt_net)) { > - tgt_net = net; > - netnsid = -1; > - } > + if (IS_ERR(tgt_net)) > + return PTR_ERR(tgt_net); > } > > if (tb[IFLA_EXT_MASK]) Sorry for the late review, I see it has been applied. I intentionally chose the behavior to preserve the behavior of the older kernels: that attribute was silently ignored. Note that the IFLA_IF_NETNSID is not returned in such case, thus it's easy to distinguish that it was not applied. And the user space has to do such check anyway to support old kernels. But you're right that there was no way to distinguish "the kernel does not support IFLA_IF_NETNSID" from "wrong IFLA_IF_NETNSID provided". I'm okay with the patch, I just don't think the "Fixes" tag is justified but whatever, can't be unapplied :-) (and it's my fault for not reviewing the patches timely). Thanks! Jiri
On 10/2/18 4:04 AM, Jiri Benc wrote: > On Fri, 28 Sep 2018 12:28:41 -0700, David Ahern wrote: >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -1898,10 +1898,8 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) >> if (tb[IFLA_IF_NETNSID]) { >> netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]); >> tgt_net = get_target_net(skb->sk, netnsid); >> - if (IS_ERR(tgt_net)) { >> - tgt_net = net; >> - netnsid = -1; >> - } >> + if (IS_ERR(tgt_net)) >> + return PTR_ERR(tgt_net); >> } >> >> if (tb[IFLA_EXT_MASK]) > > Sorry for the late review, I see it has been applied. > > I intentionally chose the behavior to preserve the behavior of the > older kernels: that attribute was silently ignored. Note that the > IFLA_IF_NETNSID is not returned in such case, thus it's easy to > distinguish that it was not applied. And the user space has to do such > check anyway to support old kernels. First, rtnl_dellink, rtnl_newlink and rtnl_getlink all fail if net namespace id is invalid. Second, the user is requesting data from a target namespace and the dump happily continued with the current namespace which is not what the user requested. Hence, it makes no sense for a dump to continue which is why I sent the patch. > > But you're right that there was no way to distinguish "the kernel does > not support IFLA_IF_NETNSID" from "wrong IFLA_IF_NETNSID provided". I'm > okay with the patch, I just don't think the "Fixes" tag is justified but > whatever, can't be unapplied :-) (and it's my fault for not reviewing > the patches timely). The id was the commit that added the code to ignore the error.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 63ce2283a456..7f37fe9c65a5 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1898,10 +1898,8 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) if (tb[IFLA_IF_NETNSID]) { netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]); tgt_net = get_target_net(skb->sk, netnsid); - if (IS_ERR(tgt_net)) { - tgt_net = net; - netnsid = -1; - } + if (IS_ERR(tgt_net)) + return PTR_ERR(tgt_net); } if (tb[IFLA_EXT_MASK])