Date: Thu, 7 Dec 2023 18:30:44 -0800
From: Kui-Feng Lee <sinquersw@...il.com>
To: David Ahern <dsahern@...nel.org>, thinker.li@...il.com,
 netdev@...r.kernel.org, martin.lau@...ux.dev, kernel-team@...a.com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Cc: kuifeng@...a.com, syzbot+c15aa445274af8674f41@...kaller.appspotmail.com
Subject: Re: [PATCH net-next] net/ipv6: insert the fib6 gc_link of a fib6_info
 only if in fib6.



On 12/7/23 17:23, David Ahern wrote:
> On 12/7/23 6:16 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/7/23 17:02, David Ahern wrote:
>>> On 12/7/23 4:33 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/7/23 15:20, David Ahern wrote:
>>>>> On 12/7/23 4:17 PM, David Ahern wrote:
>>>>>> On 12/7/23 3:16 PM, thinker.li@...il.com wrote:
>>>>>>> From: Kui-Feng Lee <thinker.li@...il.com>
>>>>>>>
>>>>>>> Check f6i->fib6_node before inserting a f6i (fib6_info) to
>>>>>>> tb6_gc_hlist.
>>>>>>
>>>>>> any place setting expires should know if the entry is in a table or
>>>>>> not.
>>>>>>
>>>>>> And the syzbot report contains a reproducer, a kernel config and other
>>>>>> means to test a patch.
>>>>>>
>>>>>
>>>>> Fundamentally, the set and clear helpers are doing 2 things; they need
>>>>> to be split into separate helpers.
>>>>
>>>> Sorry, I don't follow you.
>>>>
>>>> There are fib6_set_expires_locked()) and fib6_clean_expires_locked(),
>>>> two separate helpers. Is this what you are saying?
>>>>
>>>> Doing checks of f6i->fib6_node in fib6_set_expires_locked() should
>>>> already apply everywhere setting expires, right?
>>>>
>>>> Do I miss anything?
>>>
>>> static inline void fib6_set_expires_locked(struct fib6_info *f6i,
>>>                                              unsigned long expires)
>>> {
>>>           struct fib6_table *tb6;
>>>
>>>           tb6 = f6i->fib6_table;
>>>           f6i->expires = expires;
>>>           if (tb6 && !fib6_has_expires(f6i))
>>>                   hlist_add_head(&f6i->gc_link, &tb6->tb6_gc_hlist);
> 
> --> no check that f6i is already in the list yet fib6_set_expires and
> fib6_set_expires_locked are called on existing entries - entries already
> in the tree and so *maybe* already linked. See fib6_add_rt2node and most
> of fib6_set_expires callers.

Although it bases on a false assumption, checking tb6 ensures the
entry is added to the list only if this f6i is already on the tree.
The correct one should checks f6i->fib6_node.
So, with checking f6i->fib6_ndoe and the open code you mentioned,
fib6_has_expires() does check if a f6i is already in the list.

But, like what you mentioned earlier, hlist_unhashed(&f6i->gc_link) is
clearer. I will move to ti.

> 
> Your selftests only check that entries are removed; it does not check
> updating the expires time on an existing entry. It does not check a
> route replace that toggles between no expires value or to an expires
> (fib6_add_rt2node use of fib6_set_expires_locked) and then replacing
> that route -- various permutations here.
> 
> 
>>>           f6i->fib6_flags |= RTF_EXPIRES;
>>> }
>>>
>>> 1. You are abusing this helper in create_info to set the value of
>>> expires knowing (expecting) tb6 to NOT be set and hence not setting
>>> gc_link so no cleanup is needed on errors.
>>>
>>> 2. You then open code gc_link when adding to the tree.
>>>
>>> I had my reservations when you sent this patch months ago, but I did not
>>> have time to suggest a cleaner approach and now this is where we are --
>>> trying to find some race corrupting the list.
>>
>> So, what you said is to split fib6_set_expires_locked() into two
>> functions. One is setting expires, and the other is adding a f6i to
>> tb6_gc_hlist. fib6_clean_expires_locked() should be split in the same
>> way.
>>
>> Is it correct?
>>
> 
> one helper sets expires value and one helper that adds/removes from the
> gc_link. If it is already linked, then no need to do it again.

Got it!

Powered by blists - more mailing lists