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

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 <redacted>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <redacted>
Signed-off-by: Nicolas Dichtel <redacted>
---
 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

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

On 10/06/2017 08:18 PM, David Ahern wrote:

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

quoted

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

quoted

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

that changes the uapi

Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :

On 10/06/2017 08:18 PM, David Ahern wrote:

quoted

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

quoted

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

quoted

quoted

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.

On 10/9/17 2:23 AM, Nicolas Dichtel wrote:

Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :

quoted

On 10/06/2017 08:18 PM, David Ahern wrote:

quoted

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

quoted

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.

Le 09/10/2017 à 16:02, David Ahern a écrit :

On 10/9/17 2:23 AM, Nicolas Dichtel wrote:

quoted

Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :

quoted

On 10/06/2017 08:18 PM, David Ahern wrote:

quoted

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

quoted

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.

On 10/9/17 9:25 AM, Nicolas Dichtel wrote:

Le 09/10/2017 à 16:02, David Ahern a écrit :

quoted

On 10/9/17 2:23 AM, Nicolas Dichtel wrote:

quoted

Le 06/10/2017 à 22:10, Oliver Hartkopp a écrit :

quoted

On 10/06/2017 08:18 PM, David Ahern wrote:

quoted

On 10/5/17 4:19 AM, Nicolas Dichtel wrote:

quoted

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 ...

quoted

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.

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 <redacted>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <redacted>
Signed-off-by: Nicolas Dichtel <redacted>
---

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

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.

Seems like a reasonable solution.

Acked-by: David Ahern <redacted>

Le 10/10/2017 à 16:50, David Ahern a écrit :

On 10/10/17 6:41 AM, Nicolas Dichtel wrote:

quoted

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

On 10/11/17 6:29 AM, Nicolas Dichtel wrote:

Le 10/10/2017 à 16:50, David Ahern a écrit :

quoted

On 10/10/17 6:41 AM, Nicolas Dichtel wrote:

quoted

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.

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 <redacted>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <redacted>
Signed-off-by: Nicolas Dichtel <redacted>
---

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

From: Nicolas Dichtel <redacted>
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 <redacted>
CC: Stephen Hemminger <stephen@networkplumber.org>
Fixes: 96ca4a2cc145 ("net: remove ifalias on empty given alias")
Reported-by: Julien FLoret <redacted>
Signed-off-by: Nicolas Dichtel <redacted>

Applied, thank you.