[PATCH iproute2-next] ipaddress: fix label matching

[PATCH iproute2-next] ipaddress: fix label matching

[Vincent Bernat]

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]);
 
-- 
2.18.0

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Stephen Hemminger]

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.

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Vincent Bernat]

 ❦ 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.
-- 
The lunatic, the lover, and the poet,
Are of imagination all compact...
		-- Wm. Shakespeare, "A Midsummer Night's Dream"

Re: [PATCH iproute2-next] ipaddress: fix label matching

[David Ahern]

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?

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Vincent Bernat]

 ❦ 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.
-- 
Many pages make a thick book, except for pocket Bibles which are on very
very thin paper.

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Serhey Popovych]

[-- Attachment #1.1: Type: text/plain, Size: 1605 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Serhey Popovych]

[-- Attachment #1.1: Type: text/plain, Size: 3198 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH iproute2-next] ipaddress: fix label matching

[Vincent Bernat]

 ❦ 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.
-- 
Watch out for off-by-one errors.
            - The Elements of Programming Style (Kernighan & Plauger)