[PATCH net] net: enable interface alias removal via rtnl

[PATCH net] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
the attribute is 1 ("\0"). However, to remove an alias, the attribute
length must be 0 (see dev_set_alias()).

Let's define the type to NLA_BINARY, so that the alias can be removed.

Example:
$ ip l s dummy0 alias foo
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
    alias foo

Before the patch:
$ ip l s dummy0 alias ""
RTNETLINK answers: Numerical result out of range

After the patch:
$ ip l s dummy0 alias ""
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff

CC: Oliver Hartkopp <oliver@hartkopp.net>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <julien.floret@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4bcdcc68e92..570092cee902 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1483,7 +1483,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
 	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
 	[IFLA_NET_NS_FD]	= { .type = NLA_U32 },
-	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
+	[IFLA_IFALIAS]	        = { .type = NLA_BINARY, .len = IFALIASZ - 1 },
 	[IFLA_VFINFO_LIST]	= {. type = NLA_NESTED },
 	[IFLA_VF_PORTS]		= { .type = NLA_NESTED },
 	[IFLA_PORT_SELF]	= { .type = NLA_NESTED },
-- 
2.13.2

Re: [PATCH net] net: enable interface alias removal via rtnl

[David Ahern]

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
> the attribute is 1 ("\0"). However, to remove an alias, the attribute
> length must be 0 (see dev_set_alias()).

why not add a check in dev_set_alias that if len is 1 and the 1
character is '\0' it means remove the alias?

> 
> Let's define the type to NLA_BINARY, so that the alias can be removed.

that changes the uapi

Re: [PATCH net] net: enable interface alias removal via rtnl

[Oliver Hartkopp]

On 10/06/2017 08:18 PM, David Ahern wrote:
> On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>> length must be 0 (see dev_set_alias()).
> 
> why not add a check in dev_set_alias that if len is 1 and the 1
> character is '\0' it means remove the alias?

Yes. That looks indeed better than changing NLA_STRING to NLA_BINARY 
which does not really hit the point.

Nicolas, can you send an updated patch picking up David's suggestion?

Tnx & best regards,
Oliver

> 
>>
>> Let's define the type to NLA_BINARY, so that the alias can be removed.
> 
> that changes the uapi
> 

Re: [PATCH net] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :
> 
> 
> On 10/06/2017 08:18 PM, David Ahern wrote:
>> On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
>>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>>> length must be 0 (see dev_set_alias()).
>>
>> why not add a check in dev_set_alias that if len is 1 and the 1
>> character is '\0' it means remove the alias?
Because it requires an iproute2 patch. iproute2 doesn't send the '\0'. With the
command 'ip link set dummy0 alias ""', the attribute length is 0.
A kernel patch is probably enough for this problem. Updating iproute2 on old
distributions is not always easy.

> 
> Yes. That looks indeed better than changing NLA_STRING to NLA_BINARY which does
> not really hit the point.
> 
> Nicolas, can you send an updated patch picking up David's suggestion?
> 
> Tnx & best regards,
> Oliver
> 
>>
>>>
>>> Let's define the type to NLA_BINARY, so that the alias can be removed.
>>
>> that changes the uapi
>>
I don't understand what will be broken.

Re: [PATCH net] net: enable interface alias removal via rtnl

[David Ahern]

On 10/9/17 2:23 AM, Nicolas Dichtel wrote:
> Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :
>>
>>
>> On 10/06/2017 08:18 PM, David Ahern wrote:
>>> On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
>>>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>>>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>>>> length must be 0 (see dev_set_alias()).
>>>
>>> why not add a check in dev_set_alias that if len is 1 and the 1
>>> character is '\0' it means remove the alias?
> Because it requires an iproute2 patch. iproute2 doesn't send the '\0'. With the
> command 'ip link set dummy0 alias ""', the attribute length is 0.

iproute2 needs the feature for 0-len strings or perhaps a 'noalias' option.

You can reset the alias using the sysfs file. Given that there is a
workaround for existing kernels and userspace, upstream can get fixed
without changing the UAPI.

> A kernel patch is probably enough for this problem. Updating iproute2 on old
> distributions is not always easy.

Can't say I have ever heard someone suggest that a kernel is easier to
change than userspace.

Re: [PATCH net] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

Le 09/10/2017 à 16:02, David Ahern a écrit :
> On 10/9/17 2:23 AM, Nicolas Dichtel wrote:
>> Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :
>>>
>>>
>>> On 10/06/2017 08:18 PM, David Ahern wrote:
>>>> On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
>>>>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>>>>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>>>>> length must be 0 (see dev_set_alias()).
>>>>
>>>> why not add a check in dev_set_alias that if len is 1 and the 1
>>>> character is '\0' it means remove the alias?
>> Because it requires an iproute2 patch. iproute2 doesn't send the '\0'. With the
>> command 'ip link set dummy0 alias ""', the attribute length is 0.
> 
> iproute2 needs the feature for 0-len strings or perhaps a 'noalias' option.
iproute2 needs nothing ...

