Re: [PATCH bpf] bpf: fix rcu lockdep warning for lpm_trie map_free callback
On 2/22/18 5:37 AM, Eric Dumazet wrote:
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:
[<196bc1f0>] process_one_work+0x157/0x5c0
[ 98.301381] #1: ((work_completion)(>work)){+.+.}, at:
[<196bc1f0>] 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
Signed-off-by: Yonghong Song
---
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 = >root;
for (;;) {
- node = rcu_dereference_protected(*slot,
- lockdep_is_held(>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]
*
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();
Emic,
Thanks for the fix suggestion. It does make sense.
I indeed ran sparse before my patch send-email. Unfortunately, my dev
machine (sparse 0.5.0 + gcc 4.8.5) didn't issue warning like the above.
With the same kernel config and kernel tree, I just tried on another
machine (a FC27 VM, sparse 0.5.1 + gcc 7.3.1), I did see the warning and
the above suggested fix makes warning went away.
Need to figure out why sparse is not happy with my dev machine.
Will send a follow patch soon.
Thanks!
Yonghong
Re: [PATCH bpf] bpf: fix rcu lockdep warning for lpm_trie map_free callback
Hi Yonghong, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on bpf/master] url: https://github.com/0day-ci/linux/commits/Yonghong-Song/bpf-fix-rcu-lockdep-warning-for-lpm_trie-map_free-callback/20180222-202658 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) include/linux/init.h:134:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:135:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:268:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/init.h:269:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/printk.h:200:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:32:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:34:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:37:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:38:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:40:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:42:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:43:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:45:5: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:46:5: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/mem_encrypt.h:49:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/qspinlock.h:53:32: sparse: attribute 'indirect_branch': unknown attribute include/linux/workqueue.h:646:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/workqueue.h:647:5: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:34:12: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:35:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/numa.h:62:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:64:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:173:8: sparse: attribute 'indirect_branch': unknown attribute include/linux/vmalloc.h:174:8: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:174:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:176:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:178:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/fixmap.h:180:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/apic.h:254:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/apic.h:430:13: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/io_apic.h:184:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:113:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:125:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/smp.h:126:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:110:33: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:112:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:114:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:118:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/percpu.h:126:12: sparse: attribute 'indirect_branch': unknown attribute include/linux/memory_hotplug.h:221:13: sparse: attribute 'indirect_branch': unknown attribute include/linux/mmzone.h:1292:15: sparse: attribute 'indirect_branch': unknown attribute include/linux/kmemleak.h:29:33: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/kasan.h:29:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/kasan.h:30:6: sparse: attribute 'indirect_branch': unknown attribute arch/x86/include/asm/pgtable.h:28:5: sparse: attribute 'indirect_branch': unknown attribute include/linux/slab.h:135:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/slab.h:716:6: sparse: attribute 'indirect_branch': unknown attribute include/linux/wait_bit.h:41:13: sparse: attribute 'indirect_branch': unknown attribute
Re: [PATCH bpf] bpf: fix rcu lockdep warning for lpm_trie map_free callback
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:
> [<196bc1f0>] process_one_work+0x157/0x5c0
> [ 98.301381] #1: ((work_completion)(>work)){+.+.}, at:
> [<196bc1f0>] 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
> Signed-off-by: Yonghong Song
> ---
> 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 = >root;
>
> for (;;) {
> - node = rcu_dereference_protected(*slot,
> - lockdep_is_held(>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]
*
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();
