All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests
@ 2021-11-07  4:03 Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 1/9] selftests/bpf: pass sanitizer flags to linker through LDFLAGS Andrii Nakryiko
                   ` (9 more replies)
  0 siblings, 10 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Fix all the memory leaks reported by ASAN. All but one are just improper
resource clean up in selftests. But one memory leak was discovered in libbpf,
leaving inner map's name leaked.

First patch fixes selftests' Makefile by passing through SAN_CFLAGS to linker.
Without that compiling with SAN_CFLAGS=-fsanitize=address kept failing.

Running selftests under ASAN in BPF CI is the next step, we just need to make
sure all the necessary libraries (libasan and liblsan) are installed on the
host and inside the VM. Would be great to get some help with that, but for now
make sure that test_progs run is clean from leak sanitizer errors.

v1->v2:
  - call bpf_map__destroy() conditionally if map->inner_map is present.

Andrii Nakryiko (9):
  selftests/bpf: pass sanitizer flags to linker through LDFLAGS
  libbpf: free up resources used by inner map definition
  selftests/bpf: fix memory leaks in btf_type_c_dump() helper
  selftests/bpf: free per-cpu values array in bpf_iter selftest
  selftests/bpf: free inner strings index in btf selftest
  selftests/bpf: clean up btf and btf_dump in dump_datasec test
  selftests/bpf: avoid duplicate btf__parse() call
  selftests/bpf: destroy XDP link correctly
  selftests/bpf: fix bpf_object leak in skb_ctx selftest

 tools/lib/bpf/libbpf.c                                   | 5 ++++-
 tools/testing/selftests/bpf/Makefile                     | 1 +
 tools/testing/selftests/bpf/btf_helpers.c                | 9 +++++++--
 tools/testing/selftests/bpf/prog_tests/bpf_iter.c        | 1 +
 tools/testing/selftests/bpf/prog_tests/btf.c             | 6 ++----
 tools/testing/selftests/bpf/prog_tests/btf_dump.c        | 8 ++++++--
 tools/testing/selftests/bpf/prog_tests/core_reloc.c      | 2 +-
 .../testing/selftests/bpf/prog_tests/migrate_reuseport.c | 4 ++--
 tools/testing/selftests/bpf/prog_tests/skb_ctx.c         | 2 ++
 9 files changed, 26 insertions(+), 12 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 1/9] selftests/bpf: pass sanitizer flags to linker through LDFLAGS
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 2/9] libbpf: free up resources used by inner map definition Andrii Nakryiko
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

When adding -fsanitize=address to SAN_CFLAGS, it has to be passed both
to compiler through CFLAGS as well as linker through LDFLAGS. Add
SAN_CFLAGS into LDFLAGS to allow building selftests with ASAN.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 54b0a41a3775..851640ced5c1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -26,6 +26,7 @@ CFLAGS += -g -O0 -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)		\
 	  -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)			\
 	  -Dbpf_prog_load=bpf_prog_test_load				\
 	  -Dbpf_load_program=bpf_test_load_program
+LDFLAGS += $(SAN_CFLAGS)
 LDLIBS += -lcap -lelf -lz -lrt -lpthread
 
 # Silence some warnings when compiled with clang
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 2/9] libbpf: free up resources used by inner map definition
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 1/9] selftests/bpf: pass sanitizer flags to linker through LDFLAGS Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 3/9] selftests/bpf: fix memory leaks in btf_type_c_dump() helper Andrii Nakryiko
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

It's not enough to just free(map->inner_map), as inner_map itself can
have extra memory allocated, like map name.

Fixes: 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map support")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7fcea11ecaa9..eef71ac8b99e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9053,7 +9053,10 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
 		pr_warn("error: inner_map_fd already specified\n");
 		return libbpf_err(-EINVAL);
 	}
-	zfree(&map->inner_map);
+	if (map->inner_map) {
+		bpf_map__destroy(map->inner_map);
+		zfree(&map->inner_map);
+	}
 	map->inner_map_fd = fd;
 	return 0;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 3/9] selftests/bpf: fix memory leaks in btf_type_c_dump() helper
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 1/9] selftests/bpf: pass sanitizer flags to linker through LDFLAGS Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 2/9] libbpf: free up resources used by inner map definition Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest Andrii Nakryiko
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Free up memory and resources used by temporary allocated memstream and
btf_dump instance.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/btf_helpers.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/btf_helpers.c b/tools/testing/selftests/bpf/btf_helpers.c
index b5b6b013a245..3d1a748d09d8 100644
--- a/tools/testing/selftests/bpf/btf_helpers.c
+++ b/tools/testing/selftests/bpf/btf_helpers.c
@@ -251,18 +251,23 @@ const char *btf_type_c_dump(const struct btf *btf)
 	d = btf_dump__new(btf, NULL, &opts, btf_dump_printf);
 	if (libbpf_get_error(d)) {
 		fprintf(stderr, "Failed to create btf_dump instance: %ld\n", libbpf_get_error(d));
-		return NULL;
+		goto err_out;
 	}
 
 	for (i = 1; i < btf__type_cnt(btf); i++) {
 		err = btf_dump__dump_type(d, i);
 		if (err) {
 			fprintf(stderr, "Failed to dump type [%d]: %d\n", i, err);
-			return NULL;
+			goto err_out;
 		}
 	}
 
+	btf_dump__free(d);
 	fflush(buf_file);
 	fclose(buf_file);
 	return buf;
+err_out:
+	btf_dump__free(d);
+	fclose(buf_file);
+	return NULL;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 3/9] selftests/bpf: fix memory leaks in btf_type_c_dump() helper Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07 16:34   ` Hengqi Chen
  2021-11-07  4:03 ` [PATCH v2 bpf-next 5/9] selftests/bpf: free inner strings index in btf selftest Andrii Nakryiko
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Array holding per-cpu values wasn't freed. Fix that.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 9454331aaf85..71c724a3f988 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -770,6 +770,7 @@ static void test_bpf_percpu_hash_map(void)
 	bpf_link__destroy(link);
 out:
 	bpf_iter_bpf_percpu_hash_map__destroy(skel);
+	free(val);
 }
 
 static void test_bpf_array_map(void)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 5/9] selftests/bpf: free inner strings index in btf selftest
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 6/9] selftests/bpf: clean up btf and btf_dump in dump_datasec test Andrii Nakryiko
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Inner array of allocated strings wasn't freed on success. Now it's
always freed.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/btf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index ac596cb06e40..ebd1aa4d09d6 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -4046,11 +4046,9 @@ static void *btf_raw_create(const struct btf_header *hdr,
 			next_str_idx < strs_cnt ? strs_idx[next_str_idx] : NULL;
 
 done:
+	free(strs_idx);
 	if (err) {
-		if (raw_btf)
-			free(raw_btf);
-		if (strs_idx)
-			free(strs_idx);
+		free(raw_btf);
 		return NULL;
 	}
 	return raw_btf;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 6/9] selftests/bpf: clean up btf and btf_dump in dump_datasec test
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 5/9] selftests/bpf: free inner strings index in btf selftest Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 7/9] selftests/bpf: avoid duplicate btf__parse() call Andrii Nakryiko
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

Free up used resources at the end and on error. Also make it more
obvious that there is btf__parse() call that creates struct btf
instance.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/btf_dump.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index aa76360d8f49..a04961942dfa 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -814,21 +814,25 @@ static void test_btf_datasec(struct btf *btf, struct btf_dump *d, char *str,
 
 static void test_btf_dump_datasec_data(char *str)
 {
-	struct btf *btf = btf__parse("xdping_kern.o", NULL);
+	struct btf *btf;
 	struct btf_dump_opts opts = { .ctx = str };
 	char license[4] = "GPL";
 	struct btf_dump *d;
 
+	btf = btf__parse("xdping_kern.o", NULL);
 	if (!ASSERT_OK_PTR(btf, "xdping_kern.o BTF not found"))
 		return;
 
 	d = btf_dump__new(btf, NULL, &opts, btf_dump_snprintf);
 	if (!ASSERT_OK_PTR(d, "could not create BTF dump"))
-		return;
+		goto out;
 
 	test_btf_datasec(btf, d, str, "license",
 			 "SEC(\"license\") char[4] _license = (char[4])['G','P','L',];",
 			 license, sizeof(license));
+out:
+	btf_dump__free(d);
+	btf__free(btf);
 }
 
 void test_btf_dump() {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 7/9] selftests/bpf: avoid duplicate btf__parse() call
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (5 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 6/9] selftests/bpf: clean up btf and btf_dump in dump_datasec test Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 8/9] selftests/bpf: destroy XDP link correctly Andrii Nakryiko
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

btf__parse() is repeated after successful setup, leaving the first
instance leaked. Remove redundant and premature call.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/core_reloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index 55ec85ba7375..1041d0c593f6 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -433,7 +433,7 @@ static int setup_type_id_case_local(struct core_reloc_test_case *test)
 
 static int setup_type_id_case_success(struct core_reloc_test_case *test) {
 	struct core_reloc_type_id_output *exp = (void *)test->output;
-	struct btf *targ_btf = btf__parse(test->btf_src_file, NULL);
+	struct btf *targ_btf;
 	int err;
 
 	err = setup_type_id_case_local(test);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 8/9] selftests/bpf: destroy XDP link correctly
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (6 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 7/9] selftests/bpf: avoid duplicate btf__parse() call Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07  4:03 ` [PATCH v2 bpf-next 9/9] selftests/bpf: fix bpf_object leak in skb_ctx selftest Andrii Nakryiko
  2021-11-07 16:36 ` [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Hengqi Chen
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

bpf_link__detach() was confused with bpf_link__destroy() and leaves
leaked FD in the process. Fix the problem.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/migrate_reuseport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/migrate_reuseport.c b/tools/testing/selftests/bpf/prog_tests/migrate_reuseport.c
index 7589c03fd26b..eb2feaac81fe 100644
--- a/tools/testing/selftests/bpf/prog_tests/migrate_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/migrate_reuseport.c
@@ -204,8 +204,8 @@ static int pass_ack(struct migrate_reuseport_test_case *test_case)
 {
 	int err;
 
-	err = bpf_link__detach(test_case->link);
-	if (!ASSERT_OK(err, "bpf_link__detach"))
+	err = bpf_link__destroy(test_case->link);
+	if (!ASSERT_OK(err, "bpf_link__destroy"))
 		return -1;
 
 	test_case->link = NULL;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 bpf-next 9/9] selftests/bpf: fix bpf_object leak in skb_ctx selftest
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (7 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 8/9] selftests/bpf: destroy XDP link correctly Andrii Nakryiko
@ 2021-11-07  4:03 ` Andrii Nakryiko
  2021-11-07 16:36 ` [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Hengqi Chen
  9 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07  4:03 UTC (permalink / raw)
  To: bpf, ast, daniel; +Cc: andrii, kernel-team

skb_ctx selftest didn't close bpf_object implicitly allocated by
bpf_prog_test_load() helper. Fix the problem by explicitly calling
bpf_object__close() at the end of the test.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/skb_ctx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
index c437e6ba8fe2..db4d72563aae 100644
--- a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
+++ b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
@@ -111,4 +111,6 @@ void test_skb_ctx(void)
 		   "ctx_out_mark",
 		   "skb->mark == %u, expected %d\n",
 		   skb.mark, 10);
+
+	bpf_object__close(obj);
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest
  2021-11-07  4:03 ` [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest Andrii Nakryiko
@ 2021-11-07 16:34   ` Hengqi Chen
  2021-11-07 16:40     ` Andrii Nakryiko
  0 siblings, 1 reply; 13+ messages in thread
From: Hengqi Chen @ 2021-11-07 16:34 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team

Hi, Andrii

On 2021/11/7 12:03 PM, Andrii Nakryiko wrote:
> Array holding per-cpu values wasn't freed. Fix that.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 9454331aaf85..71c724a3f988 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -770,6 +770,7 @@ static void test_bpf_percpu_hash_map(void)
>  	bpf_link__destroy(link);
>  out:
>  	bpf_iter_bpf_percpu_hash_map__destroy(skel);
> +	free(val);
>  }
>  
>  static void test_bpf_array_map(void)
> 

