netdev - Re: [PATCH net] ipv6: avoid lockdep issue in 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <11b1b766-5eb8-4306-605c-a423fc3e1544@gmail.com>
Date:   Tue, 8 Sep 2020 11:56:06 -0600
From:   David Ahern <dsahern@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net] ipv6: avoid lockdep issue in fib6_del()

On 9/8/20 2:20 AM, Eric Dumazet wrote:
> syzbot reported twice a lockdep issue in fib6_del() [1]
> which I think is caused by net->ipv6.fib6_null_entry
> having a NULL fib6_table pointer.
> 
> fib6_del() already checks for fib6_null_entry special
> case, we only need to return earlier.
> 
> Bug seems to occur very rarely, I have thus chosen
> a 'bug origin' that makes backports not too complex.

Make sense.
> 
> [1]
...
> 
> Fixes: 421842edeaf6 ("net/ipv6: Add fib6_null_entry")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: David Ahern <dsahern@...il.com>
> ---
>  net/ipv6/ip6_fib.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 25a90f3f705c7e6d53615f490f36c5722f3bd8b1..4a664ad4f4d4bb2b521f67e8433a06c77bd301ee 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1993,14 +1993,19 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
>  /* Need to own table->tb6_lock */
>  int fib6_del(struct fib6_info *rt, struct nl_info *info)
>  {
> -	struct fib6_node *fn = rcu_dereference_protected(rt->fib6_node,
> -				    lockdep_is_held(&rt->fib6_table->tb6_lock));
> -	struct fib6_table *table = rt->fib6_table;
>  	struct net *net = info->nl_net;
>  	struct fib6_info __rcu **rtp;
>  	struct fib6_info __rcu **rtp_next;
> +	struct fib6_table *table;
> +	struct fib6_node *fn;
>  
> -	if (!fn || rt == net->ipv6.fib6_null_entry)
> +	if (rt == net->ipv6.fib6_null_entry)
> +		return -ENOENT;
> +
> +	table = rt->fib6_table;
> +	fn = rcu_dereference_protected(rt->fib6_node,
> +				       lockdep_is_held(&table->tb6_lock));
> +	if (!fn)
>  		return -ENOENT;
>  
>  	WARN_ON(!(fn->fn_flags & RTN_RTINFO));
> 

seems like a reasonable refactoring for the noted problem.

Reviewed-by: David Ahern <dsahern@...il.com>