From 76148faa161e7cfb2d7719f35b37d7db4f3f8596 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:01 -0700 Subject: [PATCH 01/11] selftests/bpf: Ensure test flavors use correct skeletons Ensure that test runner flavors include their own skeletons from / 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 Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-2-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 4e654d41c7af..01c95f8278c7 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -324,7 +324,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 \ From 02995dd4bb02a5359a08e44abb3c18c2f456bd19 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:02 -0700 Subject: [PATCH 02/11] selftests/bpf: Add SAN_CFLAGS param to selftests build to allow sanitizers Add ability to specify extra compiler flags with SAN_CFLAGS for compilation of all user-space C files. This allows to build all of selftest programs with, e.g., custom sanitizer flags, without requiring support for such sanitizers from anyone compiling selftest/bpf. As an example, to compile everything with AddressSanitizer, one would do: $ make clean && make SAN_CFLAGS="-fsanitize=address" For AddressSanitizer to work, one needs appropriate libasan shared library installed in the system, with version of libasan matching what GCC links against. 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 $ scl enable devtoolset-8 bash # set up environment For Arch Linux to run selftests, one would need to install gcc-libs package to get libasan.so.5: $ sudo pacman -S gcc-libs N.B. EXTRA_CFLAGS name wasn't used, because it's also used by libbpf's Makefile and this causes few issues: 1. default "-g -Wall" flags are overriden; 2. compiling shared library with AddressSanitizer generates a bunch of symbols like: "_GLOBAL__sub_D_00099_0_btf_dump.c", "_GLOBAL__sub_D_00099_0_bpf.c", etc, which screws up versioned symbols check. Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Cc: Julia Kartseva Link: https://lore.kernel.org/bpf/20200429012111.277390-3-andriin@fb.com --- tools/testing/selftests/bpf/Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 01c95f8278c7..887f06a514ee 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -20,9 +20,10 @@ CLANG ?= clang LLC ?= llc LLVM_OBJCOPY ?= llvm-objcopy BPF_GCC ?= $(shell command -v bpf-gcc;) -CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) -I$(CURDIR) \ - -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) -I$(TOOLSINCDIR) \ - -I$(APIDIR) \ +SAN_CFLAGS ?= +CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS) \ + -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) \ + -I$(TOOLSINCDIR) -I$(APIDIR) \ -Dbpf_prog_load=bpf_prog_test_load \ -Dbpf_load_program=bpf_test_load_program LDLIBS += -lcap -lelf -lz -lrt -lpthread From 42fce2cfb405e613f0355c4f92429d651bf0a5b3 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:03 -0700 Subject: [PATCH 03/11] selftests/bpf: Convert test_hashmap into test_progs test Fold stand-alone test_hashmap test into test_progs. Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-4-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 c30079c86998..16b9774d8b68 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 887f06a514ee..10f12a5aac20 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_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_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 -#include -#include +#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(); } From 229bf8bf4d910510bc1a2fd0b89bd467cd71050d Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:04 -0700 Subject: [PATCH 04/11] libbpf: Fix memory leak and possible double-free in hashmap__clear 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. Reported-by: Alston Tang Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-5-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; } From f25d5416d64c796aa639136eb0b076c8bd579b54 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:05 -0700 Subject: [PATCH 05/11] selftests/bpf: Fix memory leak in test selector 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 Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-6-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; From 9f56bb531a809ecaa7f0ddca61d2cf3adc1cb81a Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:06 -0700 Subject: [PATCH 06/11] selftests/bpf: Fix memory leak in extract_build_id() getline() allocates string, which has to be freed. Fixes: 81f77fd0deeb ("bpf: add selftest for stackmap with BPF_F_STACK_BUILD_ID") Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Cc: Song Liu Link: https://lore.kernel.org/bpf/20200429012111.277390-7-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); From 13c908495e5d51718a6da84ae925fa2aac056380 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:07 -0700 Subject: [PATCH 07/11] selftests/bpf: Fix invalid memory reads in core_relo selftest Another one found by AddressSanitizer. input_len is bigger than actually initialized data size. Fixes: c7566a69695c ("selftests/bpf: Add field existence CO-RE relocs tests") Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-8-andriin@fb.com --- 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 31e177adbdf1..084ed26a7d78 100644 --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c @@ -392,7 +392,7 @@ static struct core_reloc_test_case test_cases[] = { .input = STRUCT_TO_CHAR_PTR(core_reloc_existence___minimal) { .a = 42, }, - .input_len = sizeof(struct core_reloc_existence), + .input_len = sizeof(struct core_reloc_existence___minimal), .output = STRUCT_TO_CHAR_PTR(core_reloc_existence_output) { .a_exists = 1, .b_exists = 0, From 3521ffa2ee9a48c3236c93f54ae11c074490ebce Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:08 -0700 Subject: [PATCH 08/11] libbpf: Fix huge memory leak in libbpf_find_vmlinux_btf_id() BTF object wasn't freed. Fixes: a6ed02cac690 ("libbpf: Load btf_vmlinux only once per object.") Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Cc: KP Singh Link: https://lore.kernel.org/bpf/20200429012111.277390-9-andriin@fb.com --- 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 445ee903f9cd..d86ff8214b96 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -6934,6 +6934,7 @@ int libbpf_find_vmlinux_btf_id(const char *name, enum bpf_attach_type attach_type) { struct btf *btf; + int err; btf = libbpf_find_kernel_btf(); if (IS_ERR(btf)) { @@ -6941,7 +6942,9 @@ int libbpf_find_vmlinux_btf_id(const char *name, return -EINVAL; } - return __find_vmlinux_btf_id(btf, name, attach_type); + err = __find_vmlinux_btf_id(btf, name, attach_type); + btf__free(btf); + return err; } static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) From 36d0b6159f6a6f51f600bf1777702f7036fb9839 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:09 -0700 Subject: [PATCH 09/11] selftests/bpf: Disable ASAN instrumentation for mmap()'ed memory read AddressSanitizer assumes that all memory dereferences are done against memory allocated by sanitizer's malloc()/free() code and not touched by anyone else. Seems like this doesn't hold for perf buffer memory. Disable instrumentation on perf buffer callback function. Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Link: https://lore.kernel.org/bpf/20200429012111.277390-10-andriin@fb.com --- tools/testing/selftests/bpf/prog_tests/perf_buffer.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c index 1450ea2dd4cc..a122ce3b360e 100644 --- a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c +++ b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c @@ -6,6 +6,11 @@ #include #include "bpf/libbpf_internal.h" +/* AddressSanitizer sometimes crashes due to data dereference below, due to + * this being mmap()'ed memory. Disable instrumentation with + * no_sanitize_address attribute + */ +__attribute__((no_sanitize_address)) static void on_sample(void *ctx, int cpu, void *data, __u32 size) { int cpu_data = *(int *)data, duration = 0; From 8d30e80a049ad699264e4a12911e349f93c7279a Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:10 -0700 Subject: [PATCH 10/11] selftests/bpf: Fix bpf_link leak in ns_current_pid_tgid selftest If condition is inverted, but it's also just not necessary. Fixes: 1c1052e0140a ("tools/testing/selftests/bpf: Add self-tests for new helper bpf_get_ns_current_pid_tgid.") Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Cc: Carlos Neira Link: https://lore.kernel.org/bpf/20200429012111.277390-11-andriin@fb.com --- tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c index 542240e16564..e74dc501b27f 100644 --- a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c @@ -80,9 +80,6 @@ void test_ns_current_pid_tgid(void) "User pid/tgid %llu BPF pid/tgid %llu\n", id, bss.pid_tgid)) goto cleanup; cleanup: - if (!link) { - bpf_link__destroy(link); - link = NULL; - } + bpf_link__destroy(link); bpf_object__close(obj); } From e4e8f4d047fdcf7ac7d944e266e85d8041f16cd6 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 28 Apr 2020 18:21:11 -0700 Subject: [PATCH 11/11] selftests/bpf: Add runqslower binary to .gitignore With recent changes, runqslower is being copied into selftests/bpf root directory. So add it into .gitignore. Fixes: b26d1e2b6028 ("selftests/bpf: Copy runqslower to OUTPUT directory") Signed-off-by: Andrii Nakryiko Signed-off-by: Alexei Starovoitov Cc: Veronika Kabatova Link: https://lore.kernel.org/bpf/20200429012111.277390-12-andriin@fb.com --- tools/testing/selftests/bpf/.gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 16b9774d8b68..3ff031972975 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -37,4 +37,4 @@ test_cpp /no_alu32 /bpf_gcc /tools - +/runqslower