Re: [PATCH bpf-next 3/5] bpf, arm64: Support unwinding via r3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf, arm64: stale jump_target in bpf_jit_unwind_exit() skips landing pad
--

commit e9dd702da2c478ecd8523b3f071fba585759acfc
Author: Alexei Starovoitov <ast@kernel.org>

bpf, arm64: Support unwinding via r3

This commit adds arm64 JIT support for unwinding via r3 by replacing the
post-call conditional branch with an exception table entry and rewriting the
return address in the callee frame record upon unwinding exit.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 506c4201fec85..9b702930486a1 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
[ ... ]
> @@ -1781,6 +1863,13 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>  			emit_kfunc_arg_moves(ctx, fm);
>  		}
>  		emit_call(func_addr, ctx);
> +		if (bpf_jit_unwind_site(env, ctx->prog, i)) {
> +			ret = add_unwind_exentry(ctx, i + 2 + insn[1].off);
> +			if (ret)
> +				return ret;
> +			emit(A64_MOV(1, r0, A64_R(0)), ctx);
> +			return 1;
> +		}
>  		/*
>  		 * Call to arch_bpf_timed_may_goto() is emitted by the
>  		 * verifier and called with custom calling convention with
> @@ -1799,6 +1888,13 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>  		 * counter and the private stack are callee saved.
>  		 */
>  		emit(A64_BLR(dst), ctx);
> +		if (bpf_jit_unwind_site(env, ctx->prog, i)) {
> +			ret = add_unwind_exentry(ctx, i + 2 + insn[1].off);
> +			if (ret)
> +				return ret;
> +			emit(A64_MOV(1, bpf2a64[BPF_REG_0], A64_R(0)), ctx);
> +			return 1;
> +		}
>  		emit(A64_MOV(1, bpf2a64[BPF_REG_0], A64_R(0)), ctx);
>  		break;
>  	/* tail call */
> @@ -1808,6 +1904,9 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>  		break;
>  	/* function return */
>  	case BPF_JMP | BPF_EXIT:
> +		unw = bpf_jit_unwind_exit(env, ctx->prog, i);
> +		if (unw != BPF_EXIT_RETURNS)
> +			emit_unwind_ret_addr(ctx, unw == BPF_EXIT_LOOK_AT_R3);

[Severity: Critical]
Could stale jump_target metadata in env->insn_aux_data cause
bpf_jit_unwind_exit() or bpf_jit_unwind_site() to skip the landing pad when
unwinding?

In build_insn(), bpf_jit_unwind_site() elides the caller's post-call branch
and relies on the callee rewriting its return address via
emit_unwind_ret_addr(). Both bpf_jit_unwind_site() and bpf_jit_unwind_exit()
use bpf_is_jump_target() at JIT time to determine whether an instruction can
be reached by a jump rather than falling through from the preceding
instruction:

kernel/bpf/core.c:bpf_jit_unwind_exit() {
	...
	if (!i || bpf_is_jump_target(env, prog->aux->subprog_start + i) ||
	    insn[-1].code != (BPF_ALU64 | BPF_MOV | BPF_K) ||
	    insn[-1].dst_reg != BPF_REG_UNWIND)
		return BPF_EXIT_LOOK_AT_R3;
	return insn[-1].imm ? BPF_EXIT_UNWINDS : BPF_EXIT_RETURNS;
}

However, env->insn_aux_data[].jump_target is populated during bpf_check_cfg()
(which only marks branch targets, not fallthrough edges) and is not preserved
across post-CFG instruction removal passes in verifier_remove_insns():

kernel/bpf/fixups.c:verifier_remove_insns() {
	...
	memmove(aux_data + off,	aux_data + off + cnt,
		sizeof(*aux_data) * (orig_prog_len - off - cnt));
	env->insn_aux_data_len -= cnt;
}

For example, in a subprogram annotated with btf_decl_tag("unwind"):

1. Path A sets r3 = 1; goto L, while path B sets r3 = 0 and falls through to
   L, where L is a conditional branch whose target is dead (such as
   if r6 != 0 goto dead; followed by exit;).

2. bpf_check_cfg() marks L with jump_target = true (from goto L), but leaves
   the fallthrough exit at L + 1 with jump_target = false.

3. bpf_prune_dead_branches() or bpf_opt_hard_wire_dead_code_branches()
   rewrites L into goto +0 without setting jump_target on L + 1.

4. bpf_opt_remove_nops() -> verifier_remove_insns() removes the NOP at L,
   shifting exit into slot L (so goto L now targets exit directly with
   r3 = 1) and overwriting aux_data[L].jump_target with
   aux_data[L + 1].jump_target (false). A similar loss of jump_target can
   happen when bpf_remove_fastcall_spills_fills() turns a jump-target spill
   into NOPs that bpf_opt_remove_nops() removes, or when a NOP before the
   post-call if r3 != 0 goto pad is removed.

When bpf_int_jit_compile() -> build_body() -> build_insn() runs,
bpf_jit_unwind_exit() sees !bpf_is_jump_target() and insn[-1] as r3 = 0,
returning BPF_EXIT_RETURNS. As a result, build_insn() omits
emit_unwind_ret_addr() while also eliding the caller's if r3 != 0 goto pad
branch via bpf_jit_unwind_site().

When path A executes at runtime and exits with r3 = 1, won't execution bypass
the landing pad and fall through into the caller's normal return path where
the verifier assumed r3 == 0?

>  		/* Optimization: when last instruction is EXIT,
>  		   simply fallthrough to epilogue. */
>  		if (i == ctx->prog->len - 1)
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018010246.1643877-1-alexei.starovoitov@gmail.com?part=3