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

On 8/8/26 12:27 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The `aggregate_ret_kfunc_c` test will fail to load instead of gracefully skipping on 32-bit kernels when compiled with LLVM >= 23, due to a mismatch in `__SIZEOF_INT128__` definitions causing a ksym resolution failure.
> --
>
> --- Patch [10]: [PATCH bpf-next v3 10/13] selftests/bpf: Add C tests for 16-byte returns in R0:R2 ---
> commit 34be80f3aa8159c8e9fb57e485947ec4dc335cca
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add C tests for 16-byte returns in R0:R2
>
> This commit adds selftests that exercise a 16-byte return value passed in the
> R0:R2 register pair. It covers __int128, 16-byte struct, and 16-byte union
> returns. It also adds tests for __int128 and 16-byte struct returns from a
> kfunc, with bpf_kfunc_call_test_i128() guarded by __SIZEOF_INT128__.
>
>> 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..e4317399d711b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
> [ ... ]
>
>> +static void test_kfunc_c(void)
>> +{
>> +	struct aggregate_ret_kfunc_c *skel;
>> +	bool supported;
>> +	int err;
>> +
>> +	skel = aggregate_ret_kfunc_c__open();
>> +	if (!ASSERT_OK_PTR(skel, "skel_kfunc_c_open"))
>> +		return;
>> +
>> +	supported = skel->rodata->has_reg_pair_ret;
>> +
>> +	/*
>> +	 * Where the JIT cannot hand the second half of a >8-byte kfunc return
>> +	 * back in R0:R2, bpf_add_kfunc_call() rejects the call with
>> +	 * -EOPNOTSUPP. Asking the kernel keeps this test free of a list of the
>> +	 * JITs that can, which would have to be updated as the rest of them
>> +	 * learn.
>> +	 */
>> +	err = aggregate_ret_kfunc_c__load(skel);
>> +	if (err == -EOPNOTSUPP)
>> +		supported = false;
>> +	else if (!ASSERT_OK(err, "skel_kfunc_c_load"))
>> +		goto out;
> [Severity: Medium]
> Will this load fail with an assertion error instead of gracefully skipping on
> 32-bit kernels when compiled with LLVM >= 23?
>
> If the host kernel is 32-bit, bpf_testmod is built without __SIZEOF_INT128__,
> omitting bpf_kfunc_call_test_i128 from the module BTF.
>
> However, because the BPF program aggregate_ret_kfunc_c.c is compiled for a
> 64-bit BPF target, __SIZEOF_INT128__ will be defined, and it will
> unconditionally reference the bpf_kfunc_call_test_i128 kfunc via the
> bpf_testmod_kfunc.h header.
>
> During aggregate_ret_kfunc_c__load(), libbpf will attempt to resolve the
> kfunc, but since it is missing from the 32-bit kernel's BTF, it will fail and
> return a relocation error (such as -ENOENT) rather than -EOPNOTSUPP. This
> would cause the ASSERT_OK(err) check to fail.

To avoid complexity, the current implementation should be okay. The CI tests
only for 64bit arch (x86_64, arm64 and s390x). Running selftests with 32bit arch
may fail (or fail with other tests too).