> 
> You can reset the alias using the sysfs file. Given that there is a
> workaround for existing kernels and userspace, upstream can get fixed
> without changing the UAPI.
> 
I don't get the point with the UAPI. What will be broken?
I don't see why allowing an attribute with no data is a problem.

Re: [PATCH net] net: enable interface alias removal via rtnl

[David Ahern]

On 10/9/17 9:25 AM, Nicolas Dichtel wrote:
> Le 09/10/2017 à 16:02, David Ahern a écrit :
>> On 10/9/17 2:23 AM, Nicolas Dichtel wrote:
>>> Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :
>>>>
>>>>
>>>> On 10/06/2017 08:18 PM, David Ahern wrote:
>>>>> On 10/5/17 4:19 AM, Nicolas Dichtel wrote:
>>>>>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>>>>>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>>>>>> length must be 0 (see dev_set_alias()).
>>>>>
>>>>> why not add a check in dev_set_alias that if len is 1 and the 1
>>>>> character is '\0' it means remove the alias?
>>> Because it requires an iproute2 patch. iproute2 doesn't send the '\0'. With the
>>> command 'ip link set dummy0 alias ""', the attribute length is 0.
>>
>> iproute2 needs the feature for 0-len strings or perhaps a 'noalias' option.
> iproute2 needs nothing ...
> 
>>
>> You can reset the alias using the sysfs file. Given that there is a
>> workaround for existing kernels and userspace, upstream can get fixed
>> without changing the UAPI.
>>
> I don't get the point with the UAPI. What will be broken?

never mind; I see the error of my ways.

> I don't see why allowing an attribute with no data is a problem.
> 

I remember the problem now. I made a patch back in March 2016 that
adjusted the policy validation to allow 0-length string. I never sent it
and forgot about it until today. You changing ifla_policy to NLA_BINARY
is achieving the same thing.

I think a comment above the policy line is warranted that clarifies
IFLA_IFALIAS is a string but to allow a 0-length string to remove the
alias NLA_BINARY is used for policy validation.

Comparing the validation done for NLA_STRING vs NLA_BINARY it does
change the behavior for 256-character strings.

[PATCH net v2] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
the attribute is 1 ("\0"). However, to remove an alias, the attribute
length must be 0 (see dev_set_alias()).

Let's define the type to NLA_BINARY, so that the alias can be removed.

Example:
$ ip l s dummy0 alias foo
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
    alias foo

Before the patch:
$ ip l s dummy0 alias ""
RTNETLINK answers: Numerical result out of range

After the patch:
$ ip l s dummy0 alias ""
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff

CC: Oliver Hartkopp <oliver@hartkopp.net>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <julien.floret@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v1 -> v2: add the comment

 net/core/rtnetlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4bcdcc68e92..5343565d88b7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1483,7 +1483,10 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
 	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
 	[IFLA_NET_NS_FD]	= { .type = NLA_U32 },
-	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
+	/* IFLA_IFALIAS is a string, but policy is set to NLA_BINARY to
+	 * allow 0-length string (needed to remove an alias).
+	 */
+	[IFLA_IFALIAS]	        = { .type = NLA_BINARY, .len = IFALIASZ - 1 },
 	[IFLA_VFINFO_LIST]	= {. type = NLA_NESTED },
 	[IFLA_VF_PORTS]		= { .type = NLA_NESTED },
 	[IFLA_PORT_SELF]	= { .type = NLA_NESTED },
-- 
2.13.2

Re: [PATCH net v2] net: enable interface alias removal via rtnl

[David Ahern]

On 10/10/17 6:41 AM, Nicolas Dichtel wrote:
> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
> the attribute is 1 ("\0"). However, to remove an alias, the attribute
> length must be 0 (see dev_set_alias()).
> 
> Let's define the type to NLA_BINARY, so that the alias can be removed.

not to be pedantic, but we need to be clear that the type is changed
only for policy validation.

