[iproute2-next] ipaddress: fix label matching

Message ID 20180711113603.16589-1-vincent@bernat.im
State Superseded
Delegated to: David Ahern
Headers show
Series
  • [iproute2-next] ipaddress: fix label matching
Related show

Commit Message

Vincent Bernat July 11, 2018, 11:36 a.m.
Since 9516823051ce, "ip addr show label lo:1" doesn't work
anymore (doesn't show any address, despite a matching label).
Reverting to return 0 instead of -1 fix the issue.

However, the condition says: "if we filter by label [...] and the
label does NOT match the interface name". This makes little sense to
compare the label with the interface name. There is also a logic
around filter family being provided or not. The match against the
label is done by ifa_label_match_rta() in print_addrinfo() and
ipaddr_filter().

Just removing the condition makes "ip addr show" works as expected
with or without specifying a label, both when the label is matching
and not matching. It also works if we specify a label and the label is
the interface name. The flush operation also works as expected.

Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()")
Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
 ip/ipaddress.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Stephen Hemminger July 11, 2018, 8:03 p.m. | #1
On Wed, 11 Jul 2018 13:36:03 +0200
Vincent Bernat <vincent@bernat.im> wrote:

> Since 9516823051ce, "ip addr show label lo:1" doesn't work
> anymore (doesn't show any address, despite a matching label).
> Reverting to return 0 instead of -1 fix the issue.
> 
> However, the condition says: "if we filter by label [...] and the
> label does NOT match the interface name". This makes little sense to
> compare the label with the interface name. There is also a logic
> around filter family being provided or not. The match against the
> label is done by ifa_label_match_rta() in print_addrinfo() and
> ipaddr_filter().
> 
> Just removing the condition makes "ip addr show" works as expected
> with or without specifying a label, both when the label is matching
> and not matching. It also works if we specify a label and the label is
> the interface name. The flush operation also works as expected.
> 
> Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()")
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
> ---
>  ip/ipaddress.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 5009bfe6d2e3..20ef6724944e 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>  	if (!name)
>  		return -1;
>  
> -	if (filter.label &&
> -	    (!filter.family || filter.family == AF_PACKET) &&
> -	    fnmatch(filter.label, name, 0))
> -		return -1;
> -
>  	if (tb[IFLA_GROUP]) {
>  		int group = rta_getattr_u32(tb[IFLA_GROUP]);
> 

If this is a regression, it should go to iproute2 not iproute2-next.

Surprised by the solution since it is removing code that was there
before the commit you referenced in Fixes.
Vincent Bernat July 11, 2018, 8:26 p.m. | #2
❦ 11 juillet 2018 13:03 -0700, Stephen Hemminger <stephen@networkplumber.org> :

>> Since 9516823051ce, "ip addr show label lo:1" doesn't work
>> anymore (doesn't show any address, despite a matching label).
>> Reverting to return 0 instead of -1 fix the issue.
>> 
>> However, the condition says: "if we filter by label [...] and the
>> label does NOT match the interface name". This makes little sense to
>> compare the label with the interface name. There is also a logic
>> around filter family being provided or not. The match against the
>> label is done by ifa_label_match_rta() in print_addrinfo() and
>> ipaddr_filter().
>> 
>> Just removing the condition makes "ip addr show" works as expected
>> with or without specifying a label, both when the label is matching
>> and not matching. It also works if we specify a label and the label is
>> the interface name. The flush operation also works as expected.
>> 
>> Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()")
>> Signed-off-by: Vincent Bernat <vincent@bernat.im>
>> ---
>>  ip/ipaddress.c | 5 -----
>>  1 file changed, 5 deletions(-)
>> 
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 5009bfe6d2e3..20ef6724944e 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>  	if (!name)
>>  		return -1;
>>  
>> -	if (filter.label &&
>> -	    (!filter.family || filter.family == AF_PACKET) &&
>> -	    fnmatch(filter.label, name, 0))
>> -		return -1;
>> -
>>  	if (tb[IFLA_GROUP]) {
>>  		int group = rta_getattr_u32(tb[IFLA_GROUP]);
>> 
>
> If this is a regression, it should go to iproute2 not iproute2-next.
>
> Surprised by the solution since it is removing code that was there
> before the commit you referenced in Fixes.

Yes, but as I explain in the commit message, the condition does not make
sense for me: why would we match the label against the interface name?
This code exists since a long time.
David Ahern July 12, 2018, 1:01 a.m. | #3
On 7/11/18 7:36 AM, Vincent Bernat wrote:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 5009bfe6d2e3..20ef6724944e 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>  	if (!name)
>  		return -1;
>  
> -	if (filter.label &&
> -	    (!filter.family || filter.family == AF_PACKET) &&
> -	    fnmatch(filter.label, name, 0))
> -		return -1;
> -

The offending commit changed the return code:

        if (filter.label &&
            (!filter.family || filter.family == AF_PACKET) &&
-           fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
-               return 0;
+           fnmatch(filter.label, name, 0))
+               return -1;


Vincent: can you try leaving the code as is, but change the return to 0?
Vincent Bernat July 12, 2018, 5:08 a.m. | #4
❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> :

>> +++ b/ip/ipaddress.c
>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>  	if (!name)
>>  		return -1;
>>  
>> -	if (filter.label &&
>> -	    (!filter.family || filter.family == AF_PACKET) &&
>> -	    fnmatch(filter.label, name, 0))
>> -		return -1;
>> -
>
> The offending commit changed the return code:
>
>         if (filter.label &&
>             (!filter.family || filter.family == AF_PACKET) &&
> -           fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
> -               return 0;
> +           fnmatch(filter.label, name, 0))
> +               return -1;
>
>
> Vincent: can you try leaving the code as is, but change the return to 0?

Yes, it works by just returning 0. The code still doesn't make sense.
Serhey Popovych July 14, 2018, 8:27 a.m. | #5
Vincent Bernat wrote:
>  ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> :
> 
>>> +++ b/ip/ipaddress.c
>>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>>  	if (!name)
>>>  		return -1;
>>>  
>>> -	if (filter.label &&
>>> -	    (!filter.family || filter.family == AF_PACKET) &&
>>> -	    fnmatch(filter.label, name, 0))
>>> -		return -1;
>>> -
>>
>> The offending commit changed the return code:
>>
>>         if (filter.label &&
>>             (!filter.family || filter.family == AF_PACKET) &&
>> -           fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
>> -               return 0;
>> +           fnmatch(filter.label, name, 0))
>> +               return -1;
>>
>>
>> Vincent: can you try leaving the code as is, but change the return to 0?
> 
> Yes, it works by just returning 0. The code still doesn't make sense.
> 

I think return code is correct. Check presented by this code too because
print_linkinfo() isn't static and called from ipmonitor.c where no
ipaddr_filter() or similar call that filters by label present.

Instead fnmatch() compares interface *name*, not label from IFA_LABEL
attribute. Thus:

    fnmatch(pattern, string, flags) ->
    fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1)

Assuming above I would like to see ifa_label_match_rta() instead of open
coded checks for filter.label with fmatch() in print_linkinfo().

Also it might be good idea to pass @name from get_ifname_rta() (like we
do in print_linkinfo()) to ifa_label_match_rta() so that we respect
IFLA_IFNAME if present.
Serhey Popovych July 14, 2018, 6:54 p.m. | #6
Serhey Popovych wrote:
> Vincent Bernat wrote:
>>  ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> :
>>
>>>> +++ b/ip/ipaddress.c
>>>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>>>  	if (!name)
>>>>  		return -1;
>>>>  
>>>> -	if (filter.label &&
>>>> -	    (!filter.family || filter.family == AF_PACKET) &&
>>>> -	    fnmatch(filter.label, name, 0))
>>>> -		return -1;
>>>> -
>>>
>>> The offending commit changed the return code:
>>>
>>>         if (filter.label &&
>>>             (!filter.family || filter.family == AF_PACKET) &&
>>> -           fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
>>> -               return 0;
>>> +           fnmatch(filter.label, name, 0))
>>> +               return -1;
>>>
>>>
>>> Vincent: can you try leaving the code as is, but change the return to 0?
>>
>> Yes, it works by just returning 0. The code still doesn't make sense.
>>
> 
> I think return code is correct. Check presented by this code too because
> print_linkinfo() isn't static and called from ipmonitor.c where no
> ipaddr_filter() or similar call that filters by label present.

Ok, did more deep analysis of code. Vincent, David: we should return 0
as done before 9516823051ce.

This is special case to return from print_linkinfo() earlier and match
only filter.ifindex and filter.up if given and not rest fields in @filter.

Then call print_selected_addrinfo() without calling print_link_stats()
in ipaddr_list_flush_or_save().

Later print_selected_addrinfo() calls print_addrinfo() that finally
matches filter.label using ifa_label_match_rta().

> 
> Instead fnmatch() compares interface *name*, not label from IFA_LABEL
> attribute. Thus:
> 
>     fnmatch(pattern, string, flags) ->
>     fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1)

This still incorrect: we should not call fnmatch() with network device name.

Also ip-link(8) does not say anything that label could be used to
filter link output. Label is ip-address(8) specific. Therefore checking
filter.family == AF_PACKET looks incorrect. AF_PACKET is ip-link(8)
specific.

Checking against !filter.family (AF_UNSPEC) is incorrect too: user might
force address family at ip command line and we never get:

    ip -4 addr show label lo:1

So from the code:

    if (filter.label &&
       (!filter.family || filter.family == AF_PACKET) &&
        fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
            return -1;

We should leave only filter.label check and return 0:

    if (filter.label)
            return 0;

This will ensure we exit from print_linkinfo() earlier, skip
print_link_stats() and push final filtering by label to
print_selected_addrinfo() and print_addrinfo().

And finally: this is regression and should be against iproute2, not -next.

> 
> Assuming above I would like to see ifa_label_match_rta() instead of open
> coded checks for filter.label with fmatch() in print_linkinfo().
> 
> Also it might be good idea to pass @name from get_ifname_rta() (like we
> do in print_linkinfo()) to ifa_label_match_rta() so that we respect
> IFLA_IFNAME if present.
>
Vincent Bernat July 14, 2018, 6:58 p.m. | #7
❦ 14 juillet 2018 21:54 +0300, Serhey Popovych <serhe.popovych@gmail.com> :

> We should leave only filter.label check and return 0:
>
>     if (filter.label)
>             return 0;
>
> This will ensure we exit from print_linkinfo() earlier, skip
> print_link_stats() and push final filtering by label to
> print_selected_addrinfo() and print_addrinfo().
>
> And finally: this is regression and should be against iproute2, not -next.

As you did the analysis, I let you do the patch.

Patch

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5009bfe6d2e3..20ef6724944e 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -837,11 +837,6 @@  int print_linkinfo(const struct sockaddr_nl *who,
 	if (!name)
 		return -1;
 
-	if (filter.label &&
-	    (!filter.family || filter.family == AF_PACKET) &&
-	    fnmatch(filter.label, name, 0))
-		return -1;
-
 	if (tb[IFLA_GROUP]) {
 		int group = rta_getattr_u32(tb[IFLA_GROUP]);