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

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

On 7/8/26 1:28 PM, sashiko-bot@kernel.org wrote:
> 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?

Good point. I need to test it for 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?

The same as above. Need tests with s390x.

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

Will test as the above.

>
>> +
>> +	return 0;
>> +}