> 
> Example:
> $ ip l s dummy0 alias foo
> $ ip l l dev dummy0
> 5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
>     alias foo
> 
> Before the patch:
> $ ip l s dummy0 alias ""
> RTNETLINK answers: Numerical result out of range
> 
> After the patch:
> $ ip l s dummy0 alias ""
> $ ip l l dev dummy0
> 5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
> 
> CC: Oliver Hartkopp <oliver@hartkopp.net>
> CC: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
> Reported-by: Julien FLoret <julien.floret@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> v1 -> v2: add the comment
> 
>  net/core/rtnetlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index d4bcdcc68e92..5343565d88b7 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1483,7 +1483,10 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>  	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
>  	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
>  	[IFLA_NET_NS_FD]	= { .type = NLA_U32 },
> -	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
> +	/* IFLA_IFALIAS is a string, but policy is set to NLA_BINARY to
> +	 * allow 0-length string (needed to remove an alias).
> +	 */
> +	[IFLA_IFALIAS]	        = { .type = NLA_BINARY, .len = IFALIASZ - 1 },
>  	[IFLA_VFINFO_LIST]	= {. type = NLA_NESTED },
>  	[IFLA_VF_PORTS]		= { .type = NLA_NESTED },
>  	[IFLA_PORT_SELF]	= { .type = NLA_NESTED },
> 

Seems like a reasonable solution.

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

Re: [PATCH net v2] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

Le 10/10/2017 à 16:50, David Ahern a écrit :
> On 10/10/17 6:41 AM, Nicolas Dichtel wrote:
>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>> length must be 0 (see dev_set_alias()).
>>
>> Let's define the type to NLA_BINARY, so that the alias can be removed.
> 
> not to be pedantic, but we need to be clear that the type is changed
> only for policy validation.
With the comment in the code, it is clear, isn't it?


Regards,
Nicolas

Re: [PATCH net v2] net: enable interface alias removal via rtnl

[David Ahern]

On 10/11/17 6:29 AM, Nicolas Dichtel wrote:
> Le 10/10/2017 à 16:50, David Ahern a écrit :
>> On 10/10/17 6:41 AM, Nicolas Dichtel wrote:
>>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>>> length must be 0 (see dev_set_alias()).
>>>
>>> Let's define the type to NLA_BINARY, so that the alias can be removed.
>>
>> not to be pedantic, but we need to be clear that the type is changed
>> only for policy validation.
> With the comment in the code, it is clear, isn't it?

Code comment was fine; commit log -- line referenced above -- is open
for interpretation.

[PATCH net v3] net: enable interface alias removal via rtnl

[Nicolas Dichtel]

IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
the attribute is 1 ("\0"). However, to remove an alias, the attribute
length must be 0 (see dev_set_alias()).

Let's define the type to NLA_BINARY to allow 0-length string, so that the
alias can be removed.

Example:
$ ip l s dummy0 alias foo
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
    alias foo

Before the patch:
$ ip l s dummy0 alias ""
RTNETLINK answers: Numerical result out of range

After the patch:
$ ip l s dummy0 alias ""
$ ip l l dev dummy0
5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff

CC: Oliver Hartkopp <oliver@hartkopp.net>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <julien.floret@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

David A., I hope that it is now clear and that a v4 will not be needed
for a so trivial patch.

v2 -> v3: reword the commit log

v1 -> v2: add the comment

 net/core/rtnetlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4bcdcc68e92..5343565d88b7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1483,7 +1483,10 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINKINFO]		= { .type = NLA_NESTED },
 	[IFLA_NET_NS_PID]	= { .type = NLA_U32 },
 	[IFLA_NET_NS_FD]	= { .type = NLA_U32 },
-	[IFLA_IFALIAS]	        = { .type = NLA_STRING, .len = IFALIASZ-1 },
+	/* IFLA_IFALIAS is a string, but policy is set to NLA_BINARY to
+	 * allow 0-length string (needed to remove an alias).
+	 */
+	[IFLA_IFALIAS]	        = { .type = NLA_BINARY, .len = IFALIASZ - 1 },
 	[IFLA_VFINFO_LIST]	= {. type = NLA_NESTED },
 	[IFLA_VF_PORTS]		= { .type = NLA_NESTED },
 	[IFLA_PORT_SELF]	= { .type = NLA_NESTED },
-- 
2.13.2

Re: [PATCH net v3] net: enable interface alias removal via rtnl

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 11 Oct 2017 16:24:48 +0200

> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
> the attribute is 1 ("\0"). However, to remove an alias, the attribute
> length must be 0 (see dev_set_alias()).
> 
> Let's define the type to NLA_BINARY to allow 0-length string, so that the
> alias can be removed.
> 
> Example:
> $ ip l s dummy0 alias foo
> $ ip l l dev dummy0
> 5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
>     alias foo
> 
> Before the patch:
> $ ip l s dummy0 alias ""
> RTNETLINK answers: Numerical result out of range
> 
> After the patch:
> $ ip l s dummy0 alias ""
> $ ip l l dev dummy0
> 5: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether ae:20:30:4f:a7:f3 brd ff:ff:ff:ff:ff:ff
> 
> CC: Oliver Hartkopp <oliver@hartkopp.net>
> CC: Stephen Hemminger <stephen@networkplumber.org>
> Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
> Reported-by: Julien FLoret <julien.floret@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied, thank you.