[PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[zerons]

[ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]

In bpf/syscall.c, bpf_map_get_fd_by_id() use bpf_map_inc_not_zero() to increase
the refcount, both map->refcnt and map->usercnt. Then, if bpf_map_new_fd() fails,
should handle map->usercnt too.

Signed-off-by: zerons <sironhide0null@gmail.com>
---
 kernel/bpf/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cf5040f..db1ed12 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1966,7 +1966,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)

 	fd = bpf_map_new_fd(map, f_flags);
 	if (fd < 0)
-		bpf_map_put(map);
+		bpf_map_put_with_uref(map);

 	return fd;
 }
--
2.7.4

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[Daniel Borkmann]

On 02/26/2019 03:15 PM, zerons wrote:
> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]

Thanks for the fix! What do you mean by "upstream commit" above in this context?

> In bpf/syscall.c, bpf_map_get_fd_by_id() use bpf_map_inc_not_zero() to increase
> the refcount, both map->refcnt and map->usercnt. Then, if bpf_map_new_fd() fails,
> should handle map->usercnt too.
> 
> Signed-off-by: zerons <sironhide0null@gmail.com>
> ---
>  kernel/bpf/syscall.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index cf5040f..db1ed12 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1966,7 +1966,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
> 
>  	fd = bpf_map_new_fd(map, f_flags);
>  	if (fd < 0)
> -		bpf_map_put(map);
> +		bpf_map_put_with_uref(map);
> 
>  	return fd;
>  }
> --
> 2.7.4
> 

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[zerons]

On 2/26/19 22:44, Daniel Borkmann wrote:
> On 02/26/2019 03:15 PM, zerons wrote:
>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
> 
> Thanks for the fix! What do you mean by "upstream commit" above in this context?
> 

This patch is based on that commit, I thought I should mention this.
Sorry for the confusion.

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[Daniel Borkmann]

On 02/26/2019 03:58 PM, zerons wrote:
> On 2/26/19 22:44, Daniel Borkmann wrote:
>> On 02/26/2019 03:15 PM, zerons wrote:
>>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
>>
>> Thanks for the fix! What do you mean by "upstream commit" above in this context?
> 
> This patch is based on that commit, I thought I should mention this.
> Sorry for the confusion.

Ah, no. Usually that line is placed into stable commits that are
cherry-picked to point to the original commit sha in the mainline
tree. In any case, I didn't find c91951f15978, hence my question.
Anyway, for normal patch submissions in future, please don't add
it into the log, we'll remove it this time.

Thanks,
Daniel

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[Martin Lau]

On Tue, Feb 26, 2019 at 10:15:37PM +0800, zerons wrote:
> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
> 
> In bpf/syscall.c, bpf_map_get_fd_by_id() use bpf_map_inc_not_zero() to increase
> the refcount, both map->refcnt and map->usercnt. Then, if bpf_map_new_fd() fails,
> should handle map->usercnt too.
Good catch!  Thanks for the fix.

Fixes: bd5f5f4ecb78 ("bpf: Add BPF_MAP_GET_FD_BY_ID")
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[Daniel Borkmann]

On 02/26/2019 06:33 PM, Martin Lau wrote:
> On Tue, Feb 26, 2019 at 10:15:37PM +0800, zerons wrote:
>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
>>
>> In bpf/syscall.c, bpf_map_get_fd_by_id() use bpf_map_inc_not_zero() to increase
>> the refcount, both map->refcnt and map->usercnt. Then, if bpf_map_new_fd() fails,
>> should handle map->usercnt too.
> Good catch!  Thanks for the fix.

+1

> Fixes: bd5f5f4ecb78 ("bpf: Add BPF_MAP_GET_FD_BY_ID")
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Applied, thanks!

Re: [PATCH] bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

[zerons]

On 2/27/19 00:24, Daniel Borkmann wrote:
> On 02/26/2019 03:58 PM, zerons wrote:
>> On 2/26/19 22:44, Daniel Borkmann wrote:
>>> On 02/26/2019 03:15 PM, zerons wrote:
>>>> [ Upstream commit c91951f15978f1a0c6b65f063d30f7ea7bc6fb42 ]
>>>
>>> Thanks for the fix! What do you mean by "upstream commit" above in this context?
>>
>> This patch is based on that commit, I thought I should mention this.
>> Sorry for the confusion.
> 
> Ah, no. Usually that line is placed into stable commits that are
> cherry-picked to point to the original commit sha in the mainline
> tree. In any case, I didn't find c91951f15978, hence my question.

That is v4.20.12.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v4.20.12&id=c91951f15978f1a0c6b65f063d30f7ea7bc6fb42

> Anyway, for normal patch submissions in future, please don't add
> it into the log, we'll remove it this time.
> 

Thanks for reminding me. I won't do that again.