2019-02-26 14:16:42

by zerons

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

[ 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 <[email protected]>
---
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



2019-02-26 14:45:21

by Daniel Borkmann

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

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 <[email protected]>
> ---
> 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
>


2019-02-26 14:58:54

by zerons

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

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.

2019-02-26 16:26:38

by Daniel Borkmann

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

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

2019-02-26 17:35:41

by Martin KaFai Lau

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

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 <[email protected]>

2019-02-26 18:13:06

by Daniel Borkmann

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

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 <[email protected]>

Applied, thanks!

2019-02-27 00:50:47

by zerons

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

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.