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

[Yonghong Song <yonghong.song@linux.dev>]

On 4/4/24 9:33 AM, David Vernet 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@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.

Or you can remove __success from cgrp_kfunc_success.c, cpumask_success.c
and task_kfunc_success.c and also remove their corresponding
RUN_TESTS(cgrp_kfunc_success) and RUN_TESTS(task_kfunc_success).
For example, you do not have RUN_TESTS(cpumask_success) and it is okay.

Basically, those expect-to-succeed new programs should be verified
already even without RUN_TESTS.

>
> Thanks,
> David