Re: [PATCH bpf] bpf: fix rcu lockdep warning for lpm_trie map_free callback

[Eric Dumazet <eric.dumazet@gmail.com>]

On Wed, 2018-02-21 at 22:38 -0800, Yonghong Song wrote:
> Commit 9a3efb6b661f ("bpf: fix memory leak in lpm_trie map_free callback function")
> fixed a memory leak and removed unnecessary locks in map_free callback function.
> Unfortrunately, it introduced a lockdep warning. When lockdep checking is turned on,
> running tools/testing/selftests/bpf/test_lpm_map will have:
> 
>   [   98.294321] =============================
>   [   98.294807] WARNING: suspicious RCU usage
>   [   98.295359] 4.16.0-rc2+ #193 Not tainted
>   [   98.295907] -----------------------------
>   [   98.296486] /home/yhs/work/bpf/kernel/bpf/lpm_trie.c:572 suspicious rcu_dereference_check() usage!
>   [   98.297657]
>   [   98.297657] other info that might help us debug this:
>   [   98.297657]
>   [   98.298663]
>   [   98.298663] rcu_scheduler_active = 2, debug_locks = 1
>   [   98.299536] 2 locks held by kworker/2:1/54:
>   [   98.300152]  #0:  ((wq_completion)"events"){+.+.}, at: [<00000000196bc1f0>] process_one_work+0x157/0x5c0
>   [   98.301381]  #1:  ((work_completion)(&map->work)){+.+.}, at: [<00000000196bc1f0>] process_one_work+0x157/0x5c0
> 
> Since actual trie tree removal happens only after no other
> accesses to the tree are possible, this patch simply converted all
> rcu protected pointer access to normal access, which removed the
> above warning.
> 
> Fixes: 9a3efb6b661f ("bpf: fix memory leak in lpm_trie map_free callback function")
> Reported-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  kernel/bpf/lpm_trie.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> index a75e02c..0c15813 100644
> --- a/kernel/bpf/lpm_trie.c
> +++ b/kernel/bpf/lpm_trie.c
> @@ -552,7 +552,7 @@ static struct bpf_map *trie_alloc(union bpf_attr *attr)
>  static void trie_free(struct bpf_map *map)
>  {
>  	struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
> -	struct lpm_trie_node __rcu **slot;
> +	struct lpm_trie_node **slot;
>  	struct lpm_trie_node *node;
>  
>  	/* Wait for outstanding programs to complete
> @@ -569,23 +569,22 @@ static void trie_free(struct bpf_map *map)
>  		slot = &trie->root;
>  
>  		for (;;) {
> -			node = rcu_dereference_protected(*slot,
> -					lockdep_is_held(&trie->lock));
> +			node = *slot;

Hi Yonghong

It is not sparse compliant.

kernel/bpf/lpm_trie.c:573:30: warning: incorrect type in assignment (different address spaces)
kernel/bpf/lpm_trie.c:573:30:    expected struct lpm_trie_node *node
kernel/bpf/lpm_trie.c:573:30:    got struct lpm_trie_node [noderef] <asn:4>*<noident>


In my local tree, I simply did

	node = rcu_dereference_protected(*slot, 1);

Since we are the last user of the whole tree after the prior synchronize_rcu();