Re: [PATCH bpf-next v2 11/13] selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns

On 8/4/26 1:52 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The inline assembly tests verifying kfunc struct returns and __int128 values incorrectly assume little-endian register packing, which will cause deterministic test failures on big-endian architectures.
> --
>
> --- Patch [11]: [PATCH bpf-next v2 11/13] selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns ---
> commit 43d5815a2f48399212b5d17d74ef9350c219fc0e
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns
>
> Add inline-asm tests, which do not depend on the compiler lowering a
> register-pair return and so run regardless of the LLVM version, covering
> what the C tests cannot reach.
>
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_run.c b/tools/testing/selftests/bpf/progs/aggregate_ret_run.c
>> new file mode 100644
>> index 0000000000000..fac813531b4fe
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_run.c
> [ ... ]
>
>> +SEC("tc")
>> +int aggregate_ret_asm_kfunc_test(struct __sk_buff *skb)
>> +{
>> +	__u64 a = skb->len;
>> +	__u64 b = skb->len ^ 0xdeadbeefULL;
>> +	__u64 lo, hi;
>> +
>> +	asm volatile (
>> +	"r1 = %[a];"
>> +	"r2 = %[b];"
>> +	"call %[kfunc];"
>> +	"%[lo] = r0;"
>> +	"%[hi] = r2;"
>> +	: [lo]"=r"(lo), [hi]"=r"(hi)
>> +	: [a]"r"(a), [b]"r"(b), [kfunc]"i"(bpf_kfunc_call_test_i128)
>> +	: "r0", "r1", "r2", "r3", "r4", "r5"
>> +	);
>> +	if (hi != a + b)
>> +		return 1;
>> +	if (lo != a - b)
>> +		return 2;
> [Severity: Medium]
> Does this assume little-endian register packing for the __int128 return value?
>
> On big-endian architectures, the high bits typically occupy the first register
> (R0) and the low bits occupy R2, which would cause this check to fail when
> testing kfunc __int128 returns.

For x86_64, arm64 and 64bit riscv, only arm64 supports both little and big
endian. For simplificty, I will guard the above (and below) for little endian
so x86_64, arm64 and 64bit riscv all covered properly with selftests.

>
> [ ... ]
>
>> +SEC("tc")
>> +int aggregate_ret_struct_test(struct __sk_buff *skb)
>> +{
>> +	__u64 a = skb->len;
>> +	__u64 b = skb->len ^ 0xdeadbeefULL;
>> +	__u64 lo, hi;
>> +
>> +	/* struct { u64 hi; u64 lo; }: R0 = hi, R2 = lo. */
>> +	asm volatile (
>> +	"r1 = %[a];"
>> +	"r2 = %[b];"
>> +	"call %[kfunc];"
>> +	"%[lo] = r0;"
>> +	"%[hi] = r2;"
>> +	: [lo]"=r"(lo), [hi]"=r"(hi)
>> +	: [a]"r"(a), [b]"r"(b), [kfunc]"i"(bpf_kfunc_call_test_ret_pair)
>> +	: "r0", "r1", "r2", "r3", "r4", "r5"
>> +	);
>> +	if (lo != a + b)
>> +		return 1;
>> +	if (hi != a - b)
>> +		return 2;
>> +
>> +	/* struct { u64 a; int b; }: R0 = a, low 32 bits of R2 = b. */
>> +	asm volatile (
>> +	"r1 = %[a];"
>> +	"r2 = %[b];"
>> +	"call %[kfunc];"
>> +	"%[lo] = r0;"
>> +	"%[hi] = r2;"
>> +	: [lo]"=r"(lo), [hi]"=r"(hi)
>> +	: [a]"r"(a), [b]"r"(b), [kfunc]"i"(bpf_kfunc_call_test_ret_li)
>> +	: "r0", "r1", "r2", "r3", "r4", "r5"
>> +	);
>> +	if (lo != a)
>> +		return 3;
>> +	if ((int)hi != ~(int)b)
>> +		return 4;
> [Severity: Medium]
> Will this check fail deterministically on big-endian systems?
>
> The test expects the 32-bit integer b to be in the lower 32 bits of R2. On
> big-endian platforms, b would occupy the upper 32 bits of R2, leaving the
> lower 32 bits as padding.
>
>> +	/* struct { int a; int b; }: 8 bytes, packed into R0; R2 is not used. */
>> +	asm volatile (
>> +	"r1 = %[a];"
>> +	"r2 = %[b];"
>> +	"call %[kfunc];"
>> +	"%[lo] = r0;"
>> +	: [lo]"=r"(lo)
>> +	: [a]"r"(a), [b]"r"(b), [kfunc]"i"(bpf_kfunc_call_test_ret_ii)
>> +	: "r0", "r1", "r2", "r3", "r4", "r5"
>> +	);
>> +	if ((int)lo != (int)a)
>> +		return 5;
>> +	if ((int)(lo >> 32) != (int)b)
>> +		return 6;
>> +
>> +	return 0;
>> +}
> [Severity: Medium]
> Could the layout of this 8-byte struct in R0 differ depending on endianness?
>
> The hardcoded cast to (int)lo reads the lower 32 bits of R0. On big-endian
> architectures, the first member of the struct (a) is located in the upper 32
> bits, so reading the lower 32 bits will yield b instead, causing a spurious
> test failure.
>