* [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails
2024-09-12 1:28 [PATCH bpf-next 0/2] Two tiny fixes for btf record Hou Tao
@ 2024-09-12 1:28 ` Hou Tao
2024-09-12 7:55 ` Jiri Olsa
2024-09-12 1:28 ` [PATCH bpf-next 2/2] bpf: Call the missed kfree() when there is no special field in btf Hou Tao
2024-09-14 0:00 ` [PATCH bpf-next 0/2] Two tiny fixes for btf record patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Hou Tao @ 2024-09-12 1:28 UTC (permalink / raw)
To: bpf
Cc: Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Hao Luo, Yonghong Song,
Daniel Borkmann, KP Singh, Stanislav Fomichev, Jiri Olsa,
John Fastabend, Amery Hung, Dave Marchevsky, houtao1, xukuohai
From: Hou Tao <houtao1@huawei.com>
When security_bpf_map_create() in map_create() fails, map_create() will
call btf_put() and ->map_free() callback to free the map. It doesn't
free the btf_record of map value, so add the missed btf_record_free()
when map creation fails.
However btf_record_free() needs to be called after ->map_free() just
like bpf_map_free_deferred() did, because ->map_free() may use the
btf_record to free the special fields in preallocated map value. So
factor out bpf_map_free() helper to free the map, btf_record, and btf
orderly and use the helper in both map_create() and
bpf_map_free_deferred().
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
kernel/bpf/syscall.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6988e432fc3d..1a0dc1d81118 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -735,15 +735,11 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
}
}
-/* called from workqueue */
-static void bpf_map_free_deferred(struct work_struct *work)
+static void bpf_map_free(struct bpf_map *map)
{
- struct bpf_map *map = container_of(work, struct bpf_map, work);
struct btf_record *rec = map->record;
struct btf *btf = map->btf;
- security_bpf_map_free(map);
- bpf_map_release_memcg(map);
/* implementation dependent freeing */
map->ops->map_free(map);
/* Delay freeing of btf_record for maps, as map_free
@@ -762,6 +758,16 @@ static void bpf_map_free_deferred(struct work_struct *work)
btf_put(btf);
}
+/* called from workqueue */
+static void bpf_map_free_deferred(struct work_struct *work)
+{
+ struct bpf_map *map = container_of(work, struct bpf_map, work);
+
+ security_bpf_map_free(map);
+ bpf_map_release_memcg(map);
+ bpf_map_free(map);
+}
+
static void bpf_map_put_uref(struct bpf_map *map)
{
if (atomic64_dec_and_test(&map->usercnt)) {
@@ -1413,8 +1419,7 @@ static int map_create(union bpf_attr *attr)
free_map_sec:
security_bpf_map_free(map);
free_map:
- btf_put(map->btf);
- map->ops->map_free(map);
+ bpf_map_free(map);
put_token:
bpf_token_put(token);
return err;
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails
2024-09-12 1:28 ` [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails Hou Tao
@ 2024-09-12 7:55 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2024-09-12 7:55 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Hao Luo, Yonghong Song,
Daniel Borkmann, KP Singh, Stanislav Fomichev, John Fastabend,
Amery Hung, Dave Marchevsky, houtao1, xukuohai
On Thu, Sep 12, 2024 at 09:28:44AM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> When security_bpf_map_create() in map_create() fails, map_create() will
> call btf_put() and ->map_free() callback to free the map. It doesn't
> free the btf_record of map value, so add the missed btf_record_free()
> when map creation fails.
>
> However btf_record_free() needs to be called after ->map_free() just
> like bpf_map_free_deferred() did, because ->map_free() may use the
> btf_record to free the special fields in preallocated map value. So
> factor out bpf_map_free() helper to free the map, btf_record, and btf
> orderly and use the helper in both map_create() and
> bpf_map_free_deferred().
lgtm
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> kernel/bpf/syscall.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6988e432fc3d..1a0dc1d81118 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -735,15 +735,11 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
> }
> }
>
> -/* called from workqueue */
> -static void bpf_map_free_deferred(struct work_struct *work)
> +static void bpf_map_free(struct bpf_map *map)
> {
> - struct bpf_map *map = container_of(work, struct bpf_map, work);
> struct btf_record *rec = map->record;
> struct btf *btf = map->btf;
>
> - security_bpf_map_free(map);
> - bpf_map_release_memcg(map);
> /* implementation dependent freeing */
> map->ops->map_free(map);
> /* Delay freeing of btf_record for maps, as map_free
> @@ -762,6 +758,16 @@ static void bpf_map_free_deferred(struct work_struct *work)
> btf_put(btf);
> }
>
> +/* called from workqueue */
> +static void bpf_map_free_deferred(struct work_struct *work)
> +{
> + struct bpf_map *map = container_of(work, struct bpf_map, work);
> +
> + security_bpf_map_free(map);
> + bpf_map_release_memcg(map);
> + bpf_map_free(map);
> +}
> +
> static void bpf_map_put_uref(struct bpf_map *map)
> {
> if (atomic64_dec_and_test(&map->usercnt)) {
> @@ -1413,8 +1419,7 @@ static int map_create(union bpf_attr *attr)
> free_map_sec:
> security_bpf_map_free(map);
> free_map:
> - btf_put(map->btf);
> - map->ops->map_free(map);
> + bpf_map_free(map);
> put_token:
> bpf_token_put(token);
> return err;
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] bpf: Call the missed kfree() when there is no special field in btf
2024-09-12 1:28 [PATCH bpf-next 0/2] Two tiny fixes for btf record Hou Tao
2024-09-12 1:28 ` [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails Hou Tao
@ 2024-09-12 1:28 ` Hou Tao
2024-09-12 7:53 ` Jiri Olsa
2024-09-14 0:00 ` [PATCH bpf-next 0/2] Two tiny fixes for btf record patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Hou Tao @ 2024-09-12 1:28 UTC (permalink / raw)
To: bpf
Cc: Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Hao Luo, Yonghong Song,
Daniel Borkmann, KP Singh, Stanislav Fomichev, Jiri Olsa,
John Fastabend, Amery Hung, Dave Marchevsky, houtao1, xukuohai
From: Hou Tao <houtao1@huawei.com>
Call the missed kfree() in btf_parse_struct_metas() when there is no
special field in btf, otherwise will get the following kmemleak report:
unreferenced object 0xffff888101033620 (size 8):
comm "test_progs", pid 604, jiffies 4295127011
......
backtrace (crc e77dc444):
[<00000000186f90f3>] kmemleak_alloc+0x4b/0x80
[<00000000ac8e9c4d>] __kmalloc_cache_noprof+0x2a1/0x310
[<00000000d99d68d6>] btf_new_fd+0x72d/0xe90
[<00000000f010b7f8>] __sys_bpf+0xec3/0x2410
[<00000000e077ed6f>] __x64_sys_bpf+0x1f/0x30
[<00000000a12f9e55>] x64_sys_call+0x199/0x9f0
[<00000000f3029ea6>] do_syscall_64+0x3b/0xc0
[<000000005640913a>] entry_SYSCALL_64_after_hwframe+0x4b/0x53
Fixes: 7a851ecb1806 ("bpf: Search for kptrs in prog BTF structs")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
kernel/bpf/btf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 59b4f7265761..31eae516f701 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5572,8 +5572,10 @@ btf_parse_struct_metas(struct bpf_verifier_log *log, struct btf *btf)
aof->ids[aof->cnt++] = i;
}
- if (!aof->cnt)
+ if (!aof->cnt) {
+ kfree(aof);
return NULL;
+ }
sort(&aof->ids, aof->cnt, sizeof(aof->ids[0]), btf_id_cmp_func, NULL);
for (i = 1; i < n; i++) {
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 2/2] bpf: Call the missed kfree() when there is no special field in btf
2024-09-12 1:28 ` [PATCH bpf-next 2/2] bpf: Call the missed kfree() when there is no special field in btf Hou Tao
@ 2024-09-12 7:53 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2024-09-12 7:53 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Martin KaFai Lau, Alexei Starovoitov, Andrii Nakryiko,
Eduard Zingerman, Song Liu, Hao Luo, Yonghong Song,
Daniel Borkmann, KP Singh, Stanislav Fomichev, John Fastabend,
Amery Hung, Dave Marchevsky, houtao1, xukuohai
On Thu, Sep 12, 2024 at 09:28:45AM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> Call the missed kfree() in btf_parse_struct_metas() when there is no
> special field in btf, otherwise will get the following kmemleak report:
>
> unreferenced object 0xffff888101033620 (size 8):
> comm "test_progs", pid 604, jiffies 4295127011
> ......
> backtrace (crc e77dc444):
> [<00000000186f90f3>] kmemleak_alloc+0x4b/0x80
> [<00000000ac8e9c4d>] __kmalloc_cache_noprof+0x2a1/0x310
> [<00000000d99d68d6>] btf_new_fd+0x72d/0xe90
> [<00000000f010b7f8>] __sys_bpf+0xec3/0x2410
> [<00000000e077ed6f>] __x64_sys_bpf+0x1f/0x30
> [<00000000a12f9e55>] x64_sys_call+0x199/0x9f0
> [<00000000f3029ea6>] do_syscall_64+0x3b/0xc0
> [<000000005640913a>] entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
> Fixes: 7a851ecb1806 ("bpf: Search for kptrs in prog BTF structs")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> kernel/bpf/btf.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 59b4f7265761..31eae516f701 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -5572,8 +5572,10 @@ btf_parse_struct_metas(struct bpf_verifier_log *log, struct btf *btf)
> aof->ids[aof->cnt++] = i;
> }
I was wondering we could get away without the initial kmalloc and
let the first krealoc do the first allocation, but it might need
some other extra checks, so not sure it's worth it
in any case this lgtm
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
>
> - if (!aof->cnt)
> + if (!aof->cnt) {
> + kfree(aof);
> return NULL;
> + }
> sort(&aof->ids, aof->cnt, sizeof(aof->ids[0]), btf_id_cmp_func, NULL);
>
> for (i = 1; i < n; i++) {
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 0/2] Two tiny fixes for btf record
2024-09-12 1:28 [PATCH bpf-next 0/2] Two tiny fixes for btf record Hou Tao
2024-09-12 1:28 ` [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails Hou Tao
2024-09-12 1:28 ` [PATCH bpf-next 2/2] bpf: Call the missed kfree() when there is no special field in btf Hou Tao
@ 2024-09-14 0:00 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-09-14 0:00 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, martin.lau, alexei.starovoitov, andrii, eddyz87, song,
haoluo, yonghong.song, daniel, kpsingh, sdf, jolsa,
john.fastabend, amery.hung, davemarchevsky, houtao1, xukuohai
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Thu, 12 Sep 2024 09:28:43 +0800 you wrote:
> From: Hou Tao <houtao1@huawei.com>
>
> Hi,
>
> The tiny patch set aims to fix two problems found during the development
> of supporting dynptr key in hash table. Patch #1 fixes the missed
> btf_record_free() when map creation fails and patch #2 fixes the missed
> kfree() when there is no special field in the passed btf.
>
> [...]
Here is the summary with links:
- [bpf-next,1/2] bpf: Call the missed btf_record_free() when map creation fails
https://git.kernel.org/bpf/bpf-next/c/87e9675a0dfd
- [bpf-next,2/2] bpf: Call the missed kfree() when there is no special field in btf
https://git.kernel.org/bpf/bpf-next/c/986deb297d48
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread