From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:35646 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbeBVDkS (ORCPT ); Wed, 21 Feb 2018 22:40:18 -0500 Received: by mail-pf0-f195.google.com with SMTP id y186so901626pfb.2 for ; Wed, 21 Feb 2018 19:40:18 -0800 (PST) Message-ID: <1519270814.55655.48.camel@gmail.com> Subject: Re: [PATCH bpf v2] bpf: fix memory leak in lpm_trie map_free callback function From: Eric Dumazet To: Alexei Starovoitov , Yonghong Song Cc: ast@fb.com, daniel@iogearbox.net, netdev@vger.kernel.org, kernel-team@fb.com Date: Wed, 21 Feb 2018 19:40:14 -0800 In-Reply-To: <20180214031719.vbq6yvgtv76sgnvg@ast-mbp.dhcp.thefacebook.com> References: <20180214030021.1927189-1-yhs@fb.com> <20180214031719.vbq6yvgtv76sgnvg@ast-mbp.dhcp.thefacebook.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On Tue, 2018-02-13 at 19:17 -0800, Alexei Starovoitov wrote: > On Tue, Feb 13, 2018 at 07:00:21PM -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 | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > v1->v2: > > Make comments more precise and make label name more appropriate, > > as suggested by Daniel > > Applied to bpf tree, Thanks Yonghong. This does not look good. LOCKDEP surely should complain to node = rcu_dereference_protected(*slot, lockdep_is_held(&trie->lock)); Since we no longer hold trie->lock