Re: [PATCH bpf-next v6 4/4] selftests/bpf: Test parameterized task BPF iterators.

Kui-Feng Lee
2022-08-25T01:07:59Z

On Wed, 2022-08-24 at 15:30 -0700, Andrii Nakryiko wrote:
> On Fri, Aug 19, 2022 at 3:09 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> >
> > Test iterators of vma, files, and tasks of tasks.
> >
> > Ensure the API works appropriately to visit all tasks,
> > tasks in a process, or a particular task.
> >
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_iter.c       | 284
> > +++++++++++++++++-
> >  .../selftests/bpf/prog_tests/btf_dump.c       |   2 +-
> >  .../selftests/bpf/progs/bpf_iter_task.c       |   9 +
> >  .../selftests/bpf/progs/bpf_iter_task_file.c  |   9 +-
> >  .../selftests/bpf/progs/bpf_iter_task_vma.c   |   6 +-
> >  .../bpf/progs/bpf_iter_uprobe_offset.c        |  35 +++
> >  6 files changed, 326 insertions(+), 19 deletions(-)
> >  create mode 100644
> > tools/testing/selftests/bpf/progs/bpf_iter_uprobe_offset.c
>
[...]
> > +
> >  void test_bpf_iter(void)
> >  {
> > +       if (!ASSERT_OK(pthread_mutex_init(&do_nothing_mutex, NULL),
> > "pthread_mutex_init"))
> > +               return;
> > +
>
> ditto, too paranoid, IMO

Right, for test cases, we can ease checks.
I would like to have something like

INFALLIBLE(pthread_mutex_init(......)...);

It will crash at the first point.

>
> >         if (test__start_subtest("btf_id_or_null"))
> >                 test_btf_id_or_null();
> >         if (test__start_subtest("ipv6_route"))
>
> [...]
>
> > diff --git
> > a/tools/testing/selftests/bpf/progs/bpf_iter_uprobe_offset.c
> > b/tools/testing/selftests/bpf/progs/bpf_iter_uprobe_offset.c
> > new file mode 100644
> > index 000000000000..825ca86678bd
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_uprobe_offset.c
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> > +#include "bpf_iter.h"
> > +#include <bpf/bpf_helpers.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +__u32 unique_tgid_cnt = 0;
> > +uintptr_t address = 0;
> > +uintptr_t offset = 0;
> > +__u32 last_tgid = 0;
> > +__u32 pid = 0;
> > +
> > +SEC("iter/task_vma") int get_uprobe_offset(struct
> > bpf_iter__task_vma *ctx)
>
> please keep SEC() on separate line
>
> > +{
> > +       struct vm_area_struct *vma = ctx->vma;
> > +       struct seq_file *seq = ctx->meta->seq;
> > +       struct task_struct *task = ctx->task;
> > +
> > +       if (task == NULL || vma == NULL)
> > +               return 0;
> > +
> > +       if (last_tgid != task->tgid)
> > +               unique_tgid_cnt++;
> > +       last_tgid = task->tgid;
> > +
> > +       if (task->tgid != pid)
> > +               return 0;
> > +
> > +       if (vma->vm_start <= address && vma->vm_end > address) {
> > +               offset = address - vma->vm_start + (vma->vm_pgoff
> > << 12);
>
> it's best not to assume page_size is 4K, you can pass actual value
> through global variable from user-space (we've previously fixed a
> bunch of tests with fixed page_size assumption as they break some
> platforms, let's not regress that)

A getpagesize() helper would help :)

>
> > +               BPF_SEQ_PRINTF(seq, "OK\n");
> > +       }
> > +       return 0;
> > +}
> > --
> > 2.30.2
> >