The val is allocated at the very beginning of this function,
when bpf_iter_bpf_percpu_hash_map__open failed, the val still
leaked.

So we should have:

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 9454331aaf85..ee6727389ef6 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -686,7 +686,7 @@ static void test_bpf_percpu_hash_map(void)
 {
        __u32 expected_key_a = 0, expected_key_b = 0;
        DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
-       struct bpf_iter_bpf_percpu_hash_map *skel;
+       struct bpf_iter_bpf_percpu_hash_map *skel = NULL;
        int err, i, j, len, map_fd, iter_fd;
        union bpf_iter_link_info linfo;
        __u32 expected_val = 0;
@@ -704,7 +704,7 @@ static void test_bpf_percpu_hash_map(void)
        skel = bpf_iter_bpf_percpu_hash_map__open();
        if (CHECK(!skel, "bpf_iter_bpf_percpu_hash_map__open",
                  "skeleton open failed\n"))
-               return;
+               goto out;
 
        skel->rodata->num_cpus = bpf_num_possible_cpus();
 
@@ -770,6 +770,7 @@ static void test_bpf_percpu_hash_map(void)
        bpf_link__destroy(link);
 out:
        bpf_iter_bpf_percpu_hash_map__destroy(skel);
+       free(val);
 }
 
 static void test_bpf_array_map(void)

