netdev - Re: [PATCH v4 bpf-next 4/5] selftests/bpf: add tests for u[ret]probe attach by name

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

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

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

Date:   Tue, 15 Mar 2022 21:33:23 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Yucong Sun <sunyucong@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v4 bpf-next 4/5] selftests/bpf: add tests for u[ret]probe
 attach by name

On Fri, Mar 11, 2022 at 4:11 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> add tests that verify attaching by name for
>
> 1. local functions in a program
> 2. library functions in a shared object; and
> 3. library functions in a program
>
> ...succeed for uprobe and uretprobes using new "func_name"
> option for bpf_program__attach_uprobe_opts().  Also verify
> auto-attach works where uprobe, path to binary and function
> name are specified, but fails with -EOPNOTSUPP when the format
> does not match (the latter is to support backwards-compatibility).
>
> Signed-off-by: Alan Maguire <alan.maguire@...cle.com>
> ---
>  .../selftests/bpf/prog_tests/attach_probe.c        | 89 ++++++++++++++++++----
>  .../selftests/bpf/progs/test_attach_probe.c        | 37 +++++++++
>  2 files changed, 113 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index d48f6e5..b770e0e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -11,15 +11,22 @@ static void trigger_func(void)
>         asm volatile ("");
>  }
>
> +/* attach point for byname uprobe */
> +static void trigger_func2(void)
> +{
> +       asm volatile ("");
> +}
> +
>  void test_attach_probe(void)
>  {
>         DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> -       int duration = 0;
>         struct bpf_link *kprobe_link, *kretprobe_link;
>         struct bpf_link *uprobe_link, *uretprobe_link;
>         struct test_attach_probe* skel;
>         ssize_t uprobe_offset, ref_ctr_offset;
> +       struct bpf_link *uprobe_err_link;
>         bool legacy;
> +       char *mem;
>
>         /* Check if new-style kprobe/uprobe API is supported.
>          * Kernels that support new FD-based kprobe and uprobe BPF attachment

[...]

> -       if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res",
> -                 "wrong uprobe res: %d\n", skel->bss->uprobe_res))
> +       /* trigger & validate uprobe attached by name */
> +       trigger_func2();
> +
> +       if (!ASSERT_EQ(skel->bss->kprobe_res, 1, "check_kprobe_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->kretprobe_res, 2, "check_kretprobe_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->uprobe_res, 3, "check_uprobe_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->uretprobe_res, 4, "check_uretprobe_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->uprobe_byname_res, 5, "check_uprobe_byname_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->uretprobe_byname_res, 6, "check_uretprobe_byname_res"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(skel->bss->uprobe_byname2_res, 7, "check_uprobe_byname2_res"))
>                 goto cleanup;

no need for all those goto cleanup. Just do unconditional ASSERT_EQ(),
it doesn't hurt and is much cleaner.

> -       if (CHECK(skel->bss->uretprobe_res != 4, "check_uretprobe_res",
> -                 "wrong uretprobe res: %d\n", skel->bss->uretprobe_res))
> +       if (!ASSERT_EQ(skel->bss->uretprobe_byname2_res, 8, "check_uretprobe_byname2_res"))
>                 goto cleanup;
>
>  cleanup:

[...]