From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH bpf] bpf: fix memory leak in lpm_trie map_free callback function Date: Mon, 12 Feb 2018 14:15:55 -0800 Message-ID: <1518473755.3715.166.camel@gmail.com> References: <20180212215802.1827544-1-yhs@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: kernel-team@fb.com To: Yonghong Song , ast@fb.com, daniel@iogearbox.net, malat@debian.org, netdev@vger.kernel.org Return-path: Received: from mail-pg0-f68.google.com ([74.125.83.68]:44477 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932556AbeBLWP7 (ORCPT ); Mon, 12 Feb 2018 17:15:59 -0500 Received: by mail-pg0-f68.google.com with SMTP id j9so7879312pgp.11 for ; Mon, 12 Feb 2018 14:15:58 -0800 (PST) In-Reply-To: <20180212215802.1827544-1-yhs@fb.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 2018-02-12 at 13:58 -0800, Yonghong Song wrote: > There is a memory leak happening in lpm_trie map_free callback > function trie_free. The trie structure itself does not get freed. > > Also, trie_free function did not do synchronize_rcu before freeing > various data structures. This is incorrect as some rcu_read_lock > region(s) for lookup, update, delete or get_next_key may not complete yet. > The fix is to add synchronize_rcu in the beginning of trie_free. > The useless spin_lock is removed from this function as well. > > Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation") > Reported-by: Mathieu Malaterre > Reported-by: Alexei Starovoitov > Tested-by: Mathieu Malaterre > Signed-off-by: Yonghong Song > --- > kernel/bpf/lpm_trie.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c > index 7b469d1..9b41ea4 100644 > --- a/kernel/bpf/lpm_trie.c > +++ b/kernel/bpf/lpm_trie.c > @@ -555,7 +555,12 @@ static void trie_free(struct bpf_map *map) > struct lpm_trie_node __rcu **slot; > struct lpm_trie_node *node; > > - raw_spin_lock(&trie->lock); > + /* at this point bpf_prog->aux->refcnt == 0 and this map->refcnt == 0, > + * so the programs (can be more than one that used this map) were > + * disconnected from events. Wait for outstanding programs to complete > + * update/lookup/delete/get_next_key and free the trie. > + */ > + synchronize_rcu(); > Please do not do that. Use kfree_rcu() instead (adding one struct rcu_head in struct lpm_trie) > /* Always start at the root and walk down to a node that has no > * children. Then free that node, nullify its reference in the parent > @@ -588,7 +593,7 @@ static void trie_free(struct bpf_map *map) > } > > unlock: > - raw_spin_unlock(&trie->lock); > + kfree(trie); > } > > static int trie_get_next_key(struct bpf_map *map, void *_key, void *_next_key)