Re: [PATCH bpf-next v2 10/13] selftests/bpf: Add C tests for 16-byte returns in R0:R2

On 8/4/26 1:47 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Unconditional use of `__int128` in the `bpf_testmod` kernel module causes build failures on architectures lacking 128-bit integer support.
> - [Low] Redundant BPF program FD validation after successful skeleton load.
> --
>
> --- Patch [10]: [PATCH bpf-next v2 10/13] selftests/bpf: Add C tests for 16-byte returns in R0:R2 ---
> commit 9e3dc1e0617b6a9acfcd02529cae781809d11bbc
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add C tests for 16-byte returns in R0:R2
>
> Add selftests that exercise a 16-byte return value passed in the R0:R2
> register pair, written in C so that they depend on the compiler lowering
> the register-pair return. Covered are an __int128 return, a 16-byte struct
> return (from a static and from a global subprogram) and a 16-byte union
> return, plus __int128 and 16-byte struct returns from a kfunc, for which
> bpf_kfunc_call_test_i128() and bpf_kfunc_call_test_ret_pair() are added to
> bpf_testmod.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c b/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
>> new file mode 100644
>> index 0000000000000..42017f89c4da6
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
>> @@ -0,0 +1,125 @@
> [ ... ]
>> +static void run_prog(struct bpf_program *prog, bool supported)
>> +{
>> +	char buf[64] = {};
>> +	int err, prog_fd;
>> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +		    .data_in = buf,
>> +		    .data_size_in = sizeof(buf),
>> +		    .repeat = 1,
>> +	);
>> +
>> +	if (!supported) {
>> +		test__skip();
>> +		return;
>> +	}
>> +
>> +	prog_fd = bpf_program__fd(prog);
>> +	if (!ASSERT_GE(prog_fd, 0, "prog_fd"))
>> +		return;
> [Severity: Low]
> Is this manual FD validation necessary here? According to the BPF skeleton API
> guarantees, after a successful skeleton load, skeleton-generated program
> fields are guaranteed to have valid FDs. Since run_prog() is called using
> programs from successfully loaded skeletons, the extra validation appears to
> be redundant.

Yes, the following
+	if (!ASSERT_GE(prog_fd, 0, "prog_fd"))
+		return;
is redundant. Will remove.

>
> [ ... ]
>> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> index eb0f9b5e18d85..2ceb34df472e6 100644
>> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
>> @@ -857,6 +857,18 @@ __bpf_kfunc int bpf_kfunc_call_test5(u8 a, u16 b, u32 c)
>>   	return 0;
>>   }
>>   
>> +__bpf_kfunc __int128 bpf_kfunc_call_test_i128(u64 a, u64 b)
>> +{
>> +	return (__int128)(((unsigned __int128)(a + b) << 64) | (a - b));
>> +}
> [Severity: Medium]
> Will this unconditional use of __int128 cause build failures on 32-bit
> architectures? It appears bpf_kfunc_call_test_i128 is not protected by
> an #ifdef __SIZEOF_INT128__ guard, which might break compilation of the
> bpf_testmod module on platforms lacking 128-bit integer support.

Ack, will guard with __SIZEOF_INT128__.

>
> [ ... ]
>> @@ -1390,6 +1402,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test2)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test3)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test4)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test5)
>> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_i128)
> Should this BTF ID registration also be guarded to prevent compilation
> or pahole errors when the function is omitted on 32-bit builds?

Ack, will guard.

>
>> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ret_pair)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_stack_arg)
> [ ... ]
>> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> index c36bb911defa7..e6e59fdce33cf 100644
>> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> @@ -118,6 +127,8 @@ int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
>>   struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
>>   long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
>>   int bpf_kfunc_call_test5(__u8 a, __u16 b, __u32 c) __ksym;
>> +__int128 bpf_kfunc_call_test_i128(__u64 a, __u64 b) __ksym;
> Does this unconditionally declared prototype also need to be wrapped
> with an #ifdef __SIZEOF_INT128__ guard?

Ack, will guard.

>