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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardcoded little-endian assumptions in struct and `__int128` register unpacking will cause selftest failures on big-endian architectures like s390x.
--

--- Patch [10]: [PATCH bpf-next 10/12] selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns ---
commit dcf423ead16608630f40807ad0cde33d495c941d
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns

Add inline-asm and subprogram tests for more coverage, including
BPF-to-BPF cases, kfunc cases, backtracking and liveness. Both positive
and negative tests are added.

> 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..d6300b31733d8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_run.c

[ ... ]

> @@ -0,0 +1,160 @@

[ ... ]

> +SEC("tc")
> +int aggregate_ret_asm_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)
> +		return 3;
> +	if (lo != b)
> +		return 4;

[Severity: Medium]
The test expects the high 64 bits (a) in r2 and the low 64 bits (b) in
r0. On a big-endian ABI, the most significant 64 bits (a) will be returned
in the first register (r0).

Will this cause the test to fail on big-endian architectures like s390x?

> +
> +	return 0;
> +}
> +
> +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 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]
The bitwise extraction (int)hi assumes the 4-byte b is in the lower 32
bits of r2. On big-endian, wouldn't b be placed in the upper 32 bits of
the register, causing this assertion to fail?

> +
> +	/* 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;

[Severity: Medium]
The bitwise extraction (int)lo and (lo >> 32) assumes a little-endian
register layout for the returned 8-byte struct. On big-endian, wouldn't a
be in the upper 32 bits and b in the lower 32 bits?

> +
> +	return 0;
> +}

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