netdev - Re: [PATCH bpf] bpf: fix memory leak in lpm_trie map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9174898-fe82-0088-3b5d-cf8e5daa829b@iogearbox.net>
Date:   Wed, 14 Feb 2018 02:11:57 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>, ast@...com, malat@...ian.org,
        netdev@...r.kernel.org
Cc:     kernel-team@...com
Subject: Re: [PATCH bpf] bpf: fix memory leak in lpm_trie map_free callback
 function

Hi Yonghong,

On 02/12/2018 10:58 PM, 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 <malat@...ian.org>
> Reported-by: Alexei Starovoitov <ast@...nel.org>
> Tested-by: Mathieu Malaterre <malat@...ian.org>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  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.
> +	 */

I think the first part of the comment is slightly misleading, e.g. map refcount
could drop to zero also for various other reasons, not strictly due to prog
refcnt dropping to 0, so I would just keep the second part with 'Wait for
outstanding [...]' which is okay since this is eventually what is relevant in
this context.

> +	synchronize_rcu();
>  
>  	/* 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);

Could you do a minor change here: since we get rid of the locking as there's
no user left anymore after grace period, it would be great if you could also
change the 'unlock' label name above.

Other than that, good to go, thanks!

> +	kfree(trie);
>  }
>  
>  static int trie_get_next_key(struct bpf_map *map, void *_key, void *_next_key)
> 

Thanks,
Daniel