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

On Mon, 2026-08-10 at 17:10 -0700, Yonghong Song wrote:
> 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. aggregate_ret_func.c exercises BPF-to-BPF
> returns: a global subprogram whose R2 the caller may read, ones that leave
> R2 uninitialised or holding a pointer, a static subprogram whose R2 stays
> precise under backtracking, R2 liveness across a call, and a >8 byte return
> at program exit. Six kfuncs returning aggregates by value are added to
> bpf_testmod, and aggregate_ret_run.c calls them from inline asm to check
> what comes back in R0:R2.
> 
> A negative arena test is added as well: a global subprogram with a
> register-pair return that leaves an arena pointer in R2 is rejected, since
> an arena pointer is only a valid return value when it is returned in R0
> alone.
> 
> Three cases cover the boundaries of the new convention:
> 
>  - A return value larger than 16 bytes does not fit in R0:R2 and is
>    rejected by btf_distill_func_proto(), ahead of the KF_FASTCALL and
>    JIT-capability checks; one of the new kfuncs returns a 24-byte struct
>    for this. The equivalent for a BPF subprogram cannot be written in C:
>    from LLVM 23 on, a by-value return larger than 16 bytes is lowered to an
>    sret pointer argument and the BTF the verifier reads says the function
>    returns void, so the size bound in btf_validate_return_type() only
>    guards hand-crafted BTF.
> 
>  - A static subprogram returning a struct that contains a pointer is
>    accepted, and the caller can use the returned pointer. Unlike a global
>    subprogram, whose caller models the return as an opaque scalar pair, a
>    static one is verified inline, so prepare_func_exit() hands the caller
>    real register state and the pointer stays tracked.
> 
>  - An extension cannot replace a function returning more than 8 bytes.
>    btf_check_type_match() does not catch this, since it compares return
>    types by btf_type->info alone and both an __int128 and a __u64 are
>    BTF_KIND_INT with no vlen, so the rejection has to come from
>    bpf_check_attach_target(). The test reuses the freplace failure harness
>    in fexit_bpf2bpf.c, with aggregate_ret_target.c providing a target whose
>    global subprogram returns in R0:R2.
> 
> The kfunc tests need the JIT to place the second half of a return value
> into R2, which bpf_add_kfunc_call() only allows where
> bpf_jit_supports_kfunc_ret_reg_pair() is true. In aggregate_ret_kfunc.c the
> two tests that depend on getting past that check are tagged
> __arch_x86_64/__arch_arm64/__arch_riscv64; the others are rejected earlier
> (on KF_FASTCALL, on a >16 byte return, and on reading R2 after an 8-byte
> struct return). In aggregate_ret_run.c the kfunc-calling programs are
> dropped from the object when the load reports -EOPNOTSUPP and their
> subtests are skipped, and the __int128 inline-asm test is split into a
> BPF-to-BPF half, which needs no JIT capability and runs everywhere, and a
> kfunc half.
> 
> Five of the six new kfuncs return a struct or union by value and take
> arguments, so they join the x86_64/arm64 guard added in the previous patch;
> see the comment there. That includes the one returning only 8 bytes: s390x
> hands back a by-value aggregate through an sret pointer whatever its size,
> so its declared arguments are displaced just the same and pahole skips the
> function. The sixth returns 24 bytes but takes no argument, leaving nothing
> for the sret pointer to displace, and pahole does encode it there.
> Both objects calling the guarded kfuncs reference every one of them from
> __kfunc_btf_root(), so neither can load at all where those kfuncs are
> absent; the aggregate_ret_kfunc tests and the four aggregate_ret_run
> subtests are skipped as a group elsewhere. aggregate_ret_func.c calls no
> kfunc and keeps running everywhere.
> 
> R0 holds bytes 0..7 of a return value and R2 bytes 8..15, so where a member
> sits inside a register depends on the endianness of the target. The checks
> in aggregate_ret_run.c that read a member out of half a register are built
> for a little-endian target only; arm64 is the one JIT implementing the pair
> that can be built big endian, and that configuration is left for later.

Nit: is such long commit message really warranted?
     commit messages seem to be too verbose all across this series.

> 
> Signed-off-by: Yonghong Song <[email protected]>
> ---

...

