From: Xu Kuohai <[email protected]>
This series fixes bugs found by ASAN when running bpf selftests on arm64.
v4:
- Address Andrii's suggestions
v3: https://lore.kernel.org/bpf/[email protected]
- Fix error failure of case test_xdp_adjust_tail_grow exposed by this series
v2: https://lore.kernel.org/bpf/[email protected]
- Rebase and fix conflict
v1: https://lore.kernel.org/bpf/[email protected]
Xu Kuohai (6):
libbpf: Fix use-after-free in btf_dump_name_dups
libbpf: Fix memory leak in parse_usdt_arg()
selftests/bpf: Fix memory leak caused by not destroying skeleton
selftest/bpf: Fix memory leak in kprobe_multi_test
selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow
selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c
tools/lib/bpf/btf_dump.c | 30 +++++++++++++++++--
tools/lib/bpf/usdt.c | 11 +++----
.../bpf/prog_tests/kprobe_multi_test.c | 26 ++++++++--------
.../selftests/bpf/prog_tests/map_kptr.c | 3 +-
.../selftests/bpf/prog_tests/tracing_struct.c | 3 +-
.../bpf/prog_tests/xdp_adjust_tail.c | 7 +++--
6 files changed, 53 insertions(+), 27 deletions(-)
--
2.30.2
From: Xu Kuohai <[email protected]>
In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
is allocated but not freed. Fix it.
Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
Signed-off-by: Xu Kuohai <[email protected]>
---
tools/lib/bpf/usdt.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index e83b497c2245..49f3c3b7f609 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -1348,25 +1348,23 @@ static int calc_pt_regs_off(const char *reg_name)
static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
{
- char *reg_name = NULL;
+ char reg_name[16];
int arg_sz, len, reg_off;
long off;
- if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, ®_name, &off, &len) == 3) {
+ if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], %ld ] %n", &arg_sz, reg_name, &off, &len) == 3) {
/* Memory dereference case, e.g., -4@[sp, 96] */
arg->arg_type = USDT_ARG_REG_DEREF;
arg->val_off = off;
reg_off = calc_pt_regs_off(reg_name);
- free(reg_name);
if (reg_off < 0)
return reg_off;
arg->reg_off = reg_off;
- } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, ®_name, &len) == 2) {
+ } else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", &arg_sz, reg_name, &len) == 2) {
/* Memory dereference case, e.g., -4@[sp] */
arg->arg_type = USDT_ARG_REG_DEREF;
arg->val_off = 0;
reg_off = calc_pt_regs_off(reg_name);
- free(reg_name);
if (reg_off < 0)
return reg_off;
arg->reg_off = reg_off;
@@ -1375,12 +1373,11 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
arg->arg_type = USDT_ARG_CONST;
arg->val_off = off;
arg->reg_off = 0;
- } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, ®_name, &len) == 2) {
+ } else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", &arg_sz, reg_name, &len) == 2) {
/* Register read case, e.g., -8@x4 */
arg->arg_type = USDT_ARG_REG;
arg->val_off = 0;
reg_off = calc_pt_regs_off(reg_name);
- free(reg_name);
if (reg_off < 0)
return reg_off;
arg->reg_off = reg_off;
--
2.30.2
From: Xu Kuohai <[email protected]>
test_xdp_adjust_tail_grow failed with ipv6:
test_xdp_adjust_tail_grow:FAIL:ipv6 unexpected error: -28 (errno 28)
The reason is that this test case tests ipv4 before ipv6, and when ipv4
test finished, topts.data_size_out was set to 54, which is smaller than the
ipv6 output data size 114, so ipv6 test fails with NOSPC error.
Fix it by reset topts.data_size_out to sizeof(buf) before testing ipv6.
Fixes: 04fcb5f9a104 ("selftests/bpf: Migrate from bpf_prog_test_run")
Signed-off-by: Xu Kuohai <[email protected]>
---
tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 9b9cf8458adf..009ee37607df 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -63,6 +63,7 @@ static void test_xdp_adjust_tail_grow(void)
expect_sz = sizeof(pkt_v6) + 40; /* Test grow with 40 bytes */
topts.data_in = &pkt_v6;
topts.data_size_in = sizeof(pkt_v6);
+ topts.data_size_out = sizeof(buf);
err = bpf_prog_test_run_opts(prog_fd, &topts);
ASSERT_OK(err, "ipv6");
ASSERT_EQ(topts.retval, XDP_TX, "ipv6 retval");
--
2.30.2
From: Xu Kuohai <[email protected]>
The get_syms() function in kprobe_multi_test.c does not free the string
memory allocated by sscanf correctly. Fix it.
Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test")
Signed-off-by: Xu Kuohai <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
---
.../bpf/prog_tests/kprobe_multi_test.c | 26 ++++++++++---------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index d457a55ff408..287b3ac40227 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -325,7 +325,7 @@ static bool symbol_equal(const void *key1, const void *key2, void *ctx __maybe_u
static int get_syms(char ***symsp, size_t *cntp)
{
size_t cap = 0, cnt = 0, i;
- char *name, **syms = NULL;
+ char *name = NULL, **syms = NULL;
struct hashmap *map;
char buf[256];
FILE *f;
@@ -352,6 +352,8 @@ static int get_syms(char ***symsp, size_t *cntp)
/* skip modules */
if (strchr(buf, '['))
continue;
+
+ free(name);
if (sscanf(buf, "%ms$*[^\n]\n", &name) != 1)
continue;
/*
@@ -369,32 +371,32 @@ static int get_syms(char ***symsp, size_t *cntp)
if (!strncmp(name, "__ftrace_invalid_address__",
sizeof("__ftrace_invalid_address__") - 1))
continue;
+
err = hashmap__add(map, name, NULL);
- if (err) {
- free(name);
- if (err == -EEXIST)
- continue;
+ if (err == -EEXIST)
+ continue;
+ if (err)
goto error;
- }
+
err = libbpf_ensure_mem((void **) &syms, &cap,
sizeof(*syms), cnt + 1);
- if (err) {
- free(name);
+ if (err)
goto error;
- }
- syms[cnt] = name;
- cnt++;
+
+ syms[cnt++] = name;
+ name = NULL;
}
*symsp = syms;
*cntp = cnt;
error:
+ free(name);
fclose(f);
hashmap__free(map);
if (err) {
for (i = 0; i < cnt; i++)
- free(syms[cnt]);
+ free(syms[i]);
free(syms);
}
return err;
--
2.30.2
From: Xu Kuohai <[email protected]>
xdp_adjust_tail.c calls ASSERT_OK() to check the return value of
bpf_prog_test_load(), but the condition is not correct. Fix it.
Fixes: 791cad025051 ("bpf: selftests: Get rid of CHECK macro in xdp_adjust_tail.c")
Signed-off-by: Xu Kuohai <[email protected]>
---
tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 009ee37607df..39973ea1ce43 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -18,7 +18,7 @@ static void test_xdp_adjust_tail_shrink(void)
);
err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
- if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
+ if (!ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
return;
err = bpf_prog_test_run_opts(prog_fd, &topts);
@@ -53,7 +53,7 @@ static void test_xdp_adjust_tail_grow(void)
);
err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
- if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
+ if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
return;
err = bpf_prog_test_run_opts(prog_fd, &topts);
@@ -90,7 +90,7 @@ static void test_xdp_adjust_tail_grow2(void)
);
err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
- if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
+ if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
return;
/* Test case-64 */
--
2.30.2
On 10/11/22 5:01 AM, Xu Kuohai wrote:
> From: Xu Kuohai <[email protected]>
>
> test_xdp_adjust_tail_grow failed with ipv6:
> test_xdp_adjust_tail_grow:FAIL:ipv6 unexpected error: -28 (errno 28)
>
> The reason is that this test case tests ipv4 before ipv6, and when ipv4
> test finished, topts.data_size_out was set to 54, which is smaller than the
> ipv6 output data size 114, so ipv6 test fails with NOSPC error.
>
> Fix it by reset topts.data_size_out to sizeof(buf) before testing ipv6.
>
> Fixes: 04fcb5f9a104 ("selftests/bpf: Migrate from bpf_prog_test_run")
> Signed-off-by: Xu Kuohai <[email protected]>
> ---
> tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> index 9b9cf8458adf..009ee37607df 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> @@ -63,6 +63,7 @@ static void test_xdp_adjust_tail_grow(void)
> expect_sz = sizeof(pkt_v6) + 40; /* Test grow with 40 bytes */
> topts.data_in = &pkt_v6;
> topts.data_size_in = sizeof(pkt_v6);
> + topts.data_size_out = sizeof(buf);
lgtm but how was it working before... weird.
> err = bpf_prog_test_run_opts(prog_fd, &topts);
> ASSERT_OK(err, "ipv6");
> ASSERT_EQ(topts.retval, XDP_TX, "ipv6 retval");
On 10/11/22 5:01 AM, Xu Kuohai wrote:
> From: Xu Kuohai <[email protected]>
>
> This series fixes bugs found by ASAN when running bpf selftests on arm64
Overall lgtm.
Acked-by: Martin KaFai Lau <[email protected]>
On 10/11/22 5:01 AM, Xu Kuohai wrote:
> From: Xu Kuohai <[email protected]>
>
> xdp_adjust_tail.c calls ASSERT_OK() to check the return value of
> bpf_prog_test_load(), but the condition is not correct. Fix it.
>
> Fixes: 791cad025051 ("bpf: selftests: Get rid of CHECK macro in xdp_adjust_tail.c")
> Signed-off-by: Xu Kuohai <[email protected]>
> ---
> tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> index 009ee37607df..39973ea1ce43 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
> @@ -18,7 +18,7 @@ static void test_xdp_adjust_tail_shrink(void)
> );
>
> err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> - if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
> + if (!ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
> return;
>
> err = bpf_prog_test_run_opts(prog_fd, &topts);
> @@ -53,7 +53,7 @@ static void test_xdp_adjust_tail_grow(void)
> );
>
> err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> - if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
> + if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
Ouch... ic. It is why this test has been passing.
> return;
>
> err = bpf_prog_test_run_opts(prog_fd, &topts);
> @@ -90,7 +90,7 @@ static void test_xdp_adjust_tail_grow2(void)
> );
>
> err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
> - if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
> + if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
> return;
>
> /* Test case-64 */
On Tue, Oct 11, 2022 at 4:43 AM Xu Kuohai <[email protected]> wrote:
>
> From: Xu Kuohai <[email protected]>
>
> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
> is allocated but not freed. Fix it.
>
> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
> Signed-off-by: Xu Kuohai <[email protected]>
> ---
> tools/lib/bpf/usdt.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index e83b497c2245..49f3c3b7f609 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -1348,25 +1348,23 @@ static int calc_pt_regs_off(const char *reg_name)
>
> static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
> {
> - char *reg_name = NULL;
> + char reg_name[16];
> int arg_sz, len, reg_off;
> long off;
>
> - if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, ®_name, &off, &len) == 3) {
> + if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], %ld ] %n", &arg_sz, reg_name, &off, &len) == 3) {
It would be nice to do the same change for other architectures where
it makes sense and avoid having to deal with unnecessary memory
allocations. Please send follow up patches with similar changes for
other implementations of parse_usdt_arg. Thanks.
> /* Memory dereference case, e.g., -4@[sp, 96] */
> arg->arg_type = USDT_ARG_REG_DEREF;
> arg->val_off = off;
> reg_off = calc_pt_regs_off(reg_name);
> - free(reg_name);
> if (reg_off < 0)
> return reg_off;
> arg->reg_off = reg_off;
> - } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, ®_name, &len) == 2) {
> + } else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", &arg_sz, reg_name, &len) == 2) {
> /* Memory dereference case, e.g., -4@[sp] */
> arg->arg_type = USDT_ARG_REG_DEREF;
> arg->val_off = 0;
> reg_off = calc_pt_regs_off(reg_name);
> - free(reg_name);
> if (reg_off < 0)
> return reg_off;
> arg->reg_off = reg_off;
> @@ -1375,12 +1373,11 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
> arg->arg_type = USDT_ARG_CONST;
> arg->val_off = off;
> arg->reg_off = 0;
> - } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, ®_name, &len) == 2) {
> + } else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", &arg_sz, reg_name, &len) == 2) {
> /* Register read case, e.g., -8@x4 */
> arg->arg_type = USDT_ARG_REG;
> arg->val_off = 0;
> reg_off = calc_pt_regs_off(reg_name);
> - free(reg_name);
> if (reg_off < 0)
> return reg_off;
> arg->reg_off = reg_off;
> --
> 2.30.2
>
Hello:
This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[email protected]>:
On Tue, 11 Oct 2022 08:01:02 -0400 you wrote:
> From: Xu Kuohai <[email protected]>
>
> This series fixes bugs found by ASAN when running bpf selftests on arm64.
>
> v4:
> - Address Andrii's suggestions
>
> [...]
Here is the summary with links:
- [bpf-next,v4,1/6] libbpf: Fix use-after-free in btf_dump_name_dups
https://git.kernel.org/bpf/bpf-next/c/02c1e5b0bbb8
- [bpf-next,v4,2/6] libbpf: Fix memory leak in parse_usdt_arg()
https://git.kernel.org/bpf/bpf-next/c/cd168cc6f685
- [bpf-next,v4,3/6] selftests/bpf: Fix memory leak caused by not destroying skeleton
https://git.kernel.org/bpf/bpf-next/c/fbca16071678
- [bpf-next,v4,4/6] selftest/bpf: Fix memory leak in kprobe_multi_test
https://git.kernel.org/bpf/bpf-next/c/159c69121102
- [bpf-next,v4,5/6] selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow
https://git.kernel.org/bpf/bpf-next/c/496848b47126
- [bpf-next,v4,6/6] selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c
https://git.kernel.org/bpf/bpf-next/c/cafecc0e3df3
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html