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: <d58cb247-b0ac-4cbd-9eb2-34cc5fc2670f@kernel.org>
Date: Wed, 7 Feb 2024 08:58:04 -0700
From: David Ahern <dsahern@...nel.org>
To: 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: sinquersw@...il.com, kuifeng@...a.com
Subject: Re: [PATCH net-next v4 3/5] net/ipv6: Remove expired routes with a
 separated list of routes.

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.

> + */
> +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);
> +}
> +