On Sat, Dec 23, 2017 at 5:12 PM, Nicolas Dichtel
<nicolas.dich...@6wind.com> wrote:
> Le 22/12/2017 à 21:36, Craig Gallek a écrit :
>> From: Craig Gallek <kr...@google.com>
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index 60a71be75aea..4b7ea33f5705 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -627,6 +627,8 @@ static int rtnl_net_newid(struct sk_buff *skb, struct
>> nlmsghdr *nlh,
>> return -EINVAL;
>> }
>> nsid = nla_get_s32(tb[NETNSA_NSID]);
>> + if (nsid < 0)
>> + return -EINVAL;
> No, this breaks the current behavior.
> Look at alloc_netid(). If reqid is < 0, the kernel allocates an nsid with no
> constraint. If reqid is >= 0, it tries to alloc the specified nsid.
Ah, thanks. alloc_netid does appear to do the right thing. In fact,
this seems to be another clue to the problem. The current behavior is
to allocate from [0,max) when the input value is negative and the
problem seems to trigger when 0 is allocated. Changing this range to
[1, max) fixes the problem, so there must be code elsewhere that does
not handle the case where the id is zero properly...
>>
>> if (tb[NETNSA_PID]) {
>> peer = get_net_ns_by_pid(nla_get_u32(tb[NETNSA_PID]));
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index dabba2a91fc8..a928b8f081b8 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -1733,10 +1733,12 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,
>> struct netlink_callback *cb)
>> ifla_policy, NULL) >= 0) {
>> if (tb[IFLA_IF_NETNSID]) {
>> netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
>> - tgt_net = get_target_net(skb, netnsid);
>> - if (IS_ERR(tgt_net)) {
>> - tgt_net = net;
>> - netnsid = -1;
>> + if (netnsid >= 0) {
>> + tgt_net = get_target_net(skb, netnsid);
> I would prefer to put this test in get_target_net.
>
>> + if (IS_ERR(tgt_net)) {
>> + tgt_net = net;
>> + netnsid = -1;
> Maybe using NETNSA_NSID_NOT_ASSIGNED is better? Same for the initialization of
> this variable.
>
>> + }
>> }
>> }
>>
>> @@ -2792,6 +2794,11 @@ static int rtnl_newlink(struct sk_buff *skb, struct
>> nlmsghdr *nlh,
>> if (tb[IFLA_LINK_NETNSID]) {
>> int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>>
>> + if (id < 0) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>> +
> This is not needed. get_net_ns_by_id() returns NULL if id is < 0.
Indeed, and by extension get_target_net does not need this check
either (as it calls get_net_ns_by_id).
I'm having trouble debugging this remotely, so I'll give it a whirl
when I get back to the office next week.
Thanks again for the pointers,
Craig
