Received-SPF: pass (google.com: domain of linux-kernel+bounces-131821-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 4 Apr 2024 11:33:16 -0500
From: David Vernet <void@manifault.com>
To: Yonghong Song <yonghong.song@linux.dev>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	martin.lau@linux.dev, song@kernel.org, yonghong.song@linux.dev,
	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
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs
 from BPF_PROG_TYPE_SYCALL
Message-ID: <20240404163316.GA385240@maniforge>
References: <20240404010308.334604-1-void@manifault.com>
 <20240404010308.334604-2-void@manifault.com>
 <36bb0747-bff4-4fad-93ca-dae406f14099@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Tv6BS8qXzcVxLJbx"
Content-Disposition: inline
In-Reply-To: <36bb0747-bff4-4fad-93ca-dae406f14099@linux.dev>
User-Agent: Mutt/2.2.13 (2024-03-09)


--Tv6BS8qXzcVxLJbx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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@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.

>
> > ---
> >   .../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,
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

--Tv6BS8qXzcVxLJbx
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQRBxU1So5MTLwphjdFZ5LhpZcTzZAUCZg7WTAAKCRBZ5LhpZcTz
ZN4gAP90V5S5vd7CuqZm6mofUeuq5mm/kMhfW9bBet5JoQBDugD/fsW+fYqnaO17
5idK41x/IUMxA1OcM/UDvRH72sKRtwU=
=iNe9
-----END PGP SIGNATURE-----

--Tv6BS8qXzcVxLJbx--