| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzYTg+eawA9gbBM30QZpwS=wTNCpG4SsFNiLctKjChyFNg@mail.gmail.com>
Date: Mon, 26 Apr 2021 08:55:10 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Lorenz Bauer <lmb@...udflare.com>
Cc: Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 5/5] selftests/bpf: fix core_reloc test runner
On Mon, Apr 26, 2021 at 1:17 AM Lorenz Bauer <lmb@...udflare.com> wrote:
>
> On Sat, 24 Apr 2021 at 00:36, Andrii Nakryiko <andrii@...nel.org> wrote:
> >
> > Fix failed tests checks in core_reloc test runner, which allowed failing tests
> > to pass quietly. Also add extra check to make sure that expected to fail test cases with
> > invalid names are caught as test failure anyway, as this is not an expected
> > failure mode. Also fix mislabeled probed vs direct bitfield test cases.
> >
> > Fixes: 124a892d1c41 ("selftests/bpf: Test TYPE_EXISTS and TYPE_SIZE CO-RE relocations")
> > Reported-by: Lorenz Bauer <lmb@...udflare.com>
> > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> > ---
> > .../selftests/bpf/prog_tests/core_reloc.c | 20 +++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> > index 385fd7696a2e..607710826dca 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> > @@ -217,7 +217,7 @@ static int duration = 0;
> >
> > #define BITFIELDS_CASE(name, ...) { \
> > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_probed.o", \
> > - "direct:", name), \
> > + "probed:", name), \
> > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \
> > .input_len = sizeof(struct core_reloc_##name), \
> > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \
> > @@ -225,7 +225,7 @@ static int duration = 0;
> > .output_len = sizeof(struct core_reloc_bitfields_output), \
> > }, { \
> > BITFIELDS_CASE_COMMON("test_core_reloc_bitfields_direct.o", \
> > - "probed:", name), \
> > + "direct:", name), \
> > .input = STRUCT_TO_CHAR_PTR(core_reloc_##name) __VA_ARGS__, \
> > .input_len = sizeof(struct core_reloc_##name), \
> > .output = STRUCT_TO_CHAR_PTR(core_reloc_bitfields_output) \
> > @@ -546,8 +546,7 @@ static struct core_reloc_test_case test_cases[] = {
> > ARRAYS_ERR_CASE(arrays___err_too_small),
> > ARRAYS_ERR_CASE(arrays___err_too_shallow),
> > ARRAYS_ERR_CASE(arrays___err_non_array),
> > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type1),
> > - ARRAYS_ERR_CASE(arrays___err_wrong_val_type2),
> > + ARRAYS_ERR_CASE(arrays___err_wrong_val_type),
> > ARRAYS_ERR_CASE(arrays___err_bad_zero_sz_arr),
> >
> > /* enum/ptr/int handling scenarios */
> > @@ -865,13 +864,20 @@ void test_core_reloc(void)
> > "prog '%s' not found\n", probe_name))
> > goto cleanup;
> >
> > +
> > + if (test_case->btf_src_file) {
> > + err = access(test_case->btf_src_file, R_OK);
> > + if (!ASSERT_OK(err, "btf_src_file"))
> > + goto cleanup;
> > + }
> > +
> > load_attr.obj = obj;
> > load_attr.log_level = 0;
> > load_attr.target_btf_path = test_case->btf_src_file;
> > err = bpf_object__load_xattr(&load_attr);
> > if (err) {
> > if (!test_case->fails)
> > - CHECK(false, "obj_load", "failed to load prog '%s': %d\n", probe_name, err);
> > + ASSERT_OK(err, "obj_load");
> > goto cleanup;
> > }
> >
> > @@ -910,10 +916,8 @@ void test_core_reloc(void)
> > goto cleanup;
> > }
> >
> > - if (test_case->fails) {
> > - CHECK(false, "obj_load_fail", "should fail to load prog '%s'\n", probe_name);
> > + if (!ASSERT_FALSE(test_case->fails, "obj_load_should_fail"))
>
> Similar to my other comment, I find it difficult to tell when this
> triggers. Maybe it makes sense to return the status of the
> assertion (not the original value)? So if (assertion()) will be
> executed when the assertion fails? Not sure.
>
ASSERT_XXX() does return the status of assertion -- true if it holds,
false if it's violated. So false from ASSERT_xxx() means the test
already is marked failed.
Mechanically, in this case, it reads as "if we couldn't assert that
test_case->fails == false, do something about it". It's the part why
test_case->fails should be false is a bit obscure (because we
successfully loaded, but test_case is marked as should-be-failed, so
test_case->fails has to be false).
I hope it helps at least a bit.
> Acked-by: Lorenz Bauer <lmb@...udflare.com>
>
> --
> Lorenz Bauer | Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com
Powered by blists - more mailing lists