Right?

Cheers,
--
Hengqi

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests
  2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
                   ` (8 preceding siblings ...)
  2021-11-07  4:03 ` [PATCH v2 bpf-next 9/9] selftests/bpf: fix bpf_object leak in skb_ctx selftest Andrii Nakryiko
@ 2021-11-07 16:36 ` Hengqi Chen
  9 siblings, 0 replies; 13+ messages in thread
From: Hengqi Chen @ 2021-11-07 16:36 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, ast, daniel; +Cc: kernel-team



On 2021/11/7 12:03 PM, Andrii Nakryiko wrote:
> Fix all the memory leaks reported by ASAN. All but one are just improper
> resource clean up in selftests. But one memory leak was discovered in libbpf,
> leaving inner map's name leaked.
> 
> First patch fixes selftests' Makefile by passing through SAN_CFLAGS to linker.
> Without that compiling with SAN_CFLAGS=-fsanitize=address kept failing.
> 
> Running selftests under ASAN in BPF CI is the next step, we just need to make
> sure all the necessary libraries (libasan and liblsan) are installed on the
> host and inside the VM. Would be great to get some help with that, but for now
> make sure that test_progs run is clean from leak sanitizer errors.
> 
> v1->v2:
>   - call bpf_map__destroy() conditionally if map->inner_map is present.
> 
> Andrii Nakryiko (9):
>   selftests/bpf: pass sanitizer flags to linker through LDFLAGS
>   libbpf: free up resources used by inner map definition
>   selftests/bpf: fix memory leaks in btf_type_c_dump() helper
>   selftests/bpf: free per-cpu values array in bpf_iter selftest
>   selftests/bpf: free inner strings index in btf selftest
>   selftests/bpf: clean up btf and btf_dump in dump_datasec test
>   selftests/bpf: avoid duplicate btf__parse() call
>   selftests/bpf: destroy XDP link correctly
>   selftests/bpf: fix bpf_object leak in skb_ctx selftest
> 
>  tools/lib/bpf/libbpf.c                                   | 5 ++++-
>  tools/testing/selftests/bpf/Makefile                     | 1 +
>  tools/testing/selftests/bpf/btf_helpers.c                | 9 +++++++--
>  tools/testing/selftests/bpf/prog_tests/bpf_iter.c        | 1 +
>  tools/testing/selftests/bpf/prog_tests/btf.c             | 6 ++----
>  tools/testing/selftests/bpf/prog_tests/btf_dump.c        | 8 ++++++--
>  tools/testing/selftests/bpf/prog_tests/core_reloc.c      | 2 +-
>  .../testing/selftests/bpf/prog_tests/migrate_reuseport.c | 4 ++--
>  tools/testing/selftests/bpf/prog_tests/skb_ctx.c         | 2 ++
>  9 files changed, 26 insertions(+), 12 deletions(-)
> 

For the series, feel free to add:

Reviewed-by: Hengqi Chen <hengqi.chen@gmail.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest
  2021-11-07 16:34   ` Hengqi Chen
