From: Geliang Tang tanggeliang@kylinos.cn
v2: - Although all CI tests passed on x86_64 "bpf/vmtest-bpf-next-VM_Test-22 Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc", some unexpect "SKIP"s are showed in the log:
#29/1 bpf_tcp_ca/dctcp:SKIP #29/2 bpf_tcp_ca/cubic:OK #29/3 bpf_tcp_ca/invalid_license:OK #29/4 bpf_tcp_ca/dctcp_fallback:SKIP #29/5 bpf_tcp_ca/rel_setsockopt:OK #29/6 bpf_tcp_ca/write_sk_pacing:OK #29/7 bpf_tcp_ca/incompl_cong_ops:OK #29/8 bpf_tcp_ca/unsupp_cong_op:OK #29/9 bpf_tcp_ca/update_ca:OK #29/10 bpf_tcp_ca/update_wrong:OK #29/11 bpf_tcp_ca/mixed_links:OK #29/12 bpf_tcp_ca/multi_links:OK #29/13 bpf_tcp_ca/link_replace:OK #29/14 bpf_tcp_ca/tcp_ca_kfunc:OK #29/15 bpf_tcp_ca/cc_cubic:OK #29/16 bpf_tcp_ca/dctcp_autoattach_map:SKIP #29 bpf_tcp_ca:OK (SKIP: 3/16)
Shouldn't skip any tests on X86_64. Fix this in v2.
- add a new helper test_progs_get_error.
BPF selftests seem to have not been fully tested on Loongarch platforms. There are so many "ENOTSUPP" (-524) errors when running BPF selftests on them since lacking BPF trampoline on Loongarch.
For these "ENOTSUPP" tests, it's better to skip them, instead of reporting some "ENOTSUPP" errors. This patchset skips ENOTSUPP in ASSERT_OK/ ASSERT_OK_PTR/ASSERT_GE helpers to fix them. This is useful for running BPF selftests for other architectures too.
Geliang Tang (6): selftests/bpf: Define ENOTSUPP in testing_helpers.h selftests/bpf: Skip ENOTSUPP in ASSERT_OK selftests/bpf: Use ASSERT_OK to skip ENOTSUPP selftests/bpf: Null checks for link in bpf_tcp_ca selftests/bpf: Skip ENOTSUPP in ASSERT_OK_PTR selftests/bpf: Skip ENOTSUPP in ASSERT_GE
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 20 ++++++----- .../testing/selftests/bpf/prog_tests/d_path.c | 2 +- .../selftests/bpf/prog_tests/lsm_cgroup.c | 10 +----- .../selftests/bpf/prog_tests/module_attach.c | 2 +- .../selftests/bpf/prog_tests/ringbuf.c | 2 +- .../selftests/bpf/prog_tests/sock_addr.c | 4 --- .../selftests/bpf/prog_tests/test_bprm_opts.c | 2 +- .../selftests/bpf/prog_tests/test_ima.c | 2 +- .../selftests/bpf/prog_tests/trace_ext.c | 2 +- tools/testing/selftests/bpf/test_maps.c | 4 --- tools/testing/selftests/bpf/test_progs.h | 33 +++++++++++++++---- tools/testing/selftests/bpf/test_verifier.c | 4 --- tools/testing/selftests/bpf/testing_helpers.h | 4 +++ 13 files changed, 50 insertions(+), 41 deletions(-)
From: Geliang Tang tanggeliang@kylinos.cn
ENOTSUPP are defined in so many places in bpf selftests, this patch moves this definition into testing_helpers.h, which is almost included by each tests. And drop all other duplicate definitions.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c | 4 ---- tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c | 4 ---- tools/testing/selftests/bpf/prog_tests/sock_addr.c | 4 ---- tools/testing/selftests/bpf/test_maps.c | 4 ---- tools/testing/selftests/bpf/test_verifier.c | 4 ---- tools/testing/selftests/bpf/testing_helpers.h | 4 ++++ 6 files changed, 4 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 4b0169e6a708..5909c1f82f3b 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -16,10 +16,6 @@ #include "tcp_ca_kfunc.skel.h" #include "bpf_cc_cubic.skel.h"
-#ifndef ENOTSUPP -#define ENOTSUPP 524 -#endif - static const unsigned int total_bytes = 10 * 1024 * 1024; static int expected_stg = 0xeB9F;
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c index 130a3b21e467..6df25de8f080 100644 --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c @@ -10,10 +10,6 @@ #include "cgroup_helpers.h" #include "network_helpers.h"
-#ifndef ENOTSUPP -#define ENOTSUPP 524 -#endif - static struct btf *btf;
static __u32 query_prog_cnt(int cgroup_fd, const char *attach_func) diff --git a/tools/testing/selftests/bpf/prog_tests/sock_addr.c b/tools/testing/selftests/bpf/prog_tests/sock_addr.c index b880c564a204..68d9255d2bb7 100644 --- a/tools/testing/selftests/bpf/prog_tests/sock_addr.c +++ b/tools/testing/selftests/bpf/prog_tests/sock_addr.c @@ -23,10 +23,6 @@ #include "getpeername_unix_prog.skel.h" #include "network_helpers.h"
-#ifndef ENOTSUPP -# define ENOTSUPP 524 -#endif - #define TEST_NS "sock_addr" #define TEST_IF_PREFIX "test_sock_addr" #define TEST_IPV4 "127.0.0.4" diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index dfbab214f4d1..227d7d6eaf8e 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -26,10 +26,6 @@ #include "test_maps.h" #include "testing_helpers.h"
-#ifndef ENOTSUPP -#define ENOTSUPP 524 -#endif - int skips;
static struct bpf_map_create_opts map_opts = { .sz = sizeof(map_opts) }; diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 610392dfc4fb..447b68509d76 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -42,10 +42,6 @@ #include "../../../include/linux/filter.h" #include "testing_helpers.h"
-#ifndef ENOTSUPP -#define ENOTSUPP 524 -#endif - #define MAX_INSNS BPF_MAXINSNS #define MAX_EXPECTED_INSNS 32 #define MAX_UNEXPECTED_INSNS 32 diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h index d55f6ab12433..bad21e72dafc 100644 --- a/tools/testing/selftests/bpf/testing_helpers.h +++ b/tools/testing/selftests/bpf/testing_helpers.h @@ -9,6 +9,10 @@ #include <bpf/libbpf.h> #include <time.h>
+#ifndef ENOTSUPP +#define ENOTSUPP 524 +#endif + #define __TO_STR(x) #x #define TO_STR(x) __TO_STR(x)
From: Geliang Tang tanggeliang@kylinos.cn
Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch adds a new helper test_progs_get_error() to check whether the input error is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip the test instead of using test__fail().
Use this helper in ASSERT_OK() before invoking CHECK() macro.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/lsm_cgroup.c | 6 +---- tools/testing/selftests/bpf/test_progs.h | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c index 6df25de8f080..6511f5f4a00f 100644 --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c @@ -102,12 +102,8 @@ static void test_lsm_cgroup_functional(void) ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count"); ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count"); err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0); - if (err == -ENOTSUPP) { - test__skip(); - goto close_cgroup; - } if (!ASSERT_OK(err, "attach alloc_prog_fd")) - goto detach_cgroup; + goto close_cgroup; ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count"); ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 1, "total prog count");
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 930a4181dbd9..d1d77785b165 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -176,6 +176,23 @@ void test__skip(void); void test__fail(void); int test__join_cgroup(const char *path);
+static inline bool test_progs_check_errno(int error, int check) +{ + return error == -check || + (error && errno == check); +} + +static inline int test_progs_get_error(int error) +{ + if (test_progs_check_errno(error, ENOTSUP) || + test_progs_check_errno(error, ENOTSUPP)) { + test__skip(); + return 0; + } else { + return error; + } +} + #define PRINT_FAIL(format...) \ ({ \ test__fail(); \ @@ -338,8 +355,10 @@ int test__join_cgroup(const char *path); static int duration = 0; \ long long ___res = (res); \ bool ___ok = ___res == 0; \ - CHECK(!___ok, (name), "unexpected error: %lld (errno %d)\n", \ - ___res, errno); \ + if (test_progs_get_error(___res)) \ + CHECK(!___ok, (name), \ + "unexpected error: %lld (errno %d)\n", \ + ___res, errno); \ ___ok; \ })
On Thu, Jul 4, 2024 at 7:38 PM Geliang Tang geliang@kernel.org wrote:
From: Geliang Tang tanggeliang@kylinos.cn
Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch adds a new helper test_progs_get_error() to check whether the input error is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip the test instead of using test__fail().
Use this helper in ASSERT_OK() before invoking CHECK() macro.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn
.../selftests/bpf/prog_tests/lsm_cgroup.c | 6 +---- tools/testing/selftests/bpf/test_progs.h | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 7 deletions(-)
I haven't followed these patch sets, but no, let's not add magical special error codes handling into ASSERT_xxx() macros.
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c index 6df25de8f080..6511f5f4a00f 100644 --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c @@ -102,12 +102,8 @@ static void test_lsm_cgroup_functional(void) ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 0, "prog count"); ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 0, "total prog count"); err = bpf_prog_attach(alloc_prog_fd, cgroup_fd, BPF_LSM_CGROUP, 0);
if (err == -ENOTSUPP) {test__skip();goto close_cgroup;} if (!ASSERT_OK(err, "attach alloc_prog_fd"))goto detach_cgroup;
goto close_cgroup; ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_sk_alloc_security"), 1, "prog count"); ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 1, "total prog count");diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 930a4181dbd9..d1d77785b165 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -176,6 +176,23 @@ void test__skip(void); void test__fail(void); int test__join_cgroup(const char *path);
+static inline bool test_progs_check_errno(int error, int check) +{
return error == -check ||(error && errno == check);+}
+static inline int test_progs_get_error(int error) +{
if (test_progs_check_errno(error, ENOTSUP) ||test_progs_check_errno(error, ENOTSUPP)) {test__skip();return 0;} else {return error;}+}
#define PRINT_FAIL(format...) \ ({ \ test__fail(); \ @@ -338,8 +355,10 @@ int test__join_cgroup(const char *path); static int duration = 0; \ long long ___res = (res); \ bool ___ok = ___res == 0; \
CHECK(!___ok, (name), "unexpected error: %lld (errno %d)\n", \___res, errno); \
if (test_progs_get_error(___res)) \CHECK(!___ok, (name), \"unexpected error: %lld (errno %d)\n", \___res, errno); \ ___ok; \})
-- 2.43.0
On Mon, 2024-07-08 at 11:54 -0700, Andrii Nakryiko wrote:
On Thu, Jul 4, 2024 at 7:38 PM Geliang Tang geliang@kernel.org wrote:
From: Geliang Tang tanggeliang@kylinos.cn
Just like handling ENOTSUPP in test_lsm_cgroup_functional(), this patch adds a new helper test_progs_get_error() to check whether the input error is ENOTSUPP (524) or ENOTSUP (95). If it is, invoke test__skip() to skip the test instead of using test__fail().
Use this helper in ASSERT_OK() before invoking CHECK() macro.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn
.../selftests/bpf/prog_tests/lsm_cgroup.c | 6 +---- tools/testing/selftests/bpf/test_progs.h | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 7 deletions(-)
I haven't followed these patch sets, but no, let's not add magical special error codes handling into ASSERT_xxx() macros.
I agree with Andrii here. You might use -d (denylist) option for test_progs to filter out test cases you know are not supported.
[...]
From: Geliang Tang tanggeliang@kylinos.cn
There are so many "ENOTSUPP" (-524) errors when running BPF selftests on a Loongarch platform since lacking BPF trampoline on Loongarch:
''' test_d_path_basic:PASS:setup 0 nsec libbpf: prog 'prog_stat': failed to attach: unknown error (-524) libbpf: prog 'prog_stat': failed to auto-attach: -524 test_d_path_basic:FAIL:setup attach failed: -524 #77/1 d_path/basic:FAIL #77/2 d_path/check_rdonly_mem:OK #77/3 d_path/check_alloc_mem:OK #77 d_path:FAIL ... ... test_module_attach:PASS:skel_open 0 nsec test_module_attach:PASS:set_attach_target 0 nsec test_module_attach:PASS:set_attach_target_explicit 0 nsec test_module_attach:PASS:skel_load 0 nsec libbpf: prog 'handle_fentry': failed to attach: unknown error (-524) libbpf: prog 'handle_fentry': failed to auto-attach: -524 test_module_attach:FAIL:skel_attach skeleton attach failed: -524 #167 module_attach:FAIL ... ... ringbuf_subtest:PASS:skel_open 0 nsec ringbuf_subtest:PASS:skel_load 0 nsec ringbuf_subtest:PASS:rw_cons_pos 0 nsec ringbuf_subtest:PASS:rw_extend 0 nsec ringbuf_subtest:PASS:exec_cons_pos_protect 0 nsec ringbuf_subtest:PASS:unmap_rw 0 nsec ringbuf_subtest:PASS:wr_prod_pos 0 nsec ringbuf_subtest:PASS:wr_prod_pos_err 0 nsec ringbuf_subtest:PASS:wr_data_page_one 0 nsec ringbuf_subtest:PASS:wr_data_page_one_err 0 nsec ringbuf_subtest:PASS:wr_data_page_two 0 nsec ringbuf_subtest:PASS:wr_data_page_all 0 nsec ringbuf_subtest:PASS:ro_prod_pos 0 nsec ringbuf_subtest:PASS:write_protect 0 nsec ringbuf_subtest:PASS:exec_protect 0 nsec ringbuf_subtest:PASS:ro_remap 0 nsec ringbuf_subtest:PASS:unmap_ro 0 nsec ringbuf_subtest:PASS:ro_prod_pos 0 nsec ringbuf_subtest:PASS:write_protect 0 nsec ringbuf_subtest:PASS:exec_protect 0 nsec ringbuf_subtest:PASS:ro_remap 0 nsec ringbuf_subtest:PASS:unmap_ro 0 nsec ringbuf_subtest:PASS:ringbuf_create 0 nsec ringbuf_subtest:FAIL:skel_attach skeleton attachment failed: -1 #277/1 ringbuf/ringbuf:FAIL #277/2 ringbuf/ringbuf_n:SKIP #277/3 ringbuf/ringbuf_map_key:SKIP #277 ringbuf:FAIL ... ... test_test_bprm_opts:PASS:skel_load 0 nsec libbpf: prog 'secure_exec': failed to attach: unknown error (-524) libbpf: prog 'secure_exec': failed to auto-attach: -524 test_test_bprm_opts:FAIL:attach attach failed: -524 #382 test_bprm_opts:FAIL ... ... test_test_ima:PASS:skel_load 0 nsec test_test_ima:PASS:ringbuf 0 nsec libbpf: prog 'bprm_committed_creds': failed to attach: \ unknown error (-524) libbpf: prog 'bprm_committed_creds': failed to auto-attach: -524 test_test_ima:FAIL:attach attach failed: -524 #384 test_ima:FAIL ... ... test_trace_ext:PASS:setup 0 nsec test_trace_ext:PASS:setup 0 nsec test_trace_ext:PASS:setup 0 nsec test_trace_ext:PASS:setup 0 nsec libbpf: prog 'test_pkt_md_access_new': failed to attach: \ unknown error (-524) libbpf: prog 'test_pkt_md_access_new': failed to auto-attach: -524 test_trace_ext:FAIL:setup freplace/test_pkt_md_access attach failed: -524 #397 trace_ext:FAIL '''
This patch uses ASSERT_OK() instead of CHECK() to skip these "ENOTSUPP" errors. With this change, the new output of these selftests look like:
''' #77/1 d_path/basic:SKIP #77/2 d_path/check_rdonly_mem:OK #77/3 d_path/check_alloc_mem:OK #77 d_path:OK (SKIP: 1/3) ... ... #167 module_attach:SKIP ... ... #277/1 ringbuf/ringbuf:SKIP #277/2 ringbuf/ringbuf_n:SKIP #277/3 ringbuf/ringbuf_map_key:SKIP #277 ringbuf:SKIP ... ... #382 test_bprm_opts:SKIP ... ... #384 test_ima:SKIP ... ... #397 trace_ext:SKIP '''
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/prog_tests/d_path.c | 2 +- tools/testing/selftests/bpf/prog_tests/module_attach.c | 2 +- tools/testing/selftests/bpf/prog_tests/ringbuf.c | 2 +- tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c | 2 +- tools/testing/selftests/bpf/prog_tests/test_ima.c | 2 +- tools/testing/selftests/bpf/prog_tests/trace_ext.c | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c index ccc768592e66..78110075b485 100644 --- a/tools/testing/selftests/bpf/prog_tests/d_path.c +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c @@ -129,7 +129,7 @@ static void test_d_path_basic(void) goto cleanup;
err = test_d_path__attach(skel); - if (CHECK(err, "setup", "attach failed: %d\n", err)) + if (!ASSERT_OK(err, "setup")) goto cleanup;
bss = skel->bss; diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c index 6d391d95f96e..4aab747ad202 100644 --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c @@ -62,7 +62,7 @@ void test_module_attach(void) bss = skel->bss;
err = test_module_attach__attach(skel); - if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err)) + if (!ASSERT_OK(err, "skel_attach")) goto cleanup;
/* trigger tracepoint */ diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c index 4c6f42dae409..ee7deb76b60a 100644 --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c @@ -161,7 +161,7 @@ static void ringbuf_subtest(void) goto cleanup;
err = test_ringbuf_lskel__attach(skel); - if (CHECK(err, "skel_attach", "skeleton attachment failed: %d\n", err)) + if (!ASSERT_OK(err, "skel_attach")) goto cleanup;
trigger_samples(); diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c index a0054019e677..40a86a303c1a 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c @@ -88,7 +88,7 @@ void test_test_bprm_opts(void) goto close_prog;
err = bprm_opts__attach(skel); - if (CHECK(err, "attach", "attach failed: %d\n", err)) + if (!ASSERT_OK(err, "attach")) goto close_prog;
/* Run the test with the secureexec bit unset */ diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index 810b14981c2e..2a6c388fe29d 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -83,7 +83,7 @@ void test_test_ima(void) goto close_prog;
err = ima__attach(skel); - if (CHECK(err, "attach", "attach failed: %d\n", err)) + if (!ASSERT_OK(err, "attach")) goto close_prog;
measured_dir = mkdtemp(measured_dir_template); diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c index aabdff7bea3e..d006c0b91178 100644 --- a/tools/testing/selftests/bpf/prog_tests/trace_ext.c +++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c @@ -60,7 +60,7 @@ void test_trace_ext(void) }
err = test_trace_ext__attach(skel_ext); - if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err)) + if (!ASSERT_OK(err, "setup replace/test_pkt_md_access attach")) goto cleanup;
prog = skel_ext->progs.test_pkt_md_access_new;
From: Geliang Tang tanggeliang@kylinos.cn
Run bpf_tcp_ca selftests (./test_progs -t bpf_tcp_ca) on a Loongarch platform, some "Segmentation fault" errors occur:
''' test_dctcp:PASS:bpf_dctcp__open_and_load 0 nsec test_dctcp:FAIL:bpf_map__attach_struct_ops unexpected error: -524 #29/1 bpf_tcp_ca/dctcp:FAIL test_cubic:PASS:bpf_cubic__open_and_load 0 nsec test_cubic:FAIL:bpf_map__attach_struct_ops unexpected error: -524 #29/2 bpf_tcp_ca/cubic:FAIL test_dctcp_fallback:PASS:dctcp_skel 0 nsec test_dctcp_fallback:PASS:bpf_dctcp__load 0 nsec test_dctcp_fallback:FAIL:dctcp link unexpected error: -524 #29/4 bpf_tcp_ca/dctcp_fallback:FAIL test_write_sk_pacing:PASS:open_and_load 0 nsec test_write_sk_pacing:FAIL:attach_struct_ops unexpected error: -524 #29/6 bpf_tcp_ca/write_sk_pacing:FAIL test_update_ca:PASS:open 0 nsec test_update_ca:FAIL:attach_struct_ops unexpected error: -524 settcpca:FAIL:setsockopt unexpected setsockopt: \ actual -1 == expected -1 (network_helpers.c:99: errno: No such file or directory) \ Failed to call post_socket_cb start_test:FAIL:start_server_str unexpected start_server_str: \ actual -1 == expected -1 test_update_ca:FAIL:ca1_ca1_cnt unexpected ca1_ca1_cnt: \ actual 0 <= expected 0 #29/9 bpf_tcp_ca/update_ca:FAIL #29 bpf_tcp_ca:FAIL Caught signal #11! Stack trace: ./test_progs(crash_handler+0x28)[0x5555567ed91c] linux-vdso.so.1(__vdso_rt_sigreturn+0x0)[0x7ffffee408b0] ./test_progs(bpf_link__update_map+0x80)[0x555556824a78] ./test_progs(+0x94d68)[0x5555564c4d68] ./test_progs(test_bpf_tcp_ca+0xe8)[0x5555564c6a88] ./test_progs(+0x3bde54)[0x5555567ede54] ./test_progs(main+0x61c)[0x5555567efd54] /usr/lib64/libc.so.6(+0x22208)[0x7ffff2aaa208] /usr/lib64/libc.so.6(__libc_start_main+0xac)[0x7ffff2aaa30c] ./test_progs(_start+0x48)[0x55555646bca8] Segmentation fault '''
This is because "link" returned by bpf_map__attach_struct_ops() is NULL in this case. test_progs crashs when this NULL link passes to bpf_link__update_map(). This patch adds NULL checks for all links in bpf_tcp_ca to fix these errors. If "link" is NULL, goto the newly added label "err" to destroy the skel.
v2: - use "goto err" instead of "return" as Eduard suggested.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 5909c1f82f3b..9effdfb1a5ce 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -407,7 +407,8 @@ static void test_update_ca(void) return;
link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); - ASSERT_OK_PTR(link, "attach_struct_ops"); + if (!ASSERT_OK_PTR(link, "attach_struct_ops")) + goto err;
do_test(&opts); saved_ca1_cnt = skel->bss->ca1_cnt; @@ -421,6 +422,7 @@ static void test_update_ca(void) ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
bpf_link__destroy(link); +err: tcp_ca_update__destroy(skel); }
@@ -443,7 +445,8 @@ static void test_update_wrong(void) return;
link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); - ASSERT_OK_PTR(link, "attach_struct_ops"); + if (!ASSERT_OK_PTR(link, "attach_struct_ops")) + goto err;
do_test(&opts); saved_ca1_cnt = skel->bss->ca1_cnt; @@ -456,6 +459,7 @@ static void test_update_wrong(void) ASSERT_GT(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
bpf_link__destroy(link); +err: tcp_ca_update__destroy(skel); }
@@ -480,7 +484,8 @@ static void test_mixed_links(void) ASSERT_OK_PTR(link_nl, "attach_struct_ops_nl");
link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); - ASSERT_OK_PTR(link, "attach_struct_ops"); + if (!ASSERT_OK_PTR(link, "attach_struct_ops")) + goto err;
do_test(&opts); ASSERT_GT(skel->bss->ca1_cnt, 0, "ca1_ca1_cnt"); @@ -489,6 +494,7 @@ static void test_mixed_links(void) ASSERT_ERR(err, "update_map");
bpf_link__destroy(link); +err: bpf_link__destroy(link_nl); tcp_ca_update__destroy(skel); } @@ -532,7 +538,8 @@ static void test_link_replace(void) bpf_link__destroy(link);
link = bpf_map__attach_struct_ops(skel->maps.ca_update_2); - ASSERT_OK_PTR(link, "attach_struct_ops_2nd"); + if (!ASSERT_OK_PTR(link, "attach_struct_ops_2nd")) + goto err;
/* BPF_F_REPLACE with a wrong old map Fd. It should fail! * @@ -555,6 +562,7 @@ static void test_link_replace(void)
bpf_link__destroy(link);
+err: tcp_ca_update__destroy(skel); }
On Fri, 2024-07-05 at 10:38 +0800, Geliang Tang wrote:
[...]
I think that this patch is an improvement independent of the patch-set. Please submit it separately.
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 16 ++++++++++++----
[...]
@@ -489,6 +494,7 @@ static void test_mixed_links(void) ASSERT_ERR(err, "update_map"); bpf_link__destroy(link); +err:
Nit: there are two links in this test, but ASSERT_OK_PTR is added only for a single one. Also note that bpf_link__destroy(NULL) works just fine, so it is possible to initialize links as NULL and make a jump to cleanup block w/o peeking exact position within that block.
bpf_link__destroy(link_nl); tcp_ca_update__destroy(skel); }
[...]
Hi Eduard,
On Mon, 2024-07-08 at 12:42 -0700, Eduard Zingerman wrote:
On Fri, 2024-07-05 at 10:38 +0800, Geliang Tang wrote:
[...]
I think that this patch is an improvement independent of the patch- set. Please submit it separately.
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 16 ++++++++++++----
[...]
@@ -489,6 +494,7 @@ static void test_mixed_links(void) ASSERT_ERR(err, "update_map"); bpf_link__destroy(link); +err:
Nit: there are two links in this test, but ASSERT_OK_PTR is added only for a single one. Also note that bpf_link__destroy(NULL) works just fine, so it is possible to initialize links as NULL and make a jump to cleanup block w/o peeking exact position within that block.
Thanks for your review. I sent a set named "BPF selftests misc fixes" yesterday to address your comments.
But reconsider it. I think here checking the first link (link_nl) is enough. We can keep the second link as it.
I changed "BPF selftests misc fixes" as "Changes Requested".
Thanks, -Geliang
bpf_link__destroy(link_nl); tcp_ca_update__destroy(skel); }
[...]
From: Geliang Tang tanggeliang@kylinos.cn
Although the "Segmentation fault" errors are fixed in the last commit, run bpf_tcp_ca selftests (./test_progs -t bpf_tcp_ca) on a Loongarch platform, still some other "ENOTSUPP" (-524) errors left since lacking BPF trampoline on Loongarch:
''' test_dctcp:PASS:bpf_dctcp__open_and_load 0 nsec test_dctcp:FAIL:bpf_map__attach_struct_ops unexpected error: -524 #29/1 bpf_tcp_ca/dctcp:FAIL test_cubic:PASS:bpf_cubic__open_and_load 0 nsec test_cubic:FAIL:bpf_map__attach_struct_ops unexpected error: -524 #29/2 bpf_tcp_ca/cubic:FAIL #29/3 bpf_tcp_ca/invalid_license:OK test_dctcp_fallback:PASS:dctcp_skel 0 nsec test_dctcp_fallback:PASS:bpf_dctcp__load 0 nsec test_dctcp_fallback:FAIL:dctcp link unexpected error: -524 #29/4 bpf_tcp_ca/dctcp_fallback:FAIL #29/5 bpf_tcp_ca/rel_setsockopt:OK test_write_sk_pacing:PASS:open_and_load 0 nsec test_write_sk_pacing:FAIL:attach_struct_ops unexpected error: -524 #29/6 bpf_tcp_ca/write_sk_pacing:FAIL #29/7 bpf_tcp_ca/incompl_cong_ops:OK #29/8 bpf_tcp_ca/unsupp_cong_op:OK test_update_ca:PASS:open 0 nsec test_update_ca:FAIL:attach_struct_ops unexpected error: -524 #29/9 bpf_tcp_ca/update_ca:FAIL test_update_wrong:PASS:open 0 nsec test_update_wrong:FAIL:attach_struct_ops unexpected error: -524 #29/10 bpf_tcp_ca/update_wrong:FAIL test_mixed_links:PASS:open 0 nsec test_mixed_links:FAIL:attach_struct_ops_nl unexpected error: -524 test_mixed_links:FAIL:attach_struct_ops unexpected error: -524 #29/11 bpf_tcp_ca/mixed_links:FAIL test_multi_links:PASS:open 0 nsec test_multi_links:FAIL:attach_struct_ops_1st unexpected error: -524 test_multi_links:FAIL:attach_struct_ops_2nd unexpected error: -524 #29/12 bpf_tcp_ca/multi_links:FAIL test_link_replace:PASS:open 0 nsec test_link_replace:FAIL:attach_struct_ops_1st unexpected error: -524 test_link_replace:FAIL:attach_struct_ops_2nd unexpected error: -524 #29/13 bpf_tcp_ca/link_replace:FAIL #29/14 bpf_tcp_ca/tcp_ca_kfunc:OK test_cc_cubic:PASS:bpf_cc_cubic__open_and_load 0 nsec test_cc_cubic:FAIL:bpf_map__attach_struct_ops unexpected error: -524 #29/15 bpf_tcp_ca/cc_cubic:FAIL #29 bpf_tcp_ca:FAIL '''
Just like in ASSERT_OK(), this patch uses test_progs_get_error() helper to skip ENOTSUPP (524) and ENOTSUP (95) in ASSERT_OK_PTR() too.
With this change, the new output of bpf_tcp_ca selftests look like:
''' #29/1 bpf_tcp_ca/dctcp:SKIP #29/2 bpf_tcp_ca/cubic:SKIP #29/3 bpf_tcp_ca/invalid_license:OK #29/4 bpf_tcp_ca/dctcp_fallback:SKIP #29/5 bpf_tcp_ca/rel_setsockopt:OK #29/6 bpf_tcp_ca/write_sk_pacing:SKIP #29/7 bpf_tcp_ca/incompl_cong_ops:OK #29/8 bpf_tcp_ca/unsupp_cong_op:OK #29/9 bpf_tcp_ca/update_ca:SKIP #29/10 bpf_tcp_ca/update_wrong:SKIP #29/11 bpf_tcp_ca/mixed_links:SKIP #29/12 bpf_tcp_ca/multi_links:SKIP #29/13 bpf_tcp_ca/link_replace:SKIP #29/14 bpf_tcp_ca/tcp_ca_kfunc:OK #29/15 bpf_tcp_ca/cc_cubic:SKIP #29 bpf_tcp_ca:OK (SKIP: 10/15) '''
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/test_progs.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index d1d77785b165..8ca6cd970676 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -383,7 +383,8 @@ static inline int test_progs_get_error(int error) const void *___res = (ptr); \ int ___err = libbpf_get_error(___res); \ bool ___ok = ___err == 0; \ - CHECK(!___ok, (name), "unexpected error: %d\n", ___err); \ + if (test_progs_get_error(___err)) \ + CHECK(!___ok, (name), "unexpected error: %d\n", ___err);\ ___ok; \ })
From: Geliang Tang tanggeliang@kylinos.cn
There are still some "ENOTSUPP" (-524) errors left when running BPF selftests on a Loongarch platform since ASSERT_GE() are used there to check the return values, not ASSERT_OK():
''' test_bpf_cookie:PASS:skel_open 0 nsec #17/1 bpf_cookie/kprobe:OK #17/2 bpf_cookie/multi_kprobe_link_api:OK #17/3 bpf_cookie/multi_kprobe_attach_api:OK #17/4 bpf_cookie/uprobe:OK #17/5 bpf_cookie/multi_uprobe_attach_api:OK #17/6 bpf_cookie/tracepoint:OK #17/7 bpf_cookie/perf_event:OK tracing_subtest:FAIL:fentry.link_create unexpected fentry.link_create: \ actual -524 < expected 0 #17/8 bpf_cookie/trampoline:FAIL lsm_subtest:FAIL:lsm.link_create unexpected lsm.link_create: \ actual -524 < expected 0 #17/9 bpf_cookie/lsm:FAIL #17/10 bpf_cookie/tp_btf:OK #17/11 bpf_cookie/raw_tp:OK #17 bpf_cookie:FAIL ... ... test_module_fentry_shadow:PASS:load_vmlinux_btf 0 nsec test_module_fentry_shadow:PASS:get_bpf_testmod_btf_fd 0 nsec test_module_fentry_shadow:PASS:btf_get_from_fd 0 nsec test_module_fentry_shadow:PASS:btf_find_by_name 0 nsec test_module_fentry_shadow:PASS:btf_find_by_name 0 nsec test_module_fentry_shadow:PASS:bpf_prog_load 0 nsec test_module_fentry_shadow:FAIL:bpf_link_create unexpected \ bpf_link_create: actual -524 < expected 0 #168 module_fentry_shadow:FAIL '''
Just like in ASSERT_OK(), this patch uses test_progs_get_error() helper to skip ENOTSUPP (524) and ENOTSUP (95) in ASSERT_GT() too.
With this change, the new output of these selftests look like:
''' #17/1 bpf_cookie/kprobe:OK #17/2 bpf_cookie/multi_kprobe_link_api:OK #17/3 bpf_cookie/multi_kprobe_attach_api:OK #17/4 bpf_cookie/uprobe:OK #17/5 bpf_cookie/multi_uprobe_attach_api:OK #17/6 bpf_cookie/tracepoint:OK #17/7 bpf_cookie/perf_event:OK #17/8 bpf_cookie/trampoline:SKIP #17/9 bpf_cookie/lsm:SKIP #17/10 bpf_cookie/tp_btf:SKIP #17/11 bpf_cookie/raw_tp:SKIP #17 bpf_cookie:OK (SKIP: 4/11) ... ... #168 module_fentry_shadow:SKIP '''
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- tools/testing/selftests/bpf/test_progs.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 8ca6cd970676..fe6e20df97d2 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -311,9 +311,10 @@ static inline int test_progs_get_error(int error) typeof(actual) ___act = (actual); \ typeof(expected) ___exp = (expected); \ bool ___ok = ___act >= ___exp; \ - CHECK(!___ok, (name), \ - "unexpected %s: actual %lld < expected %lld\n", \ - (name), (long long)(___act), (long long)(___exp)); \ + if (test_progs_get_error(___act)) \ + CHECK(!___ok, (name), \ + "unexpected %s: actual %lld < expected %lld\n", \ + (name), (long long)(___act), (long long)(___exp));\ ___ok; \ })
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
Eduard Zingerman -
Geliang Tang