Re: [PATCH bpf-next 10/12] selftests/bpf: Add C tests for by-value arguments up to 16 bytes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `take_with_ptr_global` and its test are missing the `#if defined(__clang__)` guard used for other global functions taking structs by value, which will cause the test to fail when built with GCC.
- [Medium] `take_two_pairs_global` lacks the surrounding `int` arguments explicitly promised in the commit message to catch slot counting bugs.
--

commit a3823ad45ae612d86e218eeec89944cc894c7843
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add C tests for by-value arguments up to 16 bytes

This commit extends the by-value argument tests with aggregate cases written
in C. It relies on the compiler to lower arguments into register pairs instead
of using hand-written register layouts.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_int128_arg.c b/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
> index 044fc4a800554..b10fb4aeb4056 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
[ ... ]
> +__noinline __u64 take_two_pairs_global(struct pair p, struct pair q)
> +{
> +	return p.lo + p.hi + q.lo + q.hi;
> +}
> +
> +SEC("tc")
> +__success __retval(0)
> +int aggregate_arg_two_structs_c_test(struct __sk_buff *skb)
> +{
> +	__u64 a = skb->len ^ MIX_A;
> +	__u64 b = skb->len ^ MIX_B;
> +	struct pair p = { .lo = a, .hi = b };
> +	struct pair q = { .lo = b, .hi = a };
> +
> +	if (take_two_pairs_global(p, q) != 2 * (a + b))
> +		return 1;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this function signature and test implementation match the strategy
described in the commit message?

The commit message explicitly states a defensive testing strategy of having
"two struct arguments in a row, each with an int argument on either side so
that a wrong slot count shows up as a wrong value in the parameters around
it".

However, take_two_pairs_global completely omits these surrounding int
arguments. Can these be added to properly validate register slot counting?

[ ... ]
> +__noinline __u64 take_with_ptr_global(struct with_ptr s)
> +{
> +	return s.x;
> +}
> +
> +SEC("tc")
> +__failure __msg("type STRUCT in take_with_ptr_global() is not composed of scalars")
> +int aggregate_arg_ptr_member_fail(struct __sk_buff *skb)
> +{
> +	struct with_ptr s = { .p = skb, .x = skb->len };
> +
> +	return take_with_ptr_global(s);
> +}

[Severity: Medium]
Should take_with_ptr_global and its test aggregate_arg_ptr_member_fail
be enclosed within the #if defined(__clang__) guard?

Other global functions taking structs by value in this file are guarded.
Since GCC passes aggregates by invisible reference, it will produce a
different BTF signature (a pointer instead of a struct).

Will this cause the test to fail when compiled with GCC, as the verifier
might not emit the specific "type STRUCT ... is not composed of scalars"
failure message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904050957.3976119-1-yonghong.song@linux.dev?part=10