On Wed, Dec 13, 2023 at 12:39 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2023-12-13 at 11:25 -0800, Andrii Nakryiko wrote: > [...] > > Yes, if we add a bunch of extra log grabbing and matching logic to > > fexit_bpf2bpf test. Which, honestly, I just didn't want to touch more > > than I absolutely needed to. So I'll use your permission to ignore > > this. > > Still think it's useful and diff is not that big: > https://gist.github.com/eddyz87/5f518b96eb4188dd1afd436e811bbef9 Ok, Eduard, ok, I'll add it in this patch in the next revision. It can be done a bit simpler than in your example, though. $ git diff diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c index 39ce45337e7d..f29fc789c14b 100644 --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c @@ -348,7 +348,8 @@ static void test_func_sockmap_update(void) } static void test_obj_load_failure_common(const char *obj_file, - const char *target_obj_file) + const char *target_obj_file, + const char *exp_msg) { /* * standalone test that asserts failure to load freplace prog @@ -356,6 +357,7 @@ static void test_obj_load_failure_common(const char *obj_file, */ struct bpf_object *obj = NULL, *pkt_obj; struct bpf_program *prog; + char log_buf[64 * 1024]; int err, pkt_fd; __u32 duration = 0; @@ -374,14 +376,21 @@ static void test_obj_load_failure_common(const char *obj_file, err = bpf_program__set_attach_target(prog, pkt_fd, NULL); ASSERT_OK(err, "set_attach_target"); + log_buf[0] = '\0'; + if (exp_msg) + bpf_program__set_log_buf(prog, log_buf, sizeof(log_buf)); if (env.verbosity > VERBOSE_NONE) bpf_program__set_log_level(prog, 2); /* It should fail to load the program */ err = bpf_object__load(obj); + if (env.verbosity > VERBOSE_NONE && exp_msg) /* we overtook log */ + printf("VERIFIER LOG:\n================\n%s\n================\n", log_buf); if (CHECK(!err, "bpf_obj_load should fail", "err %d\n", err)) goto close_prog; + if (exp_msg) + ASSERT_HAS_SUBSTR(log_buf, exp_msg, "fail_msg"); close_prog: bpf_object__close(obj); bpf_object__close(pkt_obj); @@ -391,14 +400,14 @@ static void test_func_replace_return_code(void) { /* test invalid return code in the replaced program */ test_obj_load_failure_common("./freplace_connect_v4_prog.bpf.o", - "./connect4_prog.bpf.o"); + "./connect4_prog.bpf.o", NULL); } static void test_func_map_prog_compatibility(void) { /* test with spin lock map value in the replaced program */ test_obj_load_failure_common("./freplace_attach_probe.bpf.o", - "./test_attach_probe.bpf.o"); + "./test_attach_probe.bpf.o", NULL); } static void test_func_replace_unreliable(void) @@ -407,7 +416,8 @@ static void test_func_replace_unreliable(void) * "Cannot replace static functions" */ test_obj_load_failure_common("freplace_unreliable_prog.bpf.o", - "./verifier_btf_unreliable_prog.bpf.o"); + "./verifier_btf_unreliable_prog.bpf.o", + "Cannot replace static functions"); } static void test_func_replace_global_func(void) > > > > Also, maybe kernel should be tweaked to be a bit more informative, > > > as message about static function is confusing, wdyt? > > > > > > > Currently the verifier doesn't distinguish between reasons for > > "unreliable". Not sure if it's worth tracking more information just > > for this. Certainly that feels like an orthogonal to this series > > improvement. > > Fair enough.