@ 2021-11-07 16:40     ` Andrii Nakryiko
  0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2021-11-07 16:40 UTC (permalink / raw)
  To: Hengqi Chen
  Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Sun, Nov 7, 2021 at 8:34 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi, Andrii
>
> On 2021/11/7 12:03 PM, Andrii Nakryiko wrote:
> > Array holding per-cpu values wasn't freed. Fix that.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/bpf_iter.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index 9454331aaf85..71c724a3f988 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -770,6 +770,7 @@ static void test_bpf_percpu_hash_map(void)
> >       bpf_link__destroy(link);
> >  out:
> >       bpf_iter_bpf_percpu_hash_map__destroy(skel);
> > +     free(val);
> >  }
> >
> >  static void test_bpf_array_map(void)
> >
>
> The val is allocated at the very beginning of this function,
> when bpf_iter_bpf_percpu_hash_map__open failed, the val still
> leaked.
>
> So we should have:
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 9454331aaf85..ee6727389ef6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -686,7 +686,7 @@ static void test_bpf_percpu_hash_map(void)
>  {
>         __u32 expected_key_a = 0, expected_key_b = 0;
>         DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> -       struct bpf_iter_bpf_percpu_hash_map *skel;
> +       struct bpf_iter_bpf_percpu_hash_map *skel = NULL;
>         int err, i, j, len, map_fd, iter_fd;
>         union bpf_iter_link_info linfo;
>         __u32 expected_val = 0;
> @@ -704,7 +704,7 @@ static void test_bpf_percpu_hash_map(void)
>         skel = bpf_iter_bpf_percpu_hash_map__open();
>         if (CHECK(!skel, "bpf_iter_bpf_percpu_hash_map__open",
>                   "skeleton open failed\n"))
> -               return;
> +               goto out;
>

I've just moved val = malloc() here and left early return intact. Same
effect, less undoing to do.

>         skel->rodata->num_cpus = bpf_num_possible_cpus();
>
> @@ -770,6 +770,7 @@ static void test_bpf_percpu_hash_map(void)
>         bpf_link__destroy(link);
>  out:
>         bpf_iter_bpf_percpu_hash_map__destroy(skel);
> +       free(val);
>  }
>
>  static void test_bpf_array_map(void)
>
> Right?

Right, thanks for spotting!

>
> Cheers,
> --
> Hengqi

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-11-07 16:40 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-07  4:03 [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 1/9] selftests/bpf: pass sanitizer flags to linker through LDFLAGS Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 2/9] libbpf: free up resources used by inner map definition Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 3/9] selftests/bpf: fix memory leaks in btf_type_c_dump() helper Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 4/9] selftests/bpf: free per-cpu values array in bpf_iter selftest Andrii Nakryiko
2021-11-07 16:34   ` Hengqi Chen
2021-11-07 16:40     ` Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 5/9] selftests/bpf: free inner strings index in btf selftest Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 6/9] selftests/bpf: clean up btf and btf_dump in dump_datasec test Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 7/9] selftests/bpf: avoid duplicate btf__parse() call Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 8/9] selftests/bpf: destroy XDP link correctly Andrii Nakryiko
2021-11-07  4:03 ` [PATCH v2 bpf-next 9/9] selftests/bpf: fix bpf_object leak in skb_ctx selftest Andrii Nakryiko
2021-11-07 16:36 ` [PATCH v2 bpf-next 0/9] Fix leaks in libbpf and selftests Hengqi Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.