netdev - Re: [PATCH net-next] net/ipv6: insert the fib6 gc_link of a fib6

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 for Android: free password hash cracker in your pocket

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

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?