> +static void test_run(void)
> +{
> +	struct aggregate_ret_run *skel;
> +	bool kfunc_ok = true;
> +	int err;
> +
> +	/*
> +	 * Every program in this object shares __kfunc_btf_root(), so where the
> +	 * testmod kfuncs are absent the object cannot load at all -- including
> +	 * for the kfunc-free "asm" subtest.
> +	 */
> +	if (!has_ret_pair_kfuncs()) {
> +		run_subtest("asm", NULL, false);
> +		run_subtest("asm_kfunc", NULL, false);
> +		run_subtest("struct", NULL, false);
> +		run_subtest("union", NULL, false);
> +		return;
> +	}
> +
> +	skel = aggregate_ret_run__open();
> +	if (!ASSERT_OK_PTR(skel, "skel_run_open"))
> +		return;
> +
> +	err = aggregate_ret_run__load(skel);
> +	if (err == -EOPNOTSUPP) {
> +		kfunc_ok = false;
> +		aggregate_ret_run__destroy(skel);
> +
> +		skel = aggregate_ret_run__open();
> +		if (!ASSERT_OK_PTR(skel, "skel_run_reopen"))
> +			return;
> +
> +		bpf_program__set_autoload(skel->progs.aggregate_ret_asm_kfunc_test, false);
> +		bpf_program__set_autoload(skel->progs.aggregate_ret_struct_test, false);
> +		bpf_program__set_autoload(skel->progs.aggregate_ret_union_test, false);

Please find a way to avoid these special cases and rely exclusively on
RUN_TESTS(). We have other tests that rely on specific archs / compiler
version and such handling is not usually necessary.

> +
> +		err = aggregate_ret_run__load(skel);
> +	}
> +	if (!ASSERT_OK(err, "skel_run_load"))
> +		goto out;
> +
> +	run_subtest("asm", skel->progs.aggregate_ret_asm_test, true);
> +	run_subtest("asm_kfunc", skel->progs.aggregate_ret_asm_kfunc_test, kfunc_ok);
> +	run_subtest("struct", skel->progs.aggregate_ret_struct_test, kfunc_ok);
> +	run_subtest("union", skel->progs.aggregate_ret_union_test, kfunc_ok);
> +
> +out:
> +	aggregate_ret_run__destroy(skel);
> +}
> +

...

> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_func.c b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c

...

> +SEC("tc")
> +__success __retval(0)
> +__log_level(2)
> +__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1")
> +__msg("mark_precise: frame0: regs=r6 stack= before 4: (07) r1 += -8")
> +__msg("mark_precise: frame0: regs=r6 stack= before 3: (bf) r1 = r10")
> +__msg("mark_precise: frame0: regs=r6 stack= before 2: (57) r6 &= 7")
> +__msg("mark_precise: frame0: regs=r6 stack= before 1: (bf) r6 = r2")
> +__msg("mark_precise: frame0: regs=r2 stack= before 12: (95) exit")
> +__msg("mark_precise: frame1: regs=r2 stack= before 11: (b7) r2 = 4")

Maybe add a log line showing that propagation process stopped here?

> +__naked int aggregate_ret_static_precise(void)
> +{
> +	asm volatile (
> +	"call %[static_agg_precise];"
> +	"r6 = r2;"		/* derived from the aggregate's second half */
> +	"r6 &= 7;"		/* keep it in [0, 7] to index the stack */
> +	"r1 = r10;"
> +	"r1 += -8;"
> +	"r1 += r6;"		/* ptr += scalar marks r6 (hence R2) precise */
> +	"r0 = 0;"
> +	"*(u8 *)(r1 + 0) = r0;"
> +	"r0 = 0;"
> +	"exit;"
> +	:
> +	: __imm(static_agg_precise)
> +	: __clobber_all);
> +}
> +
> +SEC("tc")
> +__success __retval(0)
> +__log_level(2)
> +__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1")
> +__msg("mark_precise: frame0: regs=r6 stack= before 4: (07) r1 += -8")
> +__msg("mark_precise: frame0: regs=r6 stack= before 3: (bf) r1 = r10")
> +__msg("mark_precise: frame0: regs=r6 stack= before 2: (57) r6 &= 7")
> +__msg("mark_precise: frame0: regs=r6 stack= before 1: (bf) r6 = r2")
> +__msg("mark_precise: frame0: regs=r2 stack= before 0: (85) call pc+9")

