Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs from BPF_PROG_TYPE_SYCALL

[Andrii Nakryiko]

On Thu, Apr 4, 2024 at 9:33 AM David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 04, 2024 at 09:04:19AM -0700, Yonghong Song wrote:
> >
> > On 4/3/24 6:03 PM, David Vernet wrote:
> > > Now that we can call some kfuncs from BPF_PROG_TYPE_SYSCALL progs, let's
> > > add some selftests that verify as much. As a bonus, let's also verify
> > > that we can't call the progs from raw tracepoints.
> > >
> > > Signed-off-by: David Vernet <void@xxxxxxxxxxxxx>
> >
> > Ack with some comments below.
> >
> > Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
>
> Thanks for the review. It looks like accidentally replied directly to
> me, so I'll re-add the missing cc's.
>

And dropped bpf@vger :) adding back

> >
> > > ---
> > >   .../selftests/bpf/prog_tests/cgrp_kfunc.c     |  1 +
> > >   .../selftests/bpf/prog_tests/task_kfunc.c     |  1 +
> > >   .../selftests/bpf/progs/cgrp_kfunc_common.h   | 21 +++++++++++++++++++
> > >   .../selftests/bpf/progs/cgrp_kfunc_failure.c  |  4 ++++
> > >   .../selftests/bpf/progs/cgrp_kfunc_success.c  |  4 ++++
> > >   .../selftests/bpf/progs/cpumask_common.h      | 19 +++++++++++++++++
> > >   .../selftests/bpf/progs/cpumask_failure.c     |  4 ++++
> > >   .../selftests/bpf/progs/cpumask_success.c     |  3 +++
> > >   .../selftests/bpf/progs/task_kfunc_common.h   | 18 ++++++++++++++++
> > >   .../selftests/bpf/progs/task_kfunc_failure.c  |  4 ++++
> > >   .../selftests/bpf/progs/task_kfunc_success.c  |  4 ++++
> > >   11 files changed, 83 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > index adda85f97058..73f0ec4f4eb7 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/cgrp_kfunc.c
> > > @@ -102,6 +102,7 @@ void test_cgrp_kfunc(void)
> > >             run_success_test(success_tests[i]);
> > >     }
> > > +   RUN_TESTS(cgrp_kfunc_success);
> > >     RUN_TESTS(cgrp_kfunc_failure);
> > >   cleanup:
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > index d4579f735398..3db4c8601b70 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/task_kfunc.c
> > > @@ -94,5 +94,6 @@ void test_task_kfunc(void)
> > >             run_success_test(success_tests[i]);
> > >     }
> > > +   RUN_TESTS(task_kfunc_success);
> > >     RUN_TESTS(task_kfunc_failure);
> > >   }
> >
> > The above RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success)
> > will do duplicate work for *existing* bpf programs in their respective
> > files. I think we still prefer to have cgrp_kfunc_success tests
> > in cgrp_kfunc.c to make it easy to cross check. But in order to
> > remove duplicate work, one option is to make other non-RUN_TESTS
> > programs in those files not auto-loaded and their corresponding
> > prog_tests/*.c file need to explicitly enable loading the problem.
>
> Good point, and yes I agree with that approach of not auto-loading
> non-RUN_TESTS programs. Considering that we have a  __success BTF tag to
> say, "this prog should successfully load", it seems odd that we'd also
> automatically load and validate progs that _didn't_ specify that tag as
> well. At that point, I'm not sure what value the tag is bringing. Also,

Just more explicitness (if desired). Normally __success would be
augmented by __msg() or __retval(). I'd feel uncomfortable just
silently skipping programs that are not marked with __success, as it
would be too easy to accidentally forget to add it and not know that
the BPF program is not tested.

I'd say that RUN_TESTS-based programs should be kept separate from any
other BPF programs that have a custom user-space testing part, though.

About the patch itself. I don't really see much point in adding
*_KFUNC_LOAD_TEST macros. They are used once or twice in total, while
obscuring *what* is actually being tested. Unless you expect to add 5+
more copies of them, I'd just inline them explicitly.

> that was the expected behavior before RUN_TESTS() was introduced, so it
> hopefully shouldn't cause much if any churn.
>
> > Maybe the current patch is okay even with duplicated work as it
> > should not take much time to verify those tiny problems.
>
> IMO it should be fine for now as the overhead for validating and loading
> these progs is low, but it'd definitely be good to address this problem
> in a follow-up. I don't think it should take too much effort -- AFAICT
> we'd just have to mark a test spec as invalid if it didn't have any BTF
> test tags. Ideally I'd like to separate that from this patch set, but I
> can do it here if folks want.
>
> Thanks,
> David