bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems
@ 2020-04-28  4:46 Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 1/6] selftests/bpf: ensure test flavors use correct skeletons Andrii Nakryiko
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Add ASAN flavor for test_progs and fix all found memory leaks and other memory
use problems.

Andrii Nakryiko (6):
  selftests/bpf: ensure test flavors use correct skeletons
  selftests/bpf: add test_progs-asan flavor with AddressSantizer
  selftests/bpf: convert test_hashmap into test_progs test
  libbpf: fix memory leak and possible double-free in hashmap__clear
  selftests/bpf: fix memory leak in test selector
  selftests/bpf: fix memory leak in extract_build_id()

 tools/lib/bpf/hashmap.c                       |   7 +
 tools/testing/selftests/bpf/.gitignore        |   3 +-
 tools/testing/selftests/bpf/Makefile          |  20 +-
 .../{test_hashmap.c => prog_tests/hashmap.c}  | 280 +++++++++---------
 tools/testing/selftests/bpf/test_progs.c      |  21 +-
 5 files changed, 179 insertions(+), 152 deletions(-)
 rename tools/testing/selftests/bpf/{test_hashmap.c => prog_tests/hashmap.c} (53%)

-- 
2.24.1


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

* [PATCH bpf-next 1/6] selftests/bpf: ensure test flavors use correct skeletons
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer Andrii Nakryiko
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Ensure that test runner flavors include their own skeletons from <flavor>/
directory. Previously, skeletons generated for no-flavor test_progs were used.
Apart from fixing correctness, this also makes it possible to compile only
flavors individually:

  $ make clean && make test_progs-no_alu32
  ... now succeeds ...

Fixes: 74b5a5968fe8 ("selftests/bpf: Replace test_progs and test_maps w/ general rule")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 7729892e0b04..fd56e31a5b4f 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -323,7 +323,7 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:			\
 		      $(TRUNNER_BPF_SKELS)				\
 		      $$(BPFOBJ) | $(TRUNNER_OUTPUT)
 	$$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
-	cd $$(@D) && $$(CC) $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
+	cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
 
 $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 		       %.c						\
-- 
2.24.1


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

* [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 1/6] selftests/bpf: ensure test flavors use correct skeletons Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  2020-04-28 16:44   ` Alexei Starovoitov
  2020-04-28  4:46 ` [PATCH bpf-next 3/6] selftests/bpf: convert test_hashmap into test_progs test Andrii Nakryiko
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Julia Kartseva

Add another flavor of test_progs that is compiled and run with
AddressSanitizer and LeakSanitizer. This allows to find potential memory
correction bugs and memory leaks. Due to sometimes not trivial requirements on
the environment, this is (for now) done as a separate flavor, not by default.
Eventually I hope to enable it by default.

To run ./test_progs-asan successfully, you need to have libasan installed in
the system, where version of the package depends on GCC version you have.
E.g., GCC8 needs libasan5, while GCC7 uses libasan4.

For CentOS 7, to build everything successfully one would need to:
  $ sudo yum install devtoolset-8-gcc devtoolset-libasan-devel

For Arch Linux to run selftests, one would need to install gcc-libs package to
get libasan.so.5:
  $ sudo pacman -S gcc-libs

Cc: Julia Kartseva <hex@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/.gitignore |  1 +
 tools/testing/selftests/bpf/Makefile   | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index c30079c86998..69b545ca51b8 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -36,6 +36,7 @@ test_current_pid_tgid_new_ns
 xdping
 test_cpp
 *.skel.h
+/asan
 /no_alu32
 /bpf_gcc
 /tools
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index fd56e31a5b4f..e54d069b27a6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -33,7 +33,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
-	test_progs-no_alu32 \
+	test_progs-no_alu32 test_progs-asan \
 	test_current_pid_tgid_new_ns
 
 # Also test bpf-gcc, if present
@@ -344,7 +344,8 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
 			     $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)		\
 			     | $(TRUNNER_BINARY)-extras
 	$$(call msg,BINARY,,$$@)
-	$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
+	$$(CC) $$(CFLAGS) $(TRUNNER_SAN_CFLAGS) $$(filter %.a %.o,$$^)	\
+	       $$(LDLIBS) -o $$@
 
 endef
 
@@ -358,11 +359,18 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
 TRUNNER_BPF_LDFLAGS := -mattr=+alu32
+TRUNNER_SAN_CFLAGS :=
 $(eval $(call DEFINE_TEST_RUNNER,test_progs))
 
+# Define test_progs-asan test runner.
+TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
+TRUNNER_SAN_CFLAGS := -fsanitize=address
+$(eval $(call DEFINE_TEST_RUNNER,test_progs,asan))
+
 # Define test_progs-no_alu32 test runner.
 TRUNNER_BPF_BUILD_RULE := CLANG_NOALU32_BPF_BUILD_RULE
 TRUNNER_BPF_LDFLAGS :=
+TRUNNER_SAN_CFLAGS :=
 $(eval $(call DEFINE_TEST_RUNNER,test_progs,no_alu32))
 
 # Define test_progs BPF-GCC-flavored test runner.
@@ -370,6 +378,7 @@ ifneq ($(BPF_GCC),)
 TRUNNER_BPF_BUILD_RULE := GCC_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(call get_sys_includes,gcc)
 TRUNNER_BPF_LDFLAGS :=
+TRUNNER_SAN_CFLAGS :=
 $(eval $(call DEFINE_TEST_RUNNER,test_progs,bpf_gcc))
 endif
 
@@ -381,6 +390,7 @@ TRUNNER_EXTRA_FILES :=
 TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
 TRUNNER_BPF_CFLAGS :=
 TRUNNER_BPF_LDFLAGS :=
