Received-SPF: pass (google.com: domain of linux-kernel+bounces-131846-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <fc7e06cd-ecd5-4a2a-8ca5-cd876aa87ac1@linux.dev>
Date: Thu, 4 Apr 2024 09:47:37 -0700
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Verify calling core kfuncs
 from BPF_PROG_TYPE_SYCALL
Content-Language: en-GB
To: David Vernet <void@manifault.com>
Cc: 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
References: <20240404010308.334604-1-void@manifault.com>
 <20240404010308.334604-2-void@manifault.com>
 <36bb0747-bff4-4fad-93ca-dae406f14099@linux.dev>
 <20240404163316.GA385240@maniforge>
From: Yonghong Song <yonghong.song@linux.dev>
In-Reply-To: <20240404163316.GA385240@maniforge>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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