netdev - Re: [PATCH bpf-next 1/2] selftests/bpf: use raw

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 PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Jan 2022 09:43:48 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Hou Tao <houtao1@...wei.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <songliubraving@...com>,
        "David S . Miller" <davem@...emloft.net>,
        John Fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Zi Shen Lim <zlim.lnx@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Julien Thierry <jthierry@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH bpf-next 1/2] selftests/bpf: use raw_tp program for atomic test

On Fri, Jan 21, 2022 at 5:57 AM Hou Tao <houtao1@...wei.com> wrote:
>
> Now atomic tests will attach fentry program and run it through
> bpf_prog_test_run(), but attaching fentry program depends on bpf
> trampoline which is only available under x86-64. Considering many
> archs have atomic support, using raw_tp program instead.
>
> Signed-off-by: Hou Tao <houtao1@...wei.com>
> ---

Nits about using generic ASSERT_TRUE instead of dedicated ASSERT_OK
checks, but otherwise LGTM

Acked-by: Andrii Nakryiko <andrii@...nel.org>


>  .../selftests/bpf/prog_tests/atomics.c        | 114 +++++-------------
>  tools/testing/selftests/bpf/progs/atomics.c   |  29 ++---
>  2 files changed, 44 insertions(+), 99 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/atomics.c b/tools/testing/selftests/bpf/prog_tests/atomics.c
> index 86b7d5d84eec..0de292c1ec02 100644
> --- a/tools/testing/selftests/bpf/prog_tests/atomics.c
> +++ b/tools/testing/selftests/bpf/prog_tests/atomics.c
> @@ -8,18 +8,13 @@ static void test_add(struct atomics_lskel *skel)
>  {
>         int err, prog_fd;
>         __u32 duration = 0, retval;
> -       int link_fd;
> -
> -       link_fd = atomics_lskel__add__attach(skel);
> -       if (!ASSERT_GT(link_fd, 0, "attach(add)"))
> -               return;
>
> +       /* No need to attach it, just run it directly */
>         prog_fd = skel->progs.add.prog_fd;
> -       err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> +       err = bpf_prog_test_run(prog_fd, 0, NULL, 0,
>                                 NULL, NULL, &retval, &duration);
> -       if (CHECK(err || retval, "test_run add",
> -                 "err %d errno %d retval %d duration %d\n", err, errno, retval, duration))
> -               goto cleanup;
> +       if (!ASSERT_TRUE(!err && !retval, "test_run add"))

please do this as two separate asserts: ASSERT_OK(err) and ASSERT_OK(retval)

> +               return;
>
>         ASSERT_EQ(skel->data->add64_value, 3, "add64_value");
>         ASSERT_EQ(skel->bss->add64_result, 1, "add64_result");
> @@ -31,28 +26,19 @@ static void test_add(struct atomics_lskel *skel)
>         ASSERT_EQ(skel->bss->add_stack_result, 1, "add_stack_result");
>
>         ASSERT_EQ(skel->data->add_noreturn_value, 3, "add_noreturn_value");
> -
> -cleanup:
> -       close(link_fd);
>  }
>
>  static void test_sub(struct atomics_lskel *skel)
>  {
>         int err, prog_fd;
>         __u32 duration = 0, retval;
> -       int link_fd;
> -
> -       link_fd = atomics_lskel__sub__attach(skel);
> -       if (!ASSERT_GT(link_fd, 0, "attach(sub)"))
> -               return;
>
> +       /* No need to attach it, just run it directly */
>         prog_fd = skel->progs.sub.prog_fd;
> -       err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> +       err = bpf_prog_test_run(prog_fd, 0, NULL, 0,
>                                 NULL, NULL, &retval, &duration);
> -       if (CHECK(err || retval, "test_run sub",
> -                 "err %d errno %d retval %d duration %d\n",
> -                 err, errno, retval, duration))
> -               goto cleanup;
> +       if (!ASSERT_TRUE(!err && !retval, "test_run sub"))

same as above, same below for all the CHECKs replaced with ASSERT_TRUE

> +               return;
>
>         ASSERT_EQ(skel->data->sub64_value, -1, "sub64_value");
>         ASSERT_EQ(skel->bss->sub64_result, 1, "sub64_result");
> @@ -64,27 +50,19 @@ static void test_sub(struct atomics_lskel *skel)
>         ASSERT_EQ(skel->bss->sub_stack_result, 1, "sub_stack_result");
>
>         ASSERT_EQ(skel->data->sub_noreturn_value, -1, "sub_noreturn_value");
> -
> -cleanup:
> -       close(link_fd);
>  }
>

[...]