+TRUNNER_SAN_CFLAGS :=
 $(eval $(call DEFINE_TEST_RUNNER,test_maps))
 
 # Define test_verifier test runner.
@@ -406,4 +416,4 @@ $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)			\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
 	feature								\
-	$(addprefix $(OUTPUT)/,*.o *.skel.h no_alu32 bpf_gcc)
+	$(addprefix $(OUTPUT)/,*.o *.skel.h asan no_alu32 bpf_gcc)
-- 
2.24.1


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

* [PATCH bpf-next 3/6] selftests/bpf: convert test_hashmap into test_progs test
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 1/6] selftests/bpf: ensure test flavors use correct skeletons Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 4/6] libbpf: fix memory leak and possible double-free in hashmap__clear Andrii Nakryiko
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Fold stand-alone test_hashmap test into test_progs.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/.gitignore        |   2 -
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../{test_hashmap.c => prog_tests/hashmap.c}  | 280 +++++++++---------
 3 files changed, 140 insertions(+), 144 deletions(-)
 rename tools/testing/selftests/bpf/{test_hashmap.c => prog_tests/hashmap.c} (53%)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 69b545ca51b8..db2e142c7635 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -30,8 +30,6 @@ test_tcpnotify_user
 test_libbpf
 test_tcp_check_syncookie_user
 test_sysctl
-test_hashmap
-test_btf_dump
 test_current_pid_tgid_new_ns
 xdping
 test_cpp
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e54d069b27a6..d20405e2fc77 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -32,7 +32,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
 	test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage \
-	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
+	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
 	test_progs-no_alu32 test_progs-asan \
 	test_current_pid_tgid_new_ns
 
diff --git a/tools/testing/selftests/bpf/test_hashmap.c b/tools/testing/selftests/bpf/prog_tests/hashmap.c
similarity index 53%
rename from tools/testing/selftests/bpf/test_hashmap.c
rename to tools/testing/selftests/bpf/prog_tests/hashmap.c
index c490e012c23f..428d488830c6 100644
--- a/tools/testing/selftests/bpf/test_hashmap.c
+++ b/tools/testing/selftests/bpf/prog_tests/hashmap.c
@@ -5,26 +5,17 @@
  *
  * Copyright (c) 2019 Facebook
  */
-#include <stdio.h>
-#include <errno.h>
-#include <linux/err.h>
+#include "test_progs.h"
 #include "bpf/hashmap.h"
 
-#define CHECK(condition, format...) ({					\
-	int __ret = !!(condition);					\
-	if (__ret) {							\
-		fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);	\
-		fprintf(stderr, format);				\
-	}								\
-	__ret;								\
-})
+static int duration = 0;
 
-size_t hash_fn(const void *k, void *ctx)
+static size_t hash_fn(const void *k, void *ctx)
 {
 	return (long)k;
 }
 
-bool equal_fn(const void *a, const void *b, void *ctx)
+static bool equal_fn(const void *a, const void *b, void *ctx)
 {
 	return (long)a == (long)b;
 }
@@ -49,53 +40,55 @@ static inline size_t exp_cap(size_t sz)
 
 #define ELEM_CNT 62
 
