netdev - Re: [PATCH net-next v4 3/5] net/ipv6: Remove expired routes with a separated list of routes.

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: <74bfcc06-3fcf-49df-bd61-1439806380af@gmail.com>
Date: Wed, 7 Feb 2024 08:00:29 -0800
From: Kui-Feng Lee <sinquersw@...il.com>
To: David Ahern <dsahern@...nel.org>, thinker.li@...il.com,
 netdev@...r.kernel.org, ast@...nel.org, martin.lau@...ux.dev,
 kernel-team@...a.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, liuhangbin@...il.com
Cc: kuifeng@...a.com
Subject: Re: [PATCH net-next v4 3/5] net/ipv6: Remove expired routes with a
 separated list of routes.



On 2/7/24 07:58, David Ahern wrote:
> On 2/5/24 2:40 PM, thinker.li@...il.com wrote:
>> From: Kui-Feng Lee <thinker.li@...il.com>
>>
>> FIB6 GC walks trees of fib6_tables to remove expired routes. Walking a tree
>> can be expensive if the number of routes in a table is big, even if most of
>> them are permanent. Checking routes in a separated list of routes having
>> expiration will avoid this potential issue.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@...il.com>
>> ---
>>   include/net/ip6_fib.h | 47 ++++++++++++++++++++++++++++++++-
>>   net/ipv6/addrconf.c   | 41 ++++++++++++++++++++++++-----
>>   net/ipv6/ip6_fib.c    | 60 +++++++++++++++++++++++++++++++++++++++----
>>   net/ipv6/ndisc.c      | 10 +++++++-
>>   net/ipv6/route.c      | 13 ++++++++--
>>   5 files changed, 155 insertions(+), 16 deletions(-)
>>
> 
> one nit below, but otherwise
> 
> Reviewed-by: David Ahern <dsahern@...nel.org>
> 
> 
>> @@ -498,6 +510,39 @@ void fib6_gc_cleanup(void);
>>   
>>   int fib6_init(void);
>>   
>> +/* Add the route to the gc list if it is not already there
>> + *
>> + * The callers should hold f6i->fib6_table->tb6_lock and make sure the
>> + * route is on a table.
> 
> The last comment is not correct given the fib6_node check below.
> 


Right! I will correct it.

>> + */
>> +static inline void fib6_add_gc_list(struct fib6_info *f6i)
>> +{
>> +	/* If fib6_node is null, the f6i is not in (or removed from) the
>> +	 * table.
>> +	 *
>> +	 * There is a gap between finding the f6i from the table and
>> +	 * calling this function without the protection of the tb6_lock.
>> +	 * This check makes sure the f6i is not added to the gc list when
>> +	 * it is not on the table.
>> +	 */
>> +	if (!rcu_dereference_protected(f6i->fib6_node,
>> +				       lockdep_is_held(&f6i->fib6_table->tb6_lock)))
>> +		return;
>> +
>> +	if (hlist_unhashed(&f6i->gc_link))
>> +		hlist_add_head(&f6i->gc_link, &f6i->fib6_table->tb6_gc_hlist);
>> +}
>> +
>