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

[Yonghong Song <yonghong.song@linux.dev>]

On 7/10/26 11:38 AM, sashiko-bot@kernel.org wrote:
> 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?

Another arch difference between x86_64 and arm64. x84_64 allows __int128 at
any position in an array. But arm64 allows __int128 must be even position.

I will fix it.

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