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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The kfuncs `bpf_kfunc_call_test_i128` and `bpf_kfunc_call_test_ret_pair` are conditionally compiled out of `bpf_testmod.ko` on architectures other than x86_64 and arm64, but are unconditionally compiled and declared as strong `__ksym` externs in the selftest object `verifier_aggregate_ret.c`. This will cause libbpf to fail loading the entire object on unsupported architectures (when compiled with LLVM 23+), failing the entire test suite even for programs that don't use the kfuncs.
--

--- Patch [8]: [PATCH bpf-next v6 08/10] selftests/bpf: Add C tests for 16-byte returns in R0:R2 ---
commit 00f68ada7c423dbadb8514b3c5f6c309f446bc99
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.

The R0:R2 convention is only emitted by LLVM 23 and newer, and a by-value
aggregate return does not compile at all before that, so the programs sit
behind a __clang_major__ guard. An older compiler builds the dummy test in
the #else branch instead, which keeps the object non-empty and says in its
description why nothing was exercised.

The kfunc tests are tagged __arch_x86_64/__arch_arm64 and skip elsewhere.
Those are the architectures whose JIT advertises
bpf_jit_supports_kfunc_ret_reg_pair(), which bpf_add_kfunc_call() requires
before it accepts a kfunc returning more than 8 bytes, and they are also
the only ones building the kfuncs.

Acked-by: Eduard Zingerman <[email protected]>
Signed-off-by: Yonghong Song <[email protected]>

> 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 7d81070eefe7f..426000f9a14fa 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
[ ... ]
> @@ -131,6 +136,10 @@ 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;
> +#ifdef __SIZEOF_INT128__
> +__int128 bpf_kfunc_call_test_i128(__u64 a, __u64 b) __ksym;
> +#endif
> +struct prog_test_ret_pair bpf_kfunc_call_test_ret_pair(__u64 a, __u64 b) __ksym;

[Severity: Medium]
Should these kfuncs be declared with the __weak attribute?

While the test programs in verifier_aggregate_ret.c use the __arch_x86_64 and
__arch_arm64 tags, they still unconditionally reference these kfuncs in the
C code. Without the __weak attribute, Clang emits them as strong externs in
the BPF object file.

When libbpf attempts to load the verifier_aggregate_ret object on unsupported
architectures (when compiled with LLVM 23+), won't it fail to resolve these
strong externs because they are conditionally compiled out of bpf_testmod.ko?
This could cause the entire object load to fail, preventing even the
non-kfunc tests in the same object from running.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8