netns_id in bpf_sk_lookup_{tcp,udp}

netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

Hi Joe:

The netns_id to the bpf_sk_lookup_{tcp,udp} functions in
net/core/filter.c is a u64, yet the APIs in include/uapi/linux/bpf.h
shows a u32. Is that intentional or an oversight through the iterations?

David

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Joe Stringer]

Hi David, thanks for pointing this out.

This is more of an oversight through iterations, the runtime lookup
will fail to find a socket if the netns value is greater than the
range of a uint32 so I think it would actually make more sense to drop
the parameter size to u32 rather than u64 so that this would be
validated at load time rather than silently returning NULL because of
a bad parameter.

I'll send a patch to bpf tree.

Cheers,
Joe

On Sun, 18 Nov 2018 at 19:27, David Ahern <dsahern@gmail.com> wrote:
>
> Hi Joe:
>
> The netns_id to the bpf_sk_lookup_{tcp,udp} functions in
> net/core/filter.c is a u64, yet the APIs in include/uapi/linux/bpf.h
> shows a u32. Is that intentional or an oversight through the iterations?
>
> David

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

On 11/19/18 11:36 AM, Joe Stringer wrote:
> Hi David, thanks for pointing this out.
> 
> This is more of an oversight through iterations, the runtime lookup
> will fail to find a socket if the netns value is greater than the
> range of a uint32 so I think it would actually make more sense to drop
> the parameter size to u32 rather than u64 so that this would be
> validated at load time rather than silently returning NULL because of
> a bad parameter.

ok. I was wondering if it was a u64 to handle nsid of 0 which as I
understand it is a legal nsid. If you drop to u32, how do you know when
nsid has been set?

> 
> I'll send a patch to bpf tree.
> 
ok.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Joe Stringer]

On Mon, 19 Nov 2018 at 10:39, David Ahern <dsahern@gmail.com> wrote:
>
> On 11/19/18 11:36 AM, Joe Stringer wrote:
> > Hi David, thanks for pointing this out.
> >
> > This is more of an oversight through iterations, the runtime lookup
> > will fail to find a socket if the netns value is greater than the
> > range of a uint32 so I think it would actually make more sense to drop
> > the parameter size to u32 rather than u64 so that this would be
> > validated at load time rather than silently returning NULL because of
> > a bad parameter.
>
> ok. I was wondering if it was a u64 to handle nsid of 0 which as I
> understand it is a legal nsid. If you drop to u32, how do you know when
> nsid has been set?

I was operating under the assumption that 0 represents the root netns
id, and cannot be assigned to another non-root netns.

Looking at __peernet2id_alloc(), it seems to me like it attempts to
find a netns and if it cannot find one, returns 0, which then leads to
a scroll over the idr starting from 0 to INT_MAX to find a legitimate
id for the netns, so I think this is a fair assumption?

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

On 11/19/18 12:47 PM, Joe Stringer wrote:
> On Mon, 19 Nov 2018 at 10:39, David Ahern <dsahern@gmail.com> wrote:
>>
>> On 11/19/18 11:36 AM, Joe Stringer wrote:
>>> Hi David, thanks for pointing this out.
>>>
>>> This is more of an oversight through iterations, the runtime lookup
>>> will fail to find a socket if the netns value is greater than the
>>> range of a uint32 so I think it would actually make more sense to drop
>>> the parameter size to u32 rather than u64 so that this would be
>>> validated at load time rather than silently returning NULL because of
>>> a bad parameter.
>>
>> ok. I was wondering if it was a u64 to handle nsid of 0 which as I
>> understand it is a legal nsid. If you drop to u32, how do you know when
>> nsid has been set?
> 
> I was operating under the assumption that 0 represents the root netns
> id, and cannot be assigned to another non-root netns.
> 
> Looking at __peernet2id_alloc(), it seems to me like it attempts to
> find a netns and if it cannot find one, returns 0, which then leads to
> a scroll over the idr starting from 0 to INT_MAX to find a legitimate
> id for the netns, so I think this is a fair assumption?
> 

