netdev - Re: [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in 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]

Message-ID: <3cea4a80-09c0-4a58-87e9-25073b4e9c9f@gmail.com>
Date: Fri, 8 Dec 2023 15:38:52 -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 v2 1/2] net/ipv6: insert a f6i to a GC list only
 if the f6i is in a fib6_table tree.

On 12/8/23 15:19, Kui-Feng Lee wrote:
> 
> 
> On 12/8/23 14:43, David Ahern wrote:
>> On 12/8/23 12:45 PM, thinker.li@...il.com wrote:
>>> From: Kui-Feng Lee <thinker.li@...il.com>
>>>
>>> Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before 
>>> inserting a
>>> f6i (fib6_info) to tb6_gc_hlist.
>>>
>>> The current implementation checks if f6i->fib6_table is not NULL to
>>> determines if a f6i is on a tree, however it is not enough. When a 
>>> f6i is
>>> removed from a fib6_table, f6i->fib6_table is not reset. However, 
>>> fib6_node
>>> is always reset when a f6i is removed from a fib6_table and is set 
>>> when a
>>> f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
>>> determine if a f6i is on a tree.
>>
>> Which is an indication that the table is not the right check but neither
>> is the fib6_node. If expires is set on a route entry, add it to the
>> gc_list; if expires is reset on a route entry, remove it from the
>> gc_list. If the value of expires is changed while on the gc_list list,
>> just update the expires value.
>>
> 
> I don't quite follow you.
> If an entry is not on a tree, why do we still add the entry to the gc 
> list? (This is the reason to check f6i->fib6_node.)
> 
> The changes in this patch rely on two indications, 1. if a f6i is on a 
> tree, 2. if a f6i is on a gc list (described in the 3rd paragraph of
> the comment log.)
> An entry is added to a gc list only if it has expires and is not on the 
> list yet. An entry is removed from a gc list only if it is on a gc list.
> And, just like what you said earlier, it just updates the expires value
> while an entry is already on a gc list.

Add more context here.

Use rt6_route_rcv() as an example. It looks up or adds a route first.
Then it changes expires of the route at the very end of the function.
There is gap between looking up and the modification. The f6i found here
can be removed from the tree in-between. If the f6i here is added to the
gc list even it has been removed from the tree, fib6_info_release() at 
the end of rt6_route_rcv() might free the f6i while the f6i is still in
the gc list.

This is why it checks if a f6i is on the tree with the protection of
tb6_lock.