netdev - Re: [PATCH v3 bpf-next 4/4] 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]

Message-ID: <CAEf4BzZW6W6vVtsGEDyYcc=ZMon676r9NOAbnseZA1az2Heq3Q@mail.gmail.com>
Date:   Fri, 4 Feb 2022 11:30:15 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Yucong Sun <sunyucong@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v3 bpf-next 4/4] selftests/bpf: add tests for u[ret]probe
 attach by name

On Mon, Jan 31, 2022 at 8:13 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 -ESRCH 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, 123 insertions(+), 3 deletions(-)
>

[...]

>         if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res",
>                   "wrong uprobe res: %d\n", skel->bss->uprobe_res))
>                 goto cleanup;
> @@ -110,7 +179,21 @@ void test_attach_probe(void)
>                   "wrong uretprobe res: %d\n", skel->bss->uretprobe_res))
>                 goto cleanup;
>
> +       if (CHECK(skel->bss->uprobe_byname_res != 5, "check_uprobe_byname_res",
> +                 "wrong uprobe byname res: %d\n", skel->bss->uprobe_byname_res))
> +               goto cleanup;
> +       if (CHECK(skel->bss->uretprobe_byname_res != 6, "check_uretprobe_byname_res",
> +                 "wrong uretprobe byname res: %d\n", skel->bss->uretprobe_byname_res))
> +               goto cleanup;
> +       if (CHECK(skel->bss->uprobe_byname2_res != 7, "check_uprobe_byname2_res",
> +                 "wrong uprobe byname2 res: %d\n", skel->bss->uprobe_byname2_res))
> +               goto cleanup;
> +       if (CHECK(skel->bss->uretprobe_byname2_res != 8, "check_uretprobe_byname2_res",
> +                 "wrong uretprobe byname2 res: %d\n", skel->bss->uretprobe_byname2_res))
> +               goto cleanup;
> +

Please don't use CHECK()s for new code, only ASSERT_xxx() for new code.

>  cleanup:
> +       free(libc_path);
>         test_attach_probe__destroy(skel);
>         ASSERT_EQ(uprobe_ref_ctr, 0, "uprobe_ref_ctr_cleanup");
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index 8056a4c..9942461c 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -10,6 +10,10 @@
>  int kretprobe_res = 0;
>  int uprobe_res = 0;
>  int uretprobe_res = 0;
> +int uprobe_byname_res = 0;
> +int uretprobe_byname_res = 0;
> +int uprobe_byname2_res = 0;
> +int uretprobe_byname2_res = 0;
>
>  SEC("kprobe/sys_nanosleep")
>  int handle_kprobe(struct pt_regs *ctx)
> @@ -39,4 +43,37 @@ int handle_uretprobe(struct pt_regs *ctx)
>         return 0;
>  }
>
> +SEC("uprobe/trigger_func_byname")
> +int handle_uprobe_byname(struct pt_regs *ctx)
> +{
> +       uprobe_byname_res = 5;
> +       return 0;
> +}
> +
> +/* use auto-attach format for section definition. */
> +SEC("uretprobe//proc/self/exe:trigger_func2")
> +int handle_uretprobe_byname(struct pt_regs *ctx)
> +{
> +       uretprobe_byname_res = 6;
> +       return 0;
> +}
> +
> +SEC("uprobe/trigger_func_byname2")
> +int handle_uprobe_byname2(struct pt_regs *ctx)

this one is for malloc, so why SEC() doesn't reflect this?

It would be great to also have (probably separate) selftest for
auto-attach logic of skeleton for uprobes.
I'd add a separate uprobe-specific selftests, there is plenty to test
without having kprobes intermingled.

> +{
> +       unsigned int size = PT_REGS_PARM1(ctx);
> +
> +       /* verify malloc size */
> +       if (size == 1)
> +               uprobe_byname2_res = 7;
> +       return 0;
> +}
> +
> +SEC("uretprobe/trigger_func_byname2")
> +int handle_uretprobe_byname2(struct pt_regs *ctx)
> +{
> +       uretprobe_byname2_res = 8;
> +       return 0;
> +}
> +
>  char _license[] SEC("license") = "GPL";
> --
> 1.8.3.1
>