Maybe Nicolas can give a definitive answer; as I recall he added the
NSID option. I have not had time to walk the code. But I do recall
seeing an id of 0. e.g, on my dev box:
$ ip netns
vms (id: 0)

And include/uapi/linux/net_namespace.h shows -1 as not assigned.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Nicolas Dichtel]

Le 19/11/2018 à 20:54, David Ahern a écrit :
> On 11/19/18 12:47 PM, Joe Stringer wrote:
>> On Mon, 19 Nov 2018 at 10:39, David Ahern <dsahern@gmail.com> wrote:
>>>
>>> On 11/19/18 11:36 AM, Joe Stringer wrote:
>>>> Hi David, thanks for pointing this out.
>>>>
>>>> This is more of an oversight through iterations, the runtime lookup
>>>> will fail to find a socket if the netns value is greater than the
>>>> range of a uint32 so I think it would actually make more sense to drop
>>>> the parameter size to u32 rather than u64 so that this would be
>>>> validated at load time rather than silently returning NULL because of
>>>> a bad parameter.
>>>
>>> ok. I was wondering if it was a u64 to handle nsid of 0 which as I
>>> understand it is a legal nsid. If you drop to u32, how do you know when
>>> nsid has been set?
>>
>> I was operating under the assumption that 0 represents the root netns
>> id, and cannot be assigned to another non-root netns.
>>
>> Looking at __peernet2id_alloc(), it seems to me like it attempts to
>> find a netns and if it cannot find one, returns 0, which then leads to
>> a scroll over the idr starting from 0 to INT_MAX to find a legitimate
>> id for the netns, so I think this is a fair assumption?
The NET_ID_ZERO trick is used to manage nsid 0 in net_eq_idr() (idr_for_each()
stops when the callback returns != 0).

>>
> 
> Maybe Nicolas can give a definitive answer; as I recall he added the
> NSID option. I have not had time to walk the code. But I do recall
> seeing an id of 0. e.g, on my dev box:
> $ ip netns
> vms (id: 0)
> 
> And include/uapi/linux/net_namespace.h shows -1 as not assigned.
Yes, 0 is a valid value and can be assigned to any netns.
nsid are signed 32 bit values. Note that -1 (NETNSA_NSID_NOT_ASSIGNED) is used
by the kernel to express that the nsid is not assigned. It can also be used by
the user to let the kernel chooses a nsid.

$ ip netns add foo
$ ip netns add bar
$ ip netns
bar
foo
$ ip netns set foo 0
$ ip netns set bar auto
$ ip netns
bar (id: 1)
foo (id: 0)


Regards,
Nicolas

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Joe Stringer]

On Mon, 19 Nov 2018 at 12:29, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>
> Le 19/11/2018 à 20:54, David Ahern a écrit :
> > On 11/19/18 12:47 PM, Joe Stringer wrote:
> >> On Mon, 19 Nov 2018 at 10:39, David Ahern <dsahern@gmail.com> wrote:
> >>>
> >>> On 11/19/18 11:36 AM, Joe Stringer wrote:
> >>>> Hi David, thanks for pointing this out.
> >>>>
> >>>> This is more of an oversight through iterations, the runtime lookup
> >>>> will fail to find a socket if the netns value is greater than the
> >>>> range of a uint32 so I think it would actually make more sense to drop
> >>>> the parameter size to u32 rather than u64 so that this would be
> >>>> validated at load time rather than silently returning NULL because of
> >>>> a bad parameter.
> >>>
> >>> ok. I was wondering if it was a u64 to handle nsid of 0 which as I
> >>> understand it is a legal nsid. If you drop to u32, how do you know when
> >>> nsid has been set?
> >>
> >> I was operating under the assumption that 0 represents the root netns
> >> id, and cannot be assigned to another non-root netns.
> >>
> >> Looking at __peernet2id_alloc(), it seems to me like it attempts to
> >> find a netns and if it cannot find one, returns 0, which then leads to
> >> a scroll over the idr starting from 0 to INT_MAX to find a legitimate
> >> id for the netns, so I think this is a fair assumption?
> The NET_ID_ZERO trick is used to manage nsid 0 in net_eq_idr() (idr_for_each()
> stops when the callback returns != 0).
>
> >>
> >
> > Maybe Nicolas can give a definitive answer; as I recall he added the
> > NSID option. I have not had time to walk the code. But I do recall
> > seeing an id of 0. e.g, on my dev box:
> > $ ip netns
> > vms (id: 0)
> >
> > And include/uapi/linux/net_namespace.h shows -1 as not assigned.
> Yes, 0 is a valid value and can be assigned to any netns.
> nsid are signed 32 bit values. Note that -1 (NETNSA_NSID_NOT_ASSIGNED) is used
> by the kernel to express that the nsid is not assigned. It can also be used by
> the user to let the kernel chooses a nsid.
>
> $ ip netns add foo
> $ ip netns add bar
> $ ip netns
> bar
> foo
> $ ip netns set foo 0
> $ ip netns set bar auto
> $ ip netns
> bar (id: 1)
> foo (id: 0)

