diff mbox series

bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id()

Message ID 1551190537-28694-1-git-send-email-sironhide0null@gmail.com
State Accepted
Delegated to: BPF Maintainers
Headers show
Series bpf: decrease usercnt if bpf_map_new_fd() fails in bpf_map_get_fd_by_id() | expand

Commit Message

zerons Feb. 26, 2019, 2:15 p.m. UTC 
[ 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(-)

--
2.7.4

Comments

Daniel Borkmann Feb. 26, 2019, 2:44 p.m. UTC | #1 
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
>
zerons Feb. 26, 2019, 2:58 p.m. UTC | #2 
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.
Daniel Borkmann Feb. 26, 2019, 4:24 p.m. UTC | #3 
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
Martin KaFai Lau Feb. 26, 2019, 5:33 p.m. UTC | #4 
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>
Daniel Borkmann Feb. 26, 2019, 6:12 p.m. UTC | #5 
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!
zerons Feb. 27, 2019, 12:49 a.m. UTC | #6 
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.
diff mbox series

Patch

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;
 }