| Message ID | 20171005101940.28550-1-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: enable interface alias removal via rtnl | expand |
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: >> 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 >
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.
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.
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.
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.
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 },
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(-)