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

[Daniel Borkmann <daniel@iogearbox.net>]

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@gmail.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