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

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.

[ ... ]
> 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.

[ ... ]
> @@ -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?

> +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?

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