OK, I'll fix this up then.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Joe Stringer]

On Mon, 19 Nov 2018 at 12:54, Joe Stringer <joe@wand.net.nz> wrote:
>
> On Mon, 19 Nov 2018 at 12:29, Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> >
> > Le 19/11/2018 à 20:54, David Ahern a écrit :
> > > On 11/19/18 12:47 PM, Joe Stringer wrote:
> > >> On Mon, 19 Nov 2018 at 10:39, David Ahern <dsahern@gmail.com> wrote:
> > >>>
> > >>> On 11/19/18 11:36 AM, Joe Stringer wrote:
> > >>>> Hi David, thanks for pointing this out.
> > >>>>
> > >>>> This is more of an oversight through iterations, the runtime lookup
> > >>>> will fail to find a socket if the netns value is greater than the
> > >>>> range of a uint32 so I think it would actually make more sense to drop
> > >>>> the parameter size to u32 rather than u64 so that this would be
> > >>>> validated at load time rather than silently returning NULL because of
> > >>>> a bad parameter.
> > >>>
> > >>> ok. I was wondering if it was a u64 to handle nsid of 0 which as I
> > >>> understand it is a legal nsid. If you drop to u32, how do you know when
> > >>> nsid has been set?
> > >>
> > >> I was operating under the assumption that 0 represents the root netns
> > >> id, and cannot be assigned to another non-root netns.
> > >>
> > >> Looking at __peernet2id_alloc(), it seems to me like it attempts to
> > >> find a netns and if it cannot find one, returns 0, which then leads to
> > >> a scroll over the idr starting from 0 to INT_MAX to find a legitimate
> > >> id for the netns, so I think this is a fair assumption?
> > The NET_ID_ZERO trick is used to manage nsid 0 in net_eq_idr() (idr_for_each()
> > stops when the callback returns != 0).
> >
> > >>
> > >
> > > Maybe Nicolas can give a definitive answer; as I recall he added the
> > > NSID option. I have not had time to walk the code. But I do recall
> > > seeing an id of 0. e.g, on my dev box:
> > > $ ip netns
> > > vms (id: 0)
> > >
> > > And include/uapi/linux/net_namespace.h shows -1 as not assigned.
> > Yes, 0 is a valid value and can be assigned to any netns.
> > nsid are signed 32 bit values. Note that -1 (NETNSA_NSID_NOT_ASSIGNED) is used
> > by the kernel to express that the nsid is not assigned. It can also be used by
> > the user to let the kernel chooses a nsid.
> >
> > $ ip netns add foo
> > $ ip netns add bar
> > $ ip netns
> > bar
> > foo
> > $ ip netns set foo 0
> > $ ip netns set bar auto
> > $ ip netns
> > bar (id: 1)
> > foo (id: 0)
>
> OK, I'll fix this up then.

Here's what I have in mind:

