From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:36376 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933447AbeBVRuh (ORCPT ); Thu, 22 Feb 2018 12:50:37 -0500 Subject: Re: [PATCH bpf] bpf: fix rcu lockdep warning for lpm_trie map_free callback To: Eric Dumazet , , , , CC: References: <20180222063818.3215104-1-yhs@fb.com> <1519306651.55655.52.camel@gmail.com> From: Yonghong Song Message-ID: Date: Thu, 22 Feb 2018 09:49:39 -0800 MIME-Version: 1.0 In-Reply-To: <1519306651.55655.52.camel@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org List-ID: 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: [<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 >> 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 = &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] * > > > 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 > >