-int test_hashmap_generic(void)
+static void test_hashmap_generic(void)
 {
 	struct hashmap_entry *entry, *tmp;
 	int err, bkt, found_cnt, i;
 	long long found_msk;
 	struct hashmap *map;
 
-	fprintf(stderr, "%s: ", __func__);
-
 	map = hashmap__new(hash_fn, equal_fn, NULL);
-	if (CHECK(IS_ERR(map), "failed to create map: %ld\n", PTR_ERR(map)))
-		return 1;
+	if (CHECK(IS_ERR(map), "hashmap__new",
+		  "failed to create map: %ld\n", PTR_ERR(map)))
+		return;
 
 	for (i = 0; i < ELEM_CNT; i++) {
 		const void *oldk, *k = (const void *)(long)i;
 		void *oldv, *v = (void *)(long)(1024 + i);
 
 		err = hashmap__update(map, k, v, &oldk, &oldv);
-		if (CHECK(err != -ENOENT, "unexpected result: %d\n", err))
-			return 1;
+		if (CHECK(err != -ENOENT, "hashmap__update",
+			  "unexpected result: %d\n", err))
+			goto cleanup;
 
 		if (i % 2) {
 			err = hashmap__add(map, k, v);
 		} else {
 			err = hashmap__set(map, k, v, &oldk, &oldv);
-			if (CHECK(oldk != NULL || oldv != NULL,
+			if (CHECK(oldk != NULL || oldv != NULL, "check_kv",
 				  "unexpected k/v: %p=%p\n", oldk, oldv))
-				return 1;
+				goto cleanup;
 		}
 
-		if (CHECK(err, "failed to add k/v %ld = %ld: %d\n",
+		if (CHECK(err, "elem_add", "failed to add k/v %ld = %ld: %d\n",
 			       (long)k, (long)v, err))
-			return 1;
+			goto cleanup;
 
-		if (CHECK(!hashmap__find(map, k, &oldv),
+		if (CHECK(!hashmap__find(map, k, &oldv), "elem_find",
 			  "failed to find key %ld\n", (long)k))
-			return 1;
-		if (CHECK(oldv != v, "found value is wrong: %ld\n", (long)oldv))
-			return 1;
+			goto cleanup;
+		if (CHECK(oldv != v, "elem_val",
+			  "found value is wrong: %ld\n", (long)oldv))
+			goto cleanup;
 	}
 
-	if (CHECK(hashmap__size(map) != ELEM_CNT,
+	if (CHECK(hashmap__size(map) != ELEM_CNT, "hashmap__size",
 		  "invalid map size: %zu\n", hashmap__size(map)))
-		return 1;
+		goto cleanup;
 	if (CHECK(hashmap__capacity(map) != exp_cap(hashmap__size(map)),
+		  "hashmap_cap",
 		  "unexpected map capacity: %zu\n", hashmap__capacity(map)))
-		return 1;
+		goto cleanup;
 
 	found_msk = 0;
 	hashmap__for_each_entry(map, entry, bkt) {
@@ -103,42 +96,47 @@ int test_hashmap_generic(void)
 		long v = (long)entry->value;
 
 		found_msk |= 1ULL << k;
-		if (CHECK(v - k != 1024, "invalid k/v pair: %ld = %ld\n", k, v))
-			return 1;
+		if (CHECK(v - k != 1024, "check_kv",
+			  "invalid k/v pair: %ld = %ld\n", k, v))
+			goto cleanup;
 	}
-	if (CHECK(found_msk != (1ULL << ELEM_CNT) - 1,
+	if (CHECK(found_msk != (1ULL << ELEM_CNT) - 1, "elem_cnt",
 		  "not all keys iterated: %llx\n", found_msk))
-		return 1;
+		goto cleanup;
 
 	for (i = 0; i < ELEM_CNT; i++) {
 		const void *oldk, *k = (const void *)(long)i;
 		void *oldv, *v = (void *)(long)(256 + i);
 
 		err = hashmap__add(map, k, v);
-		if (CHECK(err != -EEXIST, "unexpected add result: %d\n", err))
-			return 1;
+		if (CHECK(err != -EEXIST, "hashmap__add",
+			  "unexpected add result: %d\n", err))
+			goto cleanup;
 
 		if (i % 2)
 			err = hashmap__update(map, k, v, &oldk, &oldv);
 		else
 			err = hashmap__set(map, k, v, &oldk, &oldv);
 
-		if (CHECK(err, "failed to update k/v %ld = %ld: %d\n",
-			       (long)k, (long)v, err))
-			return 1;
-		if (CHECK(!hashmap__find(map, k, &oldv),
+		if (CHECK(err, "elem_upd",
+			  "failed to update k/v %ld = %ld: %d\n",
+			  (long)k, (long)v, err))
+			goto cleanup;
+		if (CHECK(!hashmap__find(map, k, &oldv), "elem_find",
 			  "failed to find key %ld\n", (long)k))
-			return 1;
-		if (CHECK(oldv != v, "found value is wrong: %ld\n", (long)oldv))
-			return 1;
+			goto cleanup;
+		if (CHECK(oldv != v, "elem_val",
+			  "found value is wrong: %ld\n", (long)oldv))
+			goto cleanup;
 	}
 
-	if (CHECK(hashmap__size(map) != ELEM_CNT,
+	if (CHECK(hashmap__size(map) != ELEM_CNT, "hashmap__size",
 		  "invalid updated map size: %zu\n", hashmap__size(map)))
-		return 1;
+		goto cleanup;
 	if (CHECK(hashmap__capacity(map) != exp_cap(hashmap__size(map)),
+		  "hashmap__capacity",
 		  "unexpected map capacity: %zu\n", hashmap__capacity(map)))
-		return 1;
+		goto cleanup;
 
 	found_msk = 0;
 	hashmap__for_each_entry_safe(map, entry, tmp, bkt) {
@@ -146,20 +144,21 @@ int test_hashmap_generic(void)
 		long v = (long)entry->value;
 
 		found_msk |= 1ULL << k;
-		if (CHECK(v - k != 256,
+		if (CHECK(v - k != 256, "elem_check",
 			  "invalid updated k/v pair: %ld = %ld\n", k, v))
-			return 1;
+			goto cleanup;
 	}
-	if (CHECK(found_msk != (1ULL << ELEM_CNT) - 1,
+	if (CHECK(found_msk != (1ULL << ELEM_CNT) - 1, "elem_cnt",
 		  "not all keys iterated after update: %llx\n", found_msk))
-		return 1;
+		goto cleanup;
 
 	found_cnt = 0;
 	hashmap__for_each_key_entry(map, entry, (void *)0) {
 		found_cnt++;
 	}
-	if (CHECK(!found_cnt, "didn't find any entries for key 0\n"))
-		return 1;
+	if (CHECK(!found_cnt, "found_cnt",
+		  "didn't find any entries for key 0\n"))
+		goto cleanup;
 
 	found_msk = 0;
 	found_cnt = 0;
@@ -173,30 +172,31 @@ int test_hashmap_generic(void)
 		found_cnt++;
 		found_msk |= 1ULL << (long)k;
 
-		if (CHECK(!hashmap__delete(map, k, &oldk, &oldv),
+		if (CHECK(!hashmap__delete(map, k, &oldk, &oldv), "elem_del",
 			  "failed to delete k/v %ld = %ld\n",
 			  (long)k, (long)v))
-			return 1;
-		if (CHECK(oldk != k || oldv != v,
+			goto cleanup;
+		if (CHECK(oldk != k || oldv != v, "check_old",
 			  "invalid deleted k/v: expected %ld = %ld, got %ld = %ld\n",
 			  (long)k, (long)v, (long)oldk, (long)oldv))
-			return 1;
-		if (CHECK(hashmap__delete(map, k, &oldk, &oldv),
+			goto cleanup;
+		if (CHECK(hashmap__delete(map, k, &oldk, &oldv), "elem_del",
 			  "unexpectedly deleted k/v %ld = %ld\n",
 			  (long)oldk, (long)oldv))
-			return 1;
+			goto cleanup;
 	}
 
-	if (CHECK(!found_cnt || !found_msk,
+	if (CHECK(!found_cnt || !found_msk, "found_entries",
 		  "didn't delete any key entries\n"))
-		return 1;
-	if (CHECK(hashmap__size(map) != ELEM_CNT - found_cnt,
+		goto cleanup;
+	if (CHECK(hashmap__size(map) != ELEM_CNT - found_cnt, "elem_cnt",
 		  "invalid updated map size (already deleted: %d): %zu\n",
 		  found_cnt, hashmap__size(map)))
-		return 1;
+		goto cleanup;
 	if (CHECK(hashmap__capacity(map) != exp_cap(hashmap__size(map)),
+		  "hashmap__capacity",
 		  "unexpected map capacity: %zu\n", hashmap__capacity(map)))
-		return 1;
+		goto cleanup;
 
 	hashmap__for_each_entry_safe(map, entry, tmp, bkt) {
 		const void *oldk, *k;
@@ -208,53 +208,56 @@ int test_hashmap_generic(void)
 		found_cnt++;
 		found_msk |= 1ULL << (long)k;
 
-		if (CHECK(!hashmap__delete(map, k, &oldk, &oldv),
+		if (CHECK(!hashmap__delete(map, k, &oldk, &oldv), "elem_del",
 			  "failed to delete k/v %ld = %ld\n",
 			  (long)k, (long)v))
-			return 1;
-		if (CHECK(oldk != k || oldv != v,
+			goto cleanup;
+		if (CHECK(oldk != k || oldv != v, "elem_check",
 			  "invalid old k/v: expect %ld = %ld, got %ld = %ld\n",
 			  (long)k, (long)v, (long)oldk, (long)oldv))
-			return 1;
-		if (CHECK(hashmap__delete(map, k, &oldk, &oldv),
+			goto cleanup;
+		if (CHECK(hashmap__delete(map, k, &oldk, &oldv), "elem_del",
 			  "unexpectedly deleted k/v %ld = %ld\n",
 			  (long)k, (long)v))
-			return 1;
+			goto cleanup;
 	}
 
 	if (CHECK(found_cnt != ELEM_CNT || found_msk != (1ULL << ELEM_CNT) - 1,
+		  "found_cnt",
 		  "not all keys were deleted: found_cnt:%d, found_msk:%llx\n",
 		  found_cnt, found_msk))
-		return 1;
-	if (CHECK(hashmap__size(map) != 0,
+		goto cleanup;
+	if (CHECK(hashmap__size(map) != 0, "hashmap__size",
 		  "invalid updated map size (already deleted: %d): %zu\n",
 		  found_cnt, hashmap__size(map)))
-		return 1;
+		goto cleanup;
 
 	found_cnt = 0;
 	hashmap__for_each_entry(map, entry, bkt) {
-		CHECK(false, "unexpected map entries left: %ld = %ld\n",
-			     (long)entry->key, (long)entry->value);
-		return 1;
+		CHECK(false, "elem_exists",
+		      "unexpected map entries left: %ld = %ld\n",
+		      (long)entry->key, (long)entry->value);
+		goto cleanup;
 	}
 
-	hashmap__free(map);
+	hashmap__clear(map);
 	hashmap__for_each_entry(map, entry, bkt) {
-		CHECK(false, "unexpected map entries left: %ld = %ld\n",
-			     (long)entry->key, (long)entry->value);
-		return 1;
+		CHECK(false, "elem_exists",
+		      "unexpected map entries left: %ld = %ld\n",
+		      (long)entry->key, (long)entry->value);
+		goto cleanup;
 	}
 
-	fprintf(stderr, "OK\n");
-	return 0;
+cleanup:
+	hashmap__free(map);
 }
 
-size_t collision_hash_fn(const void *k, void *ctx)
+static size_t collision_hash_fn(const void *k, void *ctx)
 {
 	return 0;
 }
 
-int test_hashmap_multimap(void)
+static void test_hashmap_multimap(void)
 {
 	void *k1 = (void *)0, *k2 = (void *)1;
 	struct hashmap_entry *entry;
@@ -262,121 +265,116 @@ int test_hashmap_multimap(void)
 	long found_msk;
 	int err, bkt;
 
-	fprintf(stderr, "%s: ", __func__);
-
 	/* force collisions */
 	map = hashmap__new(collision_hash_fn, equal_fn, NULL);
-	if (CHECK(IS_ERR(map), "failed to create map: %ld\n", PTR_ERR(map)))
-		return 1;
-
+	if (CHECK(IS_ERR(map), "hashmap__new",
+		  "failed to create map: %ld\n", PTR_ERR(map)))
+		return;
 
 	/* set up multimap:
 	 * [0] -> 1, 2, 4;
 	 * [1] -> 8, 16, 32;
 	 */
 	err = hashmap__append(map, k1, (void *)1);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 	err = hashmap__append(map, k1, (void *)2);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 	err = hashmap__append(map, k1, (void *)4);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 
 	err = hashmap__append(map, k2, (void *)8);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 	err = hashmap__append(map, k2, (void *)16);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 	err = hashmap__append(map, k2, (void *)32);
-	if (CHECK(err, "failed to add k/v: %d\n", err))
-		return 1;
+	if (CHECK(err, "elem_add", "failed to add k/v: %d\n", err))
+		goto cleanup;
 
-	if (CHECK(hashmap__size(map) != 6,
+	if (CHECK(hashmap__size(map) != 6, "hashmap_size",
 		  "invalid map size: %zu\n", hashmap__size(map)))
-		return 1;
+		goto cleanup;
 
 	/* verify global iteration still works and sees all values */
 	found_msk = 0;
 	hashmap__for_each_entry(map, entry, bkt) {
 		found_msk |= (long)entry->value;
 	}
-	if (CHECK(found_msk != (1 << 6) - 1,
+	if (CHECK(found_msk != (1 << 6) - 1, "found_msk",
 		  "not all keys iterated: %lx\n", found_msk))
-		return 1;
+		goto cleanup;
 
 	/* iterate values for key 1 */
 	found_msk = 0;
 	hashmap__for_each_key_entry(map, entry, k1) {
 		found_msk |= (long)entry->value;
 	}
-	if (CHECK(found_msk != (1 | 2 | 4),
+	if (CHECK(found_msk != (1 | 2 | 4), "found_msk",
 		  "invalid k1 values: %lx\n", found_msk))
-		return 1;
+		goto cleanup;
 
 	/* iterate values for key 2 */
 	found_msk = 0;
 	hashmap__for_each_key_entry(map, entry, k2) {
 		found_msk |= (long)entry->value;
 	}
-	if (CHECK(found_msk != (8 | 16 | 32),
+	if (CHECK(found_msk != (8 | 16 | 32), "found_msk",
 		  "invalid k2 values: %lx\n", found_msk))
-		return 1;
+		goto cleanup;
 
-	fprintf(stderr, "OK\n");
-	return 0;
+cleanup:
+	hashmap__free(map);
 }
 
-int test_hashmap_empty()
+static void test_hashmap_empty()
 {
 	struct hashmap_entry *entry;
 	int bkt;
 	struct hashmap *map;
 	void *k = (void *)0;
 
-	fprintf(stderr, "%s: ", __func__);
-
 	/* force collisions */
 	map = hashmap__new(hash_fn, equal_fn, NULL);
-	if (CHECK(IS_ERR(map), "failed to create map: %ld\n", PTR_ERR(map)))
-		return 1;
+	if (CHECK(IS_ERR(map), "hashmap__new",
+		  "failed to create map: %ld\n", PTR_ERR(map)))
+		goto cleanup;
 
-	if (CHECK(hashmap__size(map) != 0,
+	if (CHECK(hashmap__size(map) != 0, "hashmap__size",
 		  "invalid map size: %zu\n", hashmap__size(map)))
-		return 1;
-	if (CHECK(hashmap__capacity(map) != 0,
+		goto cleanup;
+	if (CHECK(hashmap__capacity(map) != 0, "hashmap__capacity",
 		  "invalid map capacity: %zu\n", hashmap__capacity(map)))
-		return 1;
-	if (CHECK(hashmap__find(map, k, NULL), "unexpected find\n"))
-		return 1;
-	if (CHECK(hashmap__delete(map, k, NULL, NULL), "unexpected delete\n"))
-		return 1;
+		goto cleanup;
+	if (CHECK(hashmap__find(map, k, NULL), "elem_find",
+		  "unexpected find\n"))
+		goto cleanup;
+	if (CHECK(hashmap__delete(map, k, NULL, NULL), "elem_del",
+		  "unexpected delete\n"))
+		goto cleanup;
 
 	hashmap__for_each_entry(map, entry, bkt) {
-		CHECK(false, "unexpected iterated entry\n");
-		return 1;
+		CHECK(false, "elem_found", "unexpected iterated entry\n");
+		goto cleanup;
 	}
 	hashmap__for_each_key_entry(map, entry, k) {
-		CHECK(false, "unexpected key entry\n");
-		return 1;
+		CHECK(false, "key_found", "unexpected key entry\n");
+		goto cleanup;
 	}
 
-	fprintf(stderr, "OK\n");
-	return 0;
+cleanup:
+	hashmap__free(map);
 }
 
-int main(int argc, char **argv)
+void test_hashmap()
 {
-	bool failed = false;
-
-	if (test_hashmap_generic())
-		failed = true;
-	if (test_hashmap_multimap())
-		failed = true;
-	if (test_hashmap_empty())
-		failed = true;
-
-	return failed;
+	if (test__start_subtest("generic"))
+		test_hashmap_generic();
+	if (test__start_subtest("multimap"))
+		test_hashmap_multimap();
+	if (test__start_subtest("empty"))
+		test_hashmap_empty();
 }
-- 
2.24.1


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

* [PATCH bpf-next 4/6] libbpf: fix memory leak and possible double-free in hashmap__clear
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2020-04-28  4:46 ` [PATCH bpf-next 3/6] selftests/bpf: convert test_hashmap into test_progs test Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 5/6] selftests/bpf: fix memory leak in test selector Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 6/6] selftests/bpf: fix memory leak in extract_build_id() Andrii Nakryiko
  5 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Alston Tang

Fix memory leak in hashmap_clear() not freeing hashmap_entry structs for each
of the remaining entries. Also NULL-out bucket list to prevent possible
double-free between hashmap__clear() and hashmap__free().

Running test_progs-asan flavor clearly showed this problem.

Cc: Alston Tang <alston64@fb.com>
Reported-by: Alston Tang <alston64@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/hashmap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c
index 54c30c802070..cffb96202e0d 100644
--- a/tools/lib/bpf/hashmap.c
+++ b/tools/lib/bpf/hashmap.c
@@ -59,7 +59,14 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
 
 void hashmap__clear(struct hashmap *map)
 {
+	struct hashmap_entry *cur, *tmp;
+	int bkt;
+
+	hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
+		free(cur);
+	}
 	free(map->buckets);
+	map->buckets = NULL;
 	map->cap = map->cap_bits = map->sz = 0;
 }
 
-- 
2.24.1


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

* [PATCH bpf-next 5/6] selftests/bpf: fix memory leak in test selector
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2020-04-28  4:46 ` [PATCH bpf-next 4/6] libbpf: fix memory leak and possible double-free in hashmap__clear Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  2020-04-28  4:46 ` [PATCH bpf-next 6/6] selftests/bpf: fix memory leak in extract_build_id() Andrii Nakryiko
  5 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Free test selector substrings, which were strdup()'ed.

Fixes: b65053cd94f4 ("selftests/bpf: Add whitelist/blacklist of test names to test_progs")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index b521e0a512b6..86d0020c9eec 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -420,6 +420,18 @@ static int libbpf_print_fn(enum libbpf_print_level level,
 	return 0;
 }
 
+static void free_str_set(const struct str_set *set)
+{
+	int i;
+
+	if (!set)
+		return;
+
+	for (i = 0; i < set->cnt; i++)
+		free((void *)set->strs[i]);
+	free(set->strs);
+}
+
 static int parse_str_list(const char *s, struct str_set *set)
 {
 	char *input, *state = NULL, *next, **tmp, **strs = NULL;
@@ -756,11 +768,11 @@ int main(int argc, char **argv)
 	fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
 		env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
 
-	free(env.test_selector.blacklist.strs);
-	free(env.test_selector.whitelist.strs);
+	free_str_set(&env.test_selector.blacklist);
+	free_str_set(&env.test_selector.whitelist);
 	free(env.test_selector.num_set);
-	free(env.subtest_selector.blacklist.strs);
-	free(env.subtest_selector.whitelist.strs);
+	free_str_set(&env.subtest_selector.blacklist);
+	free_str_set(&env.subtest_selector.whitelist);
 	free(env.subtest_selector.num_set);
 
 	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
-- 
2.24.1


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

* [PATCH bpf-next 6/6] selftests/bpf: fix memory leak in extract_build_id()
  2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2020-04-28  4:46 ` [PATCH bpf-next 5/6] selftests/bpf: fix memory leak in test selector Andrii Nakryiko
@ 2020-04-28  4:46 ` Andrii Nakryiko
  5 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28  4:46 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Song Liu

getline() allocates string, which has to be freed.

Cc: Song Liu <songliubraving@fb.com>
Fixes: 81f77fd0deeb ("bpf: add selftest for stackmap with BPF_F_STACK_BUILD_ID")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 86d0020c9eec..93970ec1c9e9 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -351,6 +351,7 @@ int extract_build_id(char *build_id, size_t size)
 		len = size;
 	memcpy(build_id, line, len);
 	build_id[len] = '\0';
+	free(line);
 	return 0;
 err:
 	fclose(fp);
-- 
2.24.1


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

* Re: [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer
  2020-04-28  4:46 ` [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer Andrii Nakryiko
@ 2020-04-28 16:44   ` Alexei Starovoitov
  2020-04-28 18:35     ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2020-04-28 16:44 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team, Julia Kartseva

On Mon, Apr 27, 2020 at 09:46:24PM -0700, Andrii Nakryiko wrote:
> Add another flavor of test_progs that is compiled and run with
> AddressSanitizer and LeakSanitizer. This allows to find potential memory
> correction bugs and memory leaks. Due to sometimes not trivial requirements on
> the environment, this is (for now) done as a separate flavor, not by default.
> Eventually I hope to enable it by default.
> 
> To run ./test_progs-asan successfully, you need to have libasan installed in
> the system, where version of the package depends on GCC version you have.
> E.g., GCC8 needs libasan5, while GCC7 uses libasan4.
> 
> For CentOS 7, to build everything successfully one would need to:
>   $ sudo yum install devtoolset-8-gcc devtoolset-libasan-devel
> 
> For Arch Linux to run selftests, one would need to install gcc-libs package to
> get libasan.so.5:
>   $ sudo pacman -S gcc-libs
> 
> Cc: Julia Kartseva <hex@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

It needs a feature check.
selftest shouldn't be forcing asan on everyone.
Even after I did:
sudo yum install devtoolset-8-libasan-devel
it still failed to build:
  BINARY   test_progs-asan
/opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find libasan_preinit.o: No such file or directory
/opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find -lasan

Also I really don't like that skeletons are now built three times for now good reason
  GEN-SKEL [test_progs-asan] test_stack_map.skel.h
  GEN-SKEL [test_progs-asan] test_core_reloc_nesting.skel.h
default vs no_alu32 makes sense. They are different bpf.o files and different skeletons,
but for asan there is no such need.

Please resubmit the rest of the patches, since asan isn't a prerequisite.

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

* Re: [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer
  2020-04-28 16:44   ` Alexei Starovoitov
@ 2020-04-28 18:35     ` Andrii Nakryiko
  2020-04-28 20:41       ` Alexei Starovoitov
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28 18:35 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Julia Kartseva

On Tue, Apr 28, 2020 at 9:44 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Apr 27, 2020 at 09:46:24PM -0700, Andrii Nakryiko wrote:
> > Add another flavor of test_progs that is compiled and run with
> > AddressSanitizer and LeakSanitizer. This allows to find potential memory
> > correction bugs and memory leaks. Due to sometimes not trivial requirements on
> > the environment, this is (for now) done as a separate flavor, not by default.
> > Eventually I hope to enable it by default.
> >
> > To run ./test_progs-asan successfully, you need to have libasan installed in
> > the system, where version of the package depends on GCC version you have.
> > E.g., GCC8 needs libasan5, while GCC7 uses libasan4.
> >
> > For CentOS 7, to build everything successfully one would need to:
> >   $ sudo yum install devtoolset-8-gcc devtoolset-libasan-devel
> >
> > For Arch Linux to run selftests, one would need to install gcc-libs package to
> > get libasan.so.5:
> >   $ sudo pacman -S gcc-libs
> >
> > Cc: Julia Kartseva <hex@fb.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> It needs a feature check.
> selftest shouldn't be forcing asan on everyone.
> Even after I did:
> sudo yum install devtoolset-8-libasan-devel
> it still failed to build:
>   BINARY   test_progs-asan
> /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find libasan_preinit.o: No such file or directory
> /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find -lasan
>

Yeah, it worked for me initially because it still used GCC7 locally
and older version of libasan.

On CentOS you have to run the following command to set up environment
(for current session only, though):

$ scl enable devtoolset-8 bash

What it does:
- adds /opt/rh/devtoolset-8/root/usr/bin to $PATH
- sets $LD_LIBRARY_PATH to
/opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib:/opt/rh/devtoolset-8/root/usr/lib64/dyninst:/opt/rh/devtoolset-8/root/usr/lib/dyninst:/opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib

I'm going to add this to patch to ease some pain later. But yeah, I
think I have a better plan for ASAN builds. I'll add EXTRA_CFLAGS to
selftests Makefile, defaulted to nothing. Then for Travis CI (or
locally) one would do:

$ make EXTRA_CFLAGS='-fsanitize-address'

to build ASAN versions of all the same test runners (including
test_verifier, test_maps, etc).

I think this will be better overall.

> Also I really don't like that skeletons are now built three times for now good reason
>   GEN-SKEL [test_progs-asan] test_stack_map.skel.h
>   GEN-SKEL [test_progs-asan] test_core_reloc_nesting.skel.h
> default vs no_alu32 makes sense. They are different bpf.o files and different skeletons,
> but for asan there is no such need.

I agree, luckily I don't really have to change anything with the above approach.

>
> Please resubmit the rest of the patches, since asan isn't a prerequisite.

I'll update this patch to just add EXTRA_CFLAGS, if you are ok with
this (and will leave instructions on installing libasan).

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

* Re: [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer
  2020-04-28 18:35     ` Andrii Nakryiko
@ 2020-04-28 20:41       ` Alexei Starovoitov
  2020-04-28 22:13         ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2020-04-28 20:41 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Julia Kartseva

On Tue, Apr 28, 2020 at 11:35:15AM -0700, Andrii Nakryiko wrote:
> On Tue, Apr 28, 2020 at 9:44 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 09:46:24PM -0700, Andrii Nakryiko wrote:
> > > Add another flavor of test_progs that is compiled and run with
> > > AddressSanitizer and LeakSanitizer. This allows to find potential memory
> > > correction bugs and memory leaks. Due to sometimes not trivial requirements on
> > > the environment, this is (for now) done as a separate flavor, not by default.
> > > Eventually I hope to enable it by default.
> > >
> > > To run ./test_progs-asan successfully, you need to have libasan installed in
> > > the system, where version of the package depends on GCC version you have.
> > > E.g., GCC8 needs libasan5, while GCC7 uses libasan4.
> > >
> > > For CentOS 7, to build everything successfully one would need to:
> > >   $ sudo yum install devtoolset-8-gcc devtoolset-libasan-devel
> > >
> > > For Arch Linux to run selftests, one would need to install gcc-libs package to
> > > get libasan.so.5:
> > >   $ sudo pacman -S gcc-libs
> > >
> > > Cc: Julia Kartseva <hex@fb.com>
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >
> > It needs a feature check.
> > selftest shouldn't be forcing asan on everyone.
> > Even after I did:
> > sudo yum install devtoolset-8-libasan-devel
> > it still failed to build:
> >   BINARY   test_progs-asan
> > /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find libasan_preinit.o: No such file or directory
> > /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find -lasan
> >
> 
> Yeah, it worked for me initially because it still used GCC7 locally
> and older version of libasan.
> 
> On CentOS you have to run the following command to set up environment
> (for current session only, though):
> 
> $ scl enable devtoolset-8 bash
> 
> What it does:
> - adds /opt/rh/devtoolset-8/root/usr/bin to $PATH
> - sets $LD_LIBRARY_PATH to
> /opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib:/opt/rh/devtoolset-8/root/usr/lib64/dyninst:/opt/rh/devtoolset-8/root/usr/lib/dyninst:/opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib

I don't want to do this, since I prefer gcc9 for my builds since it has better warnings.
But yum cannot find devtoolset-9-libasan-devel it seems.

> I'm going to add this to patch to ease some pain later. But yeah, I
> think I have a better plan for ASAN builds. I'll add EXTRA_CFLAGS to
> selftests Makefile, defaulted to nothing. Then for Travis CI (or
> locally) one would do:
> 
> $ make EXTRA_CFLAGS='-fsanitize-address'
> 
> to build ASAN versions of all the same test runners (including
> test_verifier, test_maps, etc).
> 
> I think this will be better overall.
> 
> > Also I really don't like that skeletons are now built three times for now good reason
> >   GEN-SKEL [test_progs-asan] test_stack_map.skel.h
> >   GEN-SKEL [test_progs-asan] test_core_reloc_nesting.skel.h
> > default vs no_alu32 makes sense. They are different bpf.o files and different skeletons,
> > but for asan there is no such need.
> 
> I agree, luckily I don't really have to change anything with the above approach.
> 
> >
> > Please resubmit the rest of the patches, since asan isn't a prerequisite.
> 
> I'll update this patch to just add EXTRA_CFLAGS, if you are ok with
> this (and will leave instructions on installing libasan).

yeah. EXTRA_CFLAGS approach should work.
That will build both test_progs and test_progs-no_alu32 with libasan ?
That would be ideal.

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

* Re: [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer
  2020-04-28 20:41       ` Alexei Starovoitov
@ 2020-04-28 22:13         ` Andrii Nakryiko
  0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-04-28 22:13 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Julia Kartseva

On Tue, Apr 28, 2020 at 1:41 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 11:35:15AM -0700, Andrii Nakryiko wrote:
> > On Tue, Apr 28, 2020 at 9:44 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Apr 27, 2020 at 09:46:24PM -0700, Andrii Nakryiko wrote:
> > > > Add another flavor of test_progs that is compiled and run with
> > > > AddressSanitizer and LeakSanitizer. This allows to find potential memory
> > > > correction bugs and memory leaks. Due to sometimes not trivial requirements on
> > > > the environment, this is (for now) done as a separate flavor, not by default.
> > > > Eventually I hope to enable it by default.
> > > >
> > > > To run ./test_progs-asan successfully, you need to have libasan installed in
> > > > the system, where version of the package depends on GCC version you have.
> > > > E.g., GCC8 needs libasan5, while GCC7 uses libasan4.
> > > >
> > > > For CentOS 7, to build everything successfully one would need to:
> > > >   $ sudo yum install devtoolset-8-gcc devtoolset-libasan-devel
> > > >
> > > > For Arch Linux to run selftests, one would need to install gcc-libs package to
> > > > get libasan.so.5:
> > > >   $ sudo pacman -S gcc-libs
> > > >
> > > > Cc: Julia Kartseva <hex@fb.com>
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > >
> > > It needs a feature check.
> > > selftest shouldn't be forcing asan on everyone.
> > > Even after I did:
> > > sudo yum install devtoolset-8-libasan-devel
> > > it still failed to build:
> > >   BINARY   test_progs-asan
> > > /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find libasan_preinit.o: No such file or directory
> > > /opt/rh/devtoolset-9/root/usr/libexec/gcc/x86_64-redhat-linux/9/ld: cannot find -lasan
> > >
> >
> > Yeah, it worked for me initially because it still used GCC7 locally
> > and older version of libasan.
> >
> > On CentOS you have to run the following command to set up environment
> > (for current session only, though):
> >
> > $ scl enable devtoolset-8 bash
> >
> > What it does:
> > - adds /opt/rh/devtoolset-8/root/usr/bin to $PATH
> > - sets $LD_LIBRARY_PATH to
> > /opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib:/opt/rh/devtoolset-8/root/usr/lib64/dyninst:/opt/rh/devtoolset-8/root/usr/lib/dyninst:/opt/rh/devtoolset-8/root/usr/lib64:/opt/rh/devtoolset-8/root/usr/lib
>
> I don't want to do this, since I prefer gcc9 for my builds since it has better warnings.
> But yum cannot find devtoolset-9-libasan-devel it seems.

Hmm, strange, there should be devtoolset-9-libasan-devel according to
[0]. No idea.

  [0] https://cbs.centos.org/koji/buildinfo?buildID=27149

>
> > I'm going to add this to patch to ease some pain later. But yeah, I
> > think I have a better plan for ASAN builds. I'll add EXTRA_CFLAGS to
> > selftests Makefile, defaulted to nothing. Then for Travis CI (or
> > locally) one would do:
> >
> > $ make EXTRA_CFLAGS='-fsanitize-address'
> >
> > to build ASAN versions of all the same test runners (including
> > test_verifier, test_maps, etc).
> >
> > I think this will be better overall.
> >
> > > Also I really don't like that skeletons are now built three times for now good reason
> > >   GEN-SKEL [test_progs-asan] test_stack_map.skel.h
> > >   GEN-SKEL [test_progs-asan] test_core_reloc_nesting.skel.h
> > > default vs no_alu32 makes sense. They are different bpf.o files and different skeletons,
> > > but for asan there is no such need.
> >
> > I agree, luckily I don't really have to change anything with the above approach.
> >
> > >
> > > Please resubmit the rest of the patches, since asan isn't a prerequisite.
> >
> > I'll update this patch to just add EXTRA_CFLAGS, if you are ok with
> > this (and will leave instructions on installing libasan).
>
> yeah. EXTRA_CFLAGS approach should work.
> That will build both test_progs and test_progs-no_alu32 with libasan ?
> That would be ideal.

Yep, then everything should be built with ASAN. Ok, will do that then.

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

end of thread, other threads:[~2020-04-28 22:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28  4:46 [PATCH bpf-next 0/6] Add ASAN to selftest and fix found problems Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 1/6] selftests/bpf: ensure test flavors use correct skeletons Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 2/6] selftests/bpf: add test_progs-asan flavor with AddressSantizer Andrii Nakryiko
2020-04-28 16:44   ` Alexei Starovoitov
2020-04-28 18:35     ` Andrii Nakryiko
2020-04-28 20:41       ` Alexei Starovoitov
2020-04-28 22:13         ` Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 3/6] selftests/bpf: convert test_hashmap into test_progs test Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 4/6] libbpf: fix memory leak and possible double-free in hashmap__clear Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 5/6] selftests/bpf: fix memory leak in test selector Andrii Nakryiko
2020-04-28  4:46 ` [PATCH bpf-next 6/6] selftests/bpf: fix memory leak in extract_build_id() Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).