Re: [PATCH bpf-next v5 06/11] bpf: Add verifier support for 16-byte returns in R0:R2

> bpf: Add verifier support for 16-byte returns in R0:R2
>
> LLVM 23 added support for returning a value in two registers for an
> __int128, or a struct/union whose size is greater than 8 but not more than
> 16 bytes. See LLVM patches [1] and [2].
>
> Before LLVM 23 the BPF backend could not return these values at all. A
> by-value struct or union return (of any size) was rejected at compile time
> with:
>
>   error: aggregate returns are not supported
>
> and an __int128 return failed later in the backend with:
>
>   fatal error: error in backend: unable to allocate function return #1
>
> Both are resolved in LLVM 23, which lowers such returns into the R0:R2
> register pair.
>
> This patch models that pair at calls to global and static BPF subprograms
> and at kfunc calls: R2 is marked alongside R0 at the call, propagated out
> of a callee at its exit, and held to the same scalar-only and no-stack-
> pointer rules that R0 already is.

This isn't a bug, but could the changelog be reworded in imperative mood
to match Documentation/process/submitting-patches.rst? The other commits
in this series follow that convention, so this one reads inconsistently.

> @@ -9858,11 +9867,15 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  	struct bpf_func_state *caller, *callee;
>  	struct bpf_reg_state *r0;
>  	bool in_callback_fn;
> +	u32 i, nregs;
>  	int err;
>
>  	callee = state->frame[state->curframe];
>  	r0 = &callee->regs[BPF_REG_0];
> -	if (r0->type == PTR_TO_STACK) {
> +	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;
> +	for (i = 0; i < nregs; i++) {
> +		if (callee->regs[ret_regs[i]].type != PTR_TO_STACK)
> +			continue;
>  		/* technically it's ok to return caller's stack pointer
>  		 * (or caller's caller's pointer) back to the caller,
>  		 * since these pointers are valid. Only current stack

The new R0:R2 return-register loop is placed before the
'if (callee->in_callback_fn)' branch, so it runs for callback frames as
well. A callback follows bpf_callback_t (return value in R0 only) and R2
holds an incoming argument: set_loop_callback_state() and
set_user_ringbuf_callback_state() both do
'callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3]', i.e. the
callback_ctx, which is PTR_TO_STACK for the common
'bpf_loop(n, cb, &local_var, 0)' idiom.

bpf_compute_subprog_ret_regs() sets ret_reg_pair purely from the
subprog's BTF return type and iterates every subprog, with no exclusion
for subprogs used as callbacks. So a static subprog whose BTF declares a
9..16 byte return and which is passed to bpf_loop() or
bpf_user_ringbuf_drain() gets ret_reg_pair = true, and at its BPF_EXIT
the still-live callback_ctx in R2 is inspected as if it were the upper
half of a return value.

Consequence: the callback exit is rejected with 'cannot return stack
pointer to the caller', a diagnostic that points at a register holding an
argument rather than a return value, and it fires before the
in_callback_fn R0 checks that would otherwise produce the accurate
message. Impact is limited (any such program is already ill-typed with
respect to bpf_callback_t, and compiler-generated code for a 16-byte
return does write R2), so this is a misleading-rejection /
modelling-scope defect rather than a memory-safety bug.

Would it make sense to compute nregs after the in_callback_fn branch, or
force nregs = 1 when callee->in_callback_fn, since a callback frame has
no R0:R2 return convention?

> @@ -9897,8 +9910,12 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  			return -EFAULT;
>  		}
>  	} else {
> -		/* return to the caller whatever r0 had in the callee */
> -		caller->regs[BPF_REG_0] = *r0;
> +		/*
> +		 * return to the caller whatever the callee had in the
> +		 * return register(s)
> +		 */
> +		for (i = 0; i < nregs; i++)
> +			caller->regs[ret_regs[i]] = callee->regs[ret_regs[i]];
>  	}

prepare_func_exit() has three call sites, and only the real BPF_EXIT one
(process_bpf_exit_full()) establishes the callee's return registers. The
other two are synthetic exits that explicitly set up R0 but not R2,
because R1-R5 were already reset to NOT_INIT above them:

In check_helper_call(), BPF_FUNC_tail_call inside a subprog: the
caller-saved reset loop sets R1-R5 to NOT_INIT, then only
'mark_reg_unknown(env, regs, BPF_REG_0)' runs before
'prepare_func_exit(env, &env->insn_idx)'.

In check_ld_abs() hidden BPF_EXIT inside a subprog: reset loop, then only
'mark_reg_known_zero(env, regs, BPF_REG_0)' before prepare_func_exit().

For a static subprog with ret_reg_pair set, the new copy loop therefore
propagates a NOT_INIT R2 into caller->regs[BPF_REG_2] on those paths.
This is fail-safe (no uninitialised value is treated as readable), but it
means a caller that reads the upper half of the 16-byte return is
rejected on that path with an opaque 'R2 !read_ok', so a subprog with a
>8 byte return that contains a bpf_tail_call() or LD_ABS is unverifiable,
and only R0 gets the conservative unknown-scalar treatment the commit
message describes.

Would symmetric handling be to mark R2 unknown next to R0 at both sites
(the tail-called program only defines R0, so R2 is genuinely undefined
there), or to reject the combination with an explicit message?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31740414277