On Thu, Apr 28, 2022 at 9:54 AM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> Add tests that ensure sleepable kprobe programs cannot attach.
>
> Also attach both sleepable and non-sleepable uprobe programs to the same
> location (i.e. same bpf_prog_array).
>
Yep, great thinking! I left a few comments below, otherwise looks good.
> Signed-off-by: Delyan Kratunov <delyank@xxxxxx>
> ---
> .../selftests/bpf/prog_tests/attach_probe.c | 35 +++++++++++++++
> .../selftests/bpf/progs/test_attach_probe.c | 44 +++++++++++++++++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index c0c6d410751d..c5c601537eea 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -17,6 +17,12 @@ static void trigger_func2(void)
> asm volatile ("");
> }
>
> +/* attach point for byname sleepable uprobe */
> +static void trigger_func3(void)
> +{
> + asm volatile ("");
> +}
> +
> void test_attach_probe(void)
> {
> DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> @@ -143,6 +149,28 @@ void test_attach_probe(void)
> if (!ASSERT_OK_PTR(skel->links.handle_uretprobe_byname2, "attach_uretprobe_byname2"))
> goto cleanup;
>
> + /* sleepable kprobes should not attach successfully */
> + skel->links.handle_kprobe_sleepable = bpf_program__attach(skel->progs.handle_kprobe_sleepable);
> + if (!ASSERT_NULL(skel->links.handle_kprobe_sleepable, "attach_kprobe_sleepable"))
we have ASSERT_ERR_PTR() which is more in line with ASSERT_OK_PTR(),
let's use that one.
With dropping SEC("kprobe.s") you'll have to do one extra step to make
sure that handle_kprobe_sleepable is actually sleepable program during
BPF verification. Please use bpf_program__set_flags() before load step
for that.
> + goto cleanup;
> +
> + /* test sleepable uprobe and uretprobe variants */
> + skel->links.handle_uprobe_byname3_sleepable = bpf_program__attach(skel->progs.handle_uprobe_byname3_sleepable);
> + if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byname3_sleepable, "attach_uprobe_byname3_sleepable"))
> + goto cleanup;
> +
[...]
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index af994d16bb10..265a23af74d4 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -14,6 +14,10 @@ int uprobe_byname_res = 0;
> int uretprobe_byname_res = 0;
> int uprobe_byname2_res = 0;
> int uretprobe_byname2_res = 0;
> +int uprobe_byname3_sleepable_res = 0;
> +int uprobe_byname3_res = 0;
> +int uretprobe_byname3_sleepable_res = 0;
> +int uretprobe_byname3_res = 0;
>
> SEC("kprobe/sys_nanosleep")
> int handle_kprobe(struct pt_regs *ctx)
> @@ -22,6 +26,13 @@ int handle_kprobe(struct pt_regs *ctx)
> return 0;
> }
>
> +SEC("kprobe.s/sys_nanosleep")
can you please leave comment here that this is supposed to fail to be
attached? It took me a bit to notice that you do negative test with
this program
> +int handle_kprobe_sleepable(struct pt_regs *ctx)
> +{
> + kprobe_res = 2;
> + return 0;
> +}
> +
[...]
