Received-SPF: pass (google.com: domain of linux-kernel+bounces-132189-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240404010308.334604-1-void@manifault.com> <20240404010308.334604-2-void@manifault.com>
 <36bb0747-bff4-4fad-93ca-dae406f14099@linux.dev> <20240404163316.GA385240@maniforge>
In-Reply-To: <20240404163316.GA385240@maniforge>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Thu, 4 Apr 2024 15:16:56 -0700
Message-ID: <CAEf4Bza3rGMh=S7yb_Crf8v_SBTrp+NiKn0_frZnWpEi30DkPg@mail.gmail.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs
 from BPF_PROG_TYPE_SYCALL
To: David Vernet <void@manifault.com>
Cc: Yonghong Song <yonghong.song@linux.dev>, ast@kernel.org, daniel@iogearbox.net, 
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, 
	john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com, 
	haoluo@google.com, jolsa@kernel.org, linux-kernel@vger.kernel.org, 
	kernel-team@meta.com, bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 4, 2024 at 9:33=E2=80=AFAM David Vernet <void@manifault.com> wr=
ote:
>
> 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, le=
t'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@manifault.com>
> >
> > Ack with some comments below.
> >
> > Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
> 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/to=
ols/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/to=
ols/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_succes=
s)
> > 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