@@ -2221,12 +2222,13 @@ union bpf_attr {
 *             **sizeof**\ (*tuple*\ **->ipv6**)
 *                     Look for an IPv6 socket.
 *
- *             If the *netns* is zero, then the socket lookup table in the
- *             netns associated with the *ctx* will be used. For the TC hooks,
- *             this in the netns of the device in the skb. For socket hooks,
- *             this in the netns of the socket. If *netns* is non-zero, then
- *             it specifies the ID of the netns relative to the netns
- *             associated with the *ctx*.
+ *             If the *netns* is **BPF_F_SK_CURRENT_NS** or greater, then the
+ *             socket lookup table in the netns associated with the *ctx* will
+ *             will be used. For the TC hooks, this is the netns of the device
+ *             in the skb. For socket hooks, this is the netns of the socket.
+ *             If *netns* is less than **BPF_F_SK_CURRENT_NS**, then it
+ *             specifies the ID of the netns relative to the netns associated
+ *             with the *ctx*.
 *
 *             All values for *flags* are reserved for future usage, and must
 *             be left at zero.
@@ -2409,6 +2411,9 @@ enum bpf_func_id {
/* BPF_FUNC_perf_event_output for sk_buff input context. */
#define BPF_F_CTXLEN_MASK              (0xfffffULL << 32)

+/* BPF_FUNC_sk_lookup_tcp and BPF_FUNC_sk_lookup_udp flags. */
+#define BPF_F_SK_CURRENT_NS            0x80000000 /* For netns argument */
+
/* Mode for BPF_FUNC_skb_adjust_room helper. */
enum bpf_adj_room_mode {
       BPF_ADJ_ROOM_NET,

Plus adjusting all of the internal types and the helper headers to use
u32. With the highest bit used to specify that the netns should be the
current netns, all other netns IDs should be available.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

On 11/19/18 2:59 PM, Joe Stringer wrote:
> @@ -2221,12 +2222,13 @@ union bpf_attr {
>  *             **sizeof**\ (*tuple*\ **->ipv6**)
>  *                     Look for an IPv6 socket.
>  *
> - *             If the *netns* is zero, then the socket lookup table in the
> - *             netns associated with the *ctx* will be used. For the TC hooks,
> - *             this in the netns of the device in the skb. For socket hooks,
> - *             this in the netns of the socket. If *netns* is non-zero, then
> - *             it specifies the ID of the netns relative to the netns
> - *             associated with the *ctx*.
> + *             If the *netns* is **BPF_F_SK_CURRENT_NS** or greater, then the
> + *             socket lookup table in the netns associated with the *ctx* will
> + *             will be used. For the TC hooks, this is the netns of the device
> + *             in the skb. For socket hooks, this is the netns of the socket.
> + *             If *netns* is less than **BPF_F_SK_CURRENT_NS**, then it
> + *             specifies the ID of the netns relative to the netns associated
> + *             with the *ctx*.
>  *
>  *             All values for *flags* are reserved for future usage, and must
>  *             be left at zero.
> @@ -2409,6 +2411,9 @@ enum bpf_func_id {
> /* BPF_FUNC_perf_event_output for sk_buff input context. */
> #define BPF_F_CTXLEN_MASK              (0xfffffULL << 32)
> 
> +/* BPF_FUNC_sk_lookup_tcp and BPF_FUNC_sk_lookup_udp flags. */
> +#define BPF_F_SK_CURRENT_NS            0x80000000 /* For netns argument */
> +
> /* Mode for BPF_FUNC_skb_adjust_room helper. */
> enum bpf_adj_room_mode {
>        BPF_ADJ_ROOM_NET,
> 
> Plus adjusting all of the internal types and the helper headers to use
> u32. With the highest bit used to specify that the netns should be the
> current netns, all other netns IDs should be available.
> 

That seems reasonable if the nsid limit is s32.

That revelation shows another hole:
$ ip netns add foo
$ ip netns set foo 0xffffffff
$ ip netns list
foo (id: 0)

Seems like alloc_netid() should error out if reqid < -1 (-1 being the
NETNSA_NSID_NOT_ASSIGNED flag) as opposed to blindly ignoring it.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Nicolas Dichtel]

Le 20/11/2018 à 00:46, David Ahern a écrit :
[snip]
> That revelation shows another hole:
> $ ip netns add foo
> $ ip netns set foo 0xffffffff
It also works with 0xf0000000 ...

> $ ip netns list
> foo (id: 0)
> 
> Seems like alloc_netid() should error out if reqid < -1 (-1 being the
> NETNSA_NSID_NOT_ASSIGNED flag) as opposed to blindly ignoring it.
alloc_netid() tries to allocate the specified nsid if this nsid is valid, ie >=
0, else it allocates a new nsid (actually the lower available).
This is the expected behavior.

For me, it's more an iproute2 problem, which parses an unsigned and silently
cast it to a signed value.

-----8<--------------------

>From 79bac98bfd0acbf2526a3427d5aba96564844209 Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 20 Nov 2018 09:59:46 +0100
Subject: ipnetns: parse nsid as a signed integer

Don't confuse the user, nsid is a signed interger, this kind of command
should return an error: 'ip netns set foo 0xffffffff'.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 ip/ipnetns.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0eac18cf2682..54346ac987cf 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -739,8 +739,7 @@ static int netns_set(int argc, char **argv)
 {
 	char netns_path[PATH_MAX];
 	const char *name;
-	unsigned int nsid;
-	int netns;
+	int netns, nsid;

 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
@@ -754,7 +753,7 @@ static int netns_set(int argc, char **argv)
 	/* If a negative nsid is specified the kernel will select the nsid. */
 	if (strcmp(argv[1], "auto") == 0)
 		nsid = -1;
-	else if (get_unsigned(&nsid, argv[1], 0))
+	else if (get_integer(&nsid, argv[1], 0))
 		invarg("Invalid \"netnsid\" value\n", argv[1]);

 	snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name);
-- 
2.13.2

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

On 11/20/18 2:05 AM, Nicolas Dichtel wrote:
> Le 20/11/2018 à 00:46, David Ahern a écrit :
> [snip]
>> That revelation shows another hole:
>> $ ip netns add foo
>> $ ip netns set foo 0xffffffff
> It also works with 0xf0000000 ...

yes, I realized last night I sent a bad example. I meant any negative
number besides -1

> 
>> $ ip netns list
>> foo (id: 0)
>>
>> Seems like alloc_netid() should error out if reqid < -1 (-1 being the
>> NETNSA_NSID_NOT_ASSIGNED flag) as opposed to blindly ignoring it.
> alloc_netid() tries to allocate the specified nsid if this nsid is valid, ie >=
> 0, else it allocates a new nsid (actually the lower available).
> This is the expected behavior.
> 
> For me, it's more an iproute2 problem, which parses an unsigned and silently
> cast it to a signed value.

so your intention is that any < 0 value means auto generate not just -1.

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[Nicolas Dichtel]

Le 20/11/2018 à 16:46, David Ahern a écrit :
> On 11/20/18 2:05 AM, Nicolas Dichtel wrote:
>> Le 20/11/2018 à 00:46, David Ahern a écrit :
[snip]
>>> Seems like alloc_netid() should error out if reqid < -1 (-1 being the
>>> NETNSA_NSID_NOT_ASSIGNED flag) as opposed to blindly ignoring it.
>> alloc_netid() tries to allocate the specified nsid if this nsid is valid, ie >=
>> 0, else it allocates a new nsid (actually the lower available).
>> This is the expected behavior.
>>
>> For me, it's more an iproute2 problem, which parses an unsigned and silently
>> cast it to a signed value.
> 
> so your intention is that any < 0 value means auto generate not just -1.
Yes. If a valid value is not provided, the kernel tries to allocate one.

Nicolas

Re: netns_id in bpf_sk_lookup_{tcp,udp}

[David Ahern]

On 11/20/18 2:05 AM, Nicolas Dichtel wrote:
> Le 20/11/2018 à 00:46, David Ahern a écrit :
> [snip]
>> That revelation shows another hole:
>> $ ip netns add foo
>> $ ip netns set foo 0xffffffff
> It also works with 0xf0000000 ...
> 
>> $ ip netns list
>> foo (id: 0)
>>
>> Seems like alloc_netid() should error out if reqid < -1 (-1 being the
>> NETNSA_NSID_NOT_ASSIGNED flag) as opposed to blindly ignoring it.
> alloc_netid() tries to allocate the specified nsid if this nsid is valid, ie >=
> 0, else it allocates a new nsid (actually the lower available).
> This is the expected behavior.
> 
> For me, it's more an iproute2 problem, which parses an unsigned and silently
> cast it to a signed value.
> 
> -----8<--------------------
> 
> From 79bac98bfd0acbf2526a3427d5aba96564844209 Mon Sep 17 00:00:00 2001
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Tue, 20 Nov 2018 09:59:46 +0100
> Subject: ipnetns: parse nsid as a signed integer
> 
> Don't confuse the user, nsid is a signed interger, this kind of command
> should return an error: 'ip netns set foo 0xffffffff'.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  ip/ipnetns.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
> index 0eac18cf2682..54346ac987cf 100644
> --- a/ip/ipnetns.c
> +++ b/ip/ipnetns.c
> @@ -739,8 +739,7 @@ static int netns_set(int argc, char **argv)
>  {
>  	char netns_path[PATH_MAX];
>  	const char *name;
> -	unsigned int nsid;
> -	int netns;
> +	int netns, nsid;
> 
>  	if (argc < 1) {
>  		fprintf(stderr, "No netns name specified\n");
> @@ -754,7 +753,7 @@ static int netns_set(int argc, char **argv)
>  	/* If a negative nsid is specified the kernel will select the nsid. */
>  	if (strcmp(argv[1], "auto") == 0)
>  		nsid = -1;
> -	else if (get_unsigned(&nsid, argv[1], 0))
> +	else if (get_integer(&nsid, argv[1], 0))
>  		invarg("Invalid \"netnsid\" value\n", argv[1]);
> 
>  	snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name);
> 

Nicolas: Can you send this formally and cc Stephen so it goes into the
master branch? Thanks

[PATCH iproute2] ipnetns: parse nsid as a signed integer

[Nicolas Dichtel]

Don't confuse the user, nsid is a signed integer, this kind of command
should return an error: 'ip netns set foo 0xffffffff'.

Also, a valid value is a positive value. To let the kernel chooses a value,
the keyword 'auto' must be used.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 ip/ipnetns.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 0eac18cf2682..03879b496343 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -35,6 +35,7 @@ static int usage(void)
 	fprintf(stderr, "       ip [-all] netns exec [NAME] cmd ...\n");
 	fprintf(stderr, "       ip netns monitor\n");
 	fprintf(stderr, "       ip netns list-id\n");
+	fprintf(stderr, "NETNSID := auto | POSITIVE-INT\n");
 	exit(-1);
 }
 
@@ -739,8 +740,7 @@ static int netns_set(int argc, char **argv)
 {
 	char netns_path[PATH_MAX];
 	const char *name;
-	unsigned int nsid;
-	int netns;
+	int netns, nsid;
 
 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
@@ -754,8 +754,10 @@ static int netns_set(int argc, char **argv)
 	/* If a negative nsid is specified the kernel will select the nsid. */
 	if (strcmp(argv[1], "auto") == 0)
 		nsid = -1;
-	else if (get_unsigned(&nsid, argv[1], 0))
+	else if (get_integer(&nsid, argv[1], 0))
 		invarg("Invalid \"netnsid\" value\n", argv[1]);
+	else if (nsid < 0)
+		invarg("\"netnsid\" value should be >= 0\n", argv[1]);
 
 	snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name);
 	netns = open(netns_path, O_RDONLY | O_CLOEXEC);
-- 
2.18.0

Re: [PATCH iproute2] ipnetns: parse nsid as a signed integer

[Stephen Hemminger]

On Wed, 21 Nov 2018 10:44:27 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

> Don't confuse the user, nsid is a signed integer, this kind of command
> should return an error: 'ip netns set foo 0xffffffff'.
> 
> Also, a valid value is a positive value. To let the kernel chooses a value,
> the keyword 'auto' must be used.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Thanks for catching this. Next time add a Fixes: tag if possible.

Applied.