And here.

> +__naked int aggregate_ret_global_precise(void)
> +{
> +	asm volatile (
> +	"call %[global_agg_good];"
> +	"r6 = r2;"		/* derived from the aggregate's second half */
> +	"r6 &= 7;"		/* keep it in [0, 7] to index the stack */
> +	"r1 = r10;"
> +	"r1 += -8;"
> +	"r1 += r6;"		/* ptr += scalar marks r6 (hence R2) precise */
> +	"r0 = 0;"
> +	"*(u8 *)(r1 + 0) = r0;"
> +	"r0 = 0;"
> +	"exit;"
> +	:
> +	: __imm(global_agg_good)
> +	: __clobber_all);
> +}

...

> +SEC("tc")
> +__success __retval(0)
> +int aggregate_ret_global_struct(void *ctx)

Tbh, for this and similar tests I'd keep either C or asm versions,
keeping both seem excessive.

> +{
> +	__u64 lo, hi;
> +
> +	asm volatile (
> +	"call %[global_ret_struct];"
> +	"%[lo] = r0;"
> +	"%[hi] = r2;"
> +	: [lo]"=r"(lo), [hi]"=r"(hi)
> +	: __imm(global_ret_struct)
> +	: "r0", "r1", "r2", "r3", "r4", "r5");
> +	if (lo != 0x1234)
> +		return 1;
> +	if (hi != 0x5678)
> +		return 2;
> +	return 0;
> +}

...

> 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 000000000000..382ef3b90037
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_run.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +#include "../test_kmods/bpf_testmod_kfunc.h"
> +
> +typedef unsigned __int128 u128;
> +
> +/*
> + * Reference kfunc addresses to force those BTF to be emitted. Taking the address
> + * (rather than calling) avoids any dependence on the compiler lowering an __int128
> + * or struct return value, which the BPF backend only supports from LLVM 23 on.
> + */
> +void __kfunc_btf_root(void)
> +{
> +	asm volatile (""
> +	:
> +	: "r"(&bpf_kfunc_call_test_i128),
> +	  "r"(&bpf_kfunc_call_test_ret_pair),
> +	  "r"(&bpf_kfunc_call_test_ret_li),
> +	  "r"(&bpf_kfunc_call_test_ret_ii),
> +	  "r"(&bpf_kfunc_call_test_ret_uu));
> +}
> +
> +#define I128_ASM_LO 0xABCDabcd12345678ULL
> +#define I128_ASM_HI 0x1234567890abcdefULL
> +
> +static __naked __noinline u128 make_i128_asm(void)
> +{
> +	asm volatile (
> +	"r0 = %[lo] ll;"	/* low 64 bits */
> +	"r2 = %[hi] ll;"	/* high 64 bits */
> +	"exit;"
> +	:
> +	: __imm_const(lo, I128_ASM_LO), __imm_const(hi, I128_ASM_HI)
> +	);
> +}
> +
> +SEC("tc")
> +int aggregate_ret_asm_test(struct __sk_buff *skb)

Same here, let's keep either C or an asm variant.

> +{
> +	__u64 lo, hi;
> +
> +	asm volatile (
> +	"call %[callee];"
> +	"%[lo] = r0;"
> +	"%[hi] = r2;"
> +	: [lo]"=r"(lo), [hi]"=r"(hi)
> +	: [callee]"i"(make_i128_asm)
> +	: "r0", "r1", "r2", "r3", "r4", "r5"

Nit: we have __clobber_common macro.

> +	);
> +	if (lo != I128_ASM_LO)
> +		return 1;
> +	if (hi != I128_ASM_HI)
> +		return 2;
> +
> +	return 0;
> +}
> +
> +/*
> + * R0 holds bytes 0..7 of a kfunc return value and R2 bytes 8..15, so where a
> + * member sits inside a register depends on the endianness of the target.
> + * Although arm64 supports both little and big endian, for simplicity, only
> + * do little endian for now..
> + */
> +SEC("tc")
> +int aggregate_ret_asm_kfunc_test(struct __sk_buff *skb)
> +{

The tests below are very similar to those added in the patch
"selftests/bpf: Add C tests for 16-byte returns in R0:R2",
Maybe convert these to C and keep in the same file?

> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +	__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;
> +#endif
> +
> +	return 0;
> +}

...