Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Add test for sleepable bpf iterator programs

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Jan 13, 2022 at 4:49 PM Kenny Yu <kennyyu@fb.com> wrote:
>
> This adds a test for bpf iterator programs to make use of sleepable
> bpf helpers.
>
> Signed-off-by: Kenny Yu <kennyyu@fb.com>
> ---
>  .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++++
>  .../selftests/bpf/progs/bpf_iter_task.c       | 54 +++++++++++++++++++
>  2 files changed, 70 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index b84f859b1267..fcda0ecd8746 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -138,6 +138,20 @@ static void test_task(void)
>         bpf_iter_task__destroy(skel);
>  }
>
> +static void test_task_sleepable(void)
> +{
> +       struct bpf_iter_task *skel;
> +
> +       skel = bpf_iter_task__open_and_load();
> +       if (CHECK(!skel, "bpf_iter_task__open_and_load",
> +                 "skeleton open_and_load failed\n"))
> +               return;
> +
> +       do_dummy_read(skel->progs.dump_task_sleepable);
> +
> +       bpf_iter_task__destroy(skel);
> +}
> +
>  static void test_task_stack(void)
>  {
>         struct bpf_iter_task_stack *skel;
> @@ -1252,6 +1266,8 @@ void test_bpf_iter(void)
>                 test_bpf_map();
>         if (test__start_subtest("task"))
>                 test_task();
> +       if (test__start_subtest("task_sleepable"))
> +               test_task_sleepable();
>         if (test__start_subtest("task_stack"))
>                 test_task_stack();
>         if (test__start_subtest("task_file"))
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task.c b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
> index c86b93f33b32..bb4b63043533 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_task.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2020 Facebook */
>  #include "bpf_iter.h"
>  #include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
>
>  char _license[] SEC("license") = "GPL";
>
> @@ -23,3 +24,56 @@ int dump_task(struct bpf_iter__task *ctx)
>         BPF_SEQ_PRINTF(seq, "%8d %8d\n", task->tgid, task->pid);
>         return 0;
>  }
> +
> +// New helper added
> +static long (*bpf_access_process_vm)(
> +       struct task_struct *tsk,
> +       unsigned long addr,
> +       void *buf,
> +       int len,
> +       unsigned int gup_flags) = (void *)186;

This shouldn't be needed.
Since patch 1 updates tools/include/uapi/linux/bpf.h
it will be in bpf_helper_defs.h automatically.

> +
> +// Copied from include/linux/mm.h
> +#define FOLL_REMOTE 0x2000 /* we are working on non-current tsk/mm */

Please use C style comments only.

> +       numread = bpf_access_process_vm(task,
> +                                       (unsigned long)ptr,
> +                                       (void *)&user_data,
> +                                       sizeof(uint32_t),
> +                                       FOLL_REMOTE);

We probably would need to hide flags like FOLL_REMOTE
inside the helper otherwise prog might confuse the kernel.
In this case I'm not even sure that FOLL_REMOTE is needed.
I suspect gup_flags=0 in all cases will work fine.
We're not doing write here and not pining anything.
fast_gup is not necessary either.