[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b0fe3183-4df9-42bc-84e4-ba8e807318f4@gmail.com>
Date: Thu, 7 Dec 2023 17:16:47 -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: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);
> 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?
Powered by blists - more mailing lists