netdev - Re: [bpf PATCH v2 1/3] bpf: sockmap, map_release does not hold refcnt for pinned maps

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] [day] [month] [year] [list]

Message-ID: <9a9842e4-4775-3fb4-bc5c-fac6f2c94149@iogearbox.net>
Date:   Mon, 23 Apr 2018 23:53:32 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, ast@...nel.org
Cc:     netdev@...r.kernel.org
Subject: Re: [bpf PATCH v2 1/3] bpf: sockmap, map_release does not hold refcnt
 for pinned maps

On 04/23/2018 08:29 PM, John Fastabend wrote:
> Relying on map_release hook to decrement the reference counts when a
> map is removed only works if the map is not being pinned. In the
> pinned case the ref is decremented immediately and the BPF programs
> released. After this BPF programs may not be in-use which is not
> what the user would expect.
> 
> This patch moves the release logic into bpf_map_put_uref() and brings
> sockmap in-line with how a similar case is handled in prog array maps.
> 
> Fixes: 3d9e952697de ("bpf: sockmap, fix leaking maps with attached but not detached progs")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>

Patches look good, but one trivial request below.

[...]
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4ca46df..4b70439 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -257,8 +257,8 @@ static void bpf_map_free_deferred(struct work_struct *work)
>  static void bpf_map_put_uref(struct bpf_map *map)
>  {
>  	if (atomic_dec_and_test(&map->usercnt)) {
> -		if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY)
> -			bpf_fd_array_map_clear(map);
> +		if (map->ops->map_put_uref)
> +			map->ops->map_put_uref(map);

Could you change the callback name into something like 'map_release_uref'?
Naming it 'map_put_uref' is a bit confusing since this is only called when
the uref reference count already dropped to zero, and here we really release
the last reference point to user space. Given this is BPF core infra, would
be nice to still fix this up before applying.

Thanks,
Daniel