netdev - Re: [PATCH net 1/2] net: ipv4: fix route with nexthop object delete warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2291d26-8875-22af-b256-35e7bf3054dc@blackwall.org>
Date:   Thu, 31 Mar 2022 22:31:19 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org
Cc:     donaldsharp72@...il.com, philippe.guibert@...look.com,
        kuba@...nel.org, davem@...emloft.net, idosch@...sch.org
Subject: Re: [PATCH net 1/2] net: ipv4: fix route with nexthop object delete
 warning

On 31/03/2022 20:34, Nikolay Aleksandrov wrote:
> On 31 March 2022 20:17:14 EEST, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>> On 31 March 2022 20:05:43 EEST, David Ahern <dsahern@...nel.org> wrote:
>>> On 3/31/22 9:46 AM, Nikolay Aleksandrov wrote:
>>>> FRR folks have hit a kernel warning[1] while deleting routes[2] which is
>>>> caused by trying to delete a route pointing to a nexthop id without
>>>> specifying nhid but matching on an interface. That is, a route is found
>>>> but we hit a warning while matching it. The warning is from
>>>> fib_info_nh() in include/net/nexthop.h because we run it on a fib_info
>>>> with nexthop object. The call chain is:
>>>>  inet_rtm_delroute -> fib_table_delete -> fib_nh_match (called with a
>>>> nexthop fib_info and also with fc_oif set thus calling fib_info_nh on
>>>> the fib_info and triggering the warning). The fix is to not do any
>>>> matching in that branch if the fi has a nexthop object because those are
>>>> managed separately.
>>>>
[snip]
>>>> [2] https://github.com/FRRouting/frr/issues/6412
>>>>
>>>> Fixes: 4c7e8084fd46 ("ipv4: Plumb support for nexthop object in a fib_info")
>>>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
>>>> ---
>>>>  net/ipv4/fib_semantics.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>>>> index cc8e84ef2ae4..ccb62038f6a4 100644
>>>> --- a/net/ipv4/fib_semantics.c
>>>> +++ b/net/ipv4/fib_semantics.c
>>>> @@ -889,8 +889,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi,
>>>>  	}
>>>>  
>>>>  	if (cfg->fc_oif || cfg->fc_gw_family) {
>>>> -		struct fib_nh *nh = fib_info_nh(fi, 0);
>>>> +		struct fib_nh *nh;
>>>> +
>>>> +		/* cannot match on nexthop object attributes */
>>>> +		if (fi->nh)
>>>> +			return 1;
>>>>  
>>>> +		nh = fib_info_nh(fi, 0);
>>>>  		if (cfg->fc_encap) {
>>>>  			if (fib_encap_match(net, cfg->fc_encap_type,
>>>>  					    cfg->fc_encap, nh, cfg, extack))
>>>
>>> I think the right fix is something like this:
>>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>>> index cc8e84ef2ae4..c70775f5e155 100644
>>> --- a/net/ipv4/fib_semantics.c
>>> +++ b/net/ipv4/fib_semantics.c
>>> @@ -886,6 +886,8 @@ int fib_nh_match(struct net *net, struct fib_config
>>> *cfg, struct fib_info *fi,
>>>                if (fi->nh && cfg->fc_nh_id == fi->nh->id)
>>>                        return 0;
>>>                return 1;
>>> +       } else if (fi->nh) {
>>> +               return 1;
>>>        }
>>>
>>> ie., if the cfg has a nexthop id it needs to match fib_info.
>>> if the cfg does not have a nexthop id, but fib_info does then it is not
>>> a match
>>
>>
>> Right, that is also correct and I was tempted to cut it all short in that case
>> but seemed riskier so I went with the more narrow fix for that specific case.
>> Anyway, I'll respin with that change.
>>
>> Cheers,
>> Nik
>>
> 
> Actually I just remembered another reason - ip route del default
> should work regardless of specifying nhid or not. I can't test right now,
> but I suspect that may break if the nh match code is invoked with that change.
> 
> I'll verify it once I'm back in front of a pc.
> 

I just tested it and yes - the change breaks the standard ip route del <addr> case.
I.e.:
$ ip r add 1.2.3.4/32 nhid 12
$ ip r del 1.2.3.4/32
RTNETLINK answers: No such process

Even though:
$ ip r show 1.2.3.4/32
1.2.3.4 nhid 12 via 192.168.11.2 dev dummy0 

Before that change it worked, so I think we should proceed with my fix instead.
The command must not match if there's any nexthop specification after the route
as it doesn't right now, and it must match if there isn't any nh spec.

Cheers,
 Nik