Re: [PATCH v3 bpf-next 1/6] bpf: Add unwind_goto and unwind_exit insns

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: dead code removal after unwind_goto drops speculative verification
--

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

bpf: Add unwind_goto and unwind_exit insns

This patch introduces unwind_goto and unwind_exit BPF instructions to
support zero-cost stack unwinding for Rust Drop, C++ destructors, and C
cleanup attributes. It updates the BPF verifier to track unwinding state,
validate landing pads, and explore speculative execution paths following
unwind_goto.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 409b9104c723e..9ae3a81604483 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -666,6 +666,10 @@ void bpf_opt_hard_wire_dead_code_branches(struct bpf_verifier_env *env)
>  		if (!bpf_insn_is_cond_jump(insn->code))
>  			continue;
>  
> +		/* unwind_exit of a callee that never exits needs it too */
> +		if (bpf_is_unwind_goto(insn) && aux_data[i + 1 + insn->off].seen)
> +			continue;
> +
>  		if (!aux_data[i + 1].seen)
>  			ja.off = insn->off;
>  		else if (!aux_data[i + 1 + insn->off].seen)

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 01434e1d23b59..6c56675c3298d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -18759,6 +18826,23 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		return -EINVAL;
>  	}
>  
> +	if (bpf_is_unwind_goto(insn)) {
> +		if (!env->cur_state->unwound)
> +			return 0;
> +		env->cur_state->unwound = false;
> +		/*
> +		 * unwind_exit changed its return address, so the return is
> +		 * mispredicted and what follows unwind_goto runs speculatively.
> +		 */
> +		if (!env->bypass_spec_v1) {
> +			err = sanitize_speculative_path(env, NULL, *insn_idx + 1, *insn_idx);
> +			if (err)
> +				return err;
> +		}
> +		*insn_idx += insn->off;
> +		return 0;
> +	}

[Severity: High]
When a static callee never exits normally and only leaves via unwind_exit,
can bpf_opt_remove_dead_code() delete the speculatively verified
fallthrough instructions after unwind_goto and shift unverified live code
into the return misprediction path?

For a loader with CAP_BPF (so is_priv is true) and without CAP_PERFMON (so
env->bypass_spec_v1 is false), check_cond_jmp_op() pushes the fallthrough
at *insn_idx + 1 via sanitize_speculative_path() with vstate->speculative
set to true.

Because the callee never returns normally, those fallthrough instructions
are only ever visited speculatively. In sanitize_mark_insn_seen(), seen is
only updated for non-speculative states:

kernel/bpf/verifier.c:sanitize_mark_insn_seen() {
	if (!vstate->speculative)
		env->insn_aux_data[env->insn_idx].seen = env->pass_cnt;
}

This leaves aux_data[i + 1].seen as 0.

Unlike ordinary conditional jumps where
bpf_opt_hard_wire_dead_code_branches() rewrites the jump into an
unconditional BPF_JA when !aux_data[i + 1].seen (which eliminates branch
misprediction into i + 1), bpf_opt_hard_wire_dead_code_branches() preserves
unwind_goto when aux_data[i + 1 + insn->off].seen is set.

Immediately afterward, bpf_opt_remove_dead_code() removes all instructions
with !aux_data[idx].seen:

kernel/bpf/fixups.c:bpf_opt_remove_dead_code() {
	for (i = 0; i < insn_cnt; i++) {
		int j;

		j = 0;
		while (i + j < insn_cnt && !aux_data[i + j].seen)
			j++;
		if (!j)
			continue;

		err = verifier_remove_insns(env, i, j);
}

Doesn't this cause two problems when the CPU mispredicts the return to the
instruction immediately following unwind_goto?

1. If speculative verification hit an unsafe instruction in the fallthrough
block and do_check() set insn_aux->nospec = true, verifier_remove_insns()
deletes that instruction and its aux_data before bpf_convert_ctx_accesses()
can emit BPF_ST_NOSPEC().

2. If the fallthrough block was a benign dead block (such as r0 = 0; exit)
followed by an unrelated live basic block while the landing pad is
elsewhere, deleting the dead block shifts that unrelated live block directly
into i + 1 (or leaves unwind_goto at the end of the subprogram if the
landing pad is at a negative offset), where the live block will execute
speculatively on return misprediction under the post-call register state
without having been verified for that state.

[ ... ]

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