Re: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `test_int128_args` selftest will predictably fail on `aarch64` due to an unhandled ABI alignment requirement in the ARM64 BPF trampoline.
--

--- Patch [3]: [PATCH bpf v2 3/3] selftests/bpf: Add tests for >8 byte return value and 128-bit arguments ---
commit c4e66c7551048f829ca6d77d8b1b2e6467e4525e
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add tests for >8 byte return value and 128-bit arguments

The BPF trampoline preserves only 8 bytes of the target's return value
(R0), so attaching an fexit/fmod_ret/fsession program to a function that
returns a >8 byte value is now rejected by the verifier. Add a bpf_testmod
function returning __int128 and an fexit program that targets it. The
program is expected to fail to load with the "with a >8 byte return value
is not supported for this attach type" message.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> index 6f8c0bfb04155..d32198ee02dff 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
[ ... ]
> @@ -112,6 +113,39 @@ static void test_struct_many_args(void)
>  	tracing_struct_many_args__destroy(skel);
>  }
>  
> +static void test_int128_args(void)
> +{
> +	/*
> +	 * __int128 arguments are passed in a register pair on x86_64 and
> +	 * arm64, which the trampoline packs into two context slots. Other
> +	 * architectures pass a __int128 differently (e.g. s390x passes larger
> +	 * arguments by reference), so only exercise this on x86_64 and arm64.
> +	 */
> +#if defined(__x86_64__) || defined(__aarch64__)
> +	struct tracing_struct_int128 *skel;
> +	int err;
> +
> +	skel = tracing_struct_int128__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "tracing_struct_int128__open_and_load"))
> +		return;
> +
> +	err = tracing_struct_int128__attach(skel);
> +	if (!ASSERT_OK(err, "tracing_struct_int128__attach"))
> +		goto destroy_skel;
> +
> +	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
> +
> +	ASSERT_EQ(skel->bss->t_a, 1, "t:a");
> +	ASSERT_EQ(skel->bss->t_c, 3, "t:c");
        ^^^^

[Severity: Medium]
Will this test predictably fail on AArch64?

The test calls bpf_testmod_test_int128_arg(1, 2, 3). According to the AAPCS64
calling convention, a 16-byte aligned argument (__int128) must be passed in an
even-odd register pair. Thus, the integer 'a' goes to w0, x1 is skipped for
alignment padding, the __int128 'b' goes to x2 and x3, and the long 'c' goes
to x4.

However, the pre-existing logic in the ARM64 BPF trampoline fails to account
for this padding. It solely calculates register usage based on argument sizes:

arch/arm64/net/bpf_jit_comp.c:calc_arg_aux() {
	...
	for (i = 0, nregs = 0; i < m->nr_args; i++) {
		slots = (m->arg_size[i] + 7) / 8;
		if (nregs + slots <= 8) /* passed through register ? */
			nregs += slots;
	...
}

Because it incorrectly assumes the arguments use exactly 4 contiguous
registers (x0 through x3), it only saves those.

As a result, when the test's BPF program reads ctx[3] expecting 'c', it
actually reads x3 (the upper 64 bits of 'b', which is 0).

Would it be better to fix the ARM64 trampoline padding handling first, or
exclude AArch64 from this test for now to prevent CI failures?

> +	ASSERT_EQ(skel->bss->t_ret, 6, "t ret");
> +
> +destroy_skel:
> +	tracing_struct_int128__destroy(skel);
> +#else
> +	test__skip();
> +#endif
> +}

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