On Mon, May 2, 2022 at 4:09 PM 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).
>
> Signed-off-by: Delyan Kratunov <delyank@xxxxxx>
> ---
> .../selftests/bpf/prog_tests/attach_probe.c | 51 +++++++++++++++-
> .../selftests/bpf/progs/test_attach_probe.c | 58 +++++++++++++++++++
> 2 files changed, 108 insertions(+), 1 deletion(-)
>
LGTM, suggestion below is just about making flag setting less verbose,
but it's minor.
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index 08c0601b3e84..cddb17ab0588 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -17,6 +17,14 @@ static void trigger_func2(void)
> asm volatile ("");
> }
>
> +/* attach point for byname sleepable uprobe */
> +static void trigger_func3(void)
> +{
> + asm volatile ("");
> +}
> +
> +static char test_data[] = "test_data";
> +
> void test_attach_probe(void)
> {
> DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> @@ -27,6 +35,7 @@ void test_attach_probe(void)
> struct bpf_link *uprobe_err_link;
> bool legacy;
> char *mem;
> + int kprobe_s_flags;
>
> /* Check if new-style kprobe/uprobe API is supported.
> * Kernels that support new FD-based kprobe and uprobe BPF attachment
> @@ -49,9 +58,18 @@ void test_attach_probe(void)
> if (!ASSERT_GE(ref_ctr_offset, 0, "ref_ctr_offset"))
> return;
>
> - skel = test_attach_probe__open_and_load();
> + skel = test_attach_probe__open();
> if (!ASSERT_OK_PTR(skel, "skel_open"))
> return;
> +
> + /* sleepable kprobe test case needs flags set before loading */
> + kprobe_s_flags = bpf_program__flags(skel->progs.handle_kprobe_sleepable);
> + if (!ASSERT_OK(bpf_program__set_flags(skel->progs.handle_kprobe_sleepable,
> + kprobe_s_flags | BPF_F_SLEEPABLE), "kprobe_sleepable_flags"))
> + goto cleanup;
This feels like unnecessary checks and fetching of the flag. We
control all this, so just doing
bpf_program__set_flag(skel->progs.handle_kprobe_sleepable, BPF_F_SLEEPABLE);
seems totally justified and reliable
> +
> + if (!ASSERT_OK(test_attach_probe__load(skel), "skel_load"))
> + goto cleanup;
> if (!ASSERT_OK_PTR(skel->bss, "check_bss"))
> goto cleanup;
>
[...]
