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

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.

[ ... ]

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

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