[kernel-patches-daemon-bpf]

Pull request for series with
subject: bpf: Unwinding with unwind_goto and unwind_exit
version: 2
url: https://patchwork.kernel.org/series/1188607/

[kernel-patches-daemon-bpf]

Upstream branch: 4f84263
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-review-bot]

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

selftests/bpf: Add tests for unwind_goto and unwind_exit

This adds selftests for the unwind_goto and unwind_exit insns, written as
raw insns.  verifier_unwind.c covers verifier and JIT behavior, including
private stack, unwind_attach covers fentry, freplace and fexit attached to
a subprog that unwinds, and test_loader learns to run raw_tp progs.

> selftests/bpf: Add tests for unwind_goto and unwind_exit

TL;DR:
- Two selftests may not test what they claim: callee_never_returns and
  no_pruning_after_the_call can pass without the checks they target
- private_stack_unwinds __jited() patterns may fail with call depth accounting
  on x86-64

> diff --git a/tools/testing/selftests/bpf/progs/verifier_unwind.c b/tools/testing/selftests/bpf/progs/verifier_unwind.c
> new file mode 100644
> index 000000000000..a235c6143e10
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_unwind.c
> @@ -0,0 +1,1047 @@

[ ... ]

> +/* What follows unwind_goto is dead code. unwind_exit still has to find the pad. */
> +SEC("tc")
> +__description("unwind: the callee never returns")
> +__arch_x86_64
> +__arch_arm64
> +__load_if_JITed()
> +__success __retval(42)
> +__naked int callee_never_returns(void)
> +{
> +	asm volatile (
> +		"call always_unwinds;"
> +		unwind_goto(2)
> +		"r0 = 1;"
> +		"exit;"
> +		"r0 = 42;"
> +		"exit;"
> +		:
> +		: __imm_unwind_goto(2)
> +		: __clobber_all
> +	);
> +}

The comment says unwind_exit still has to find the pad.  That depends on
the exception added to bpf_opt_hard_wire_dead_code_branches():

    /* 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;

Can __retval(42) tell whether that exception is present?

Without it, unwind_goto(2) would become 'ja +2', because the fall-through
insn was never seen.  bpf_opt_remove_dead_code() would then drop the dead
insns and bpf_opt_remove_nops() would drop the resulting 'ja +0', leaving
'call; r0 = 42; exit' with no extable entry.

At run time, unwind_exit in always_unwinds calls bpf_unwind_target(), finds
no entry, hits WARN_ON_ONCE(!e) and falls back to ret_addr:

    if (WARN_ON_ONCE(!e))
        return ret_addr;

After dead code removal ret_addr is the pad ('r0 = 42'), so the prog still
returns 42 and the test passes.  The only sign of the problem is a warning
splat, which test_progs does not check for.

callee_never_unwinds below already uses __xlated() to pin the opposite
case, where the pad goes away together with unwind_goto.  Would something
like __xlated("1: unwind_goto pc+0") here show that unwind_goto stays when
only the fall-through is dead?

[ ... ]

> +/*
> + * After the call the two states differ only in the way the callee left.
> + * The second one must not be pruned, or nobody looks at the pad.
> + */
> +SEC("tc")
> +__description("unwind: the way the callee left is a part of the state")
> +__arch_x86_64
> +__arch_arm64
> +__load_if_JITed()
> +__failure __msg("R0 invalid mem access 'scalar'")
> +__naked int no_pruning_after_the_call(void)
> +{
> +	asm volatile (
> +		"r1 = *(u32 *)(r1 + %[len]);"
> +		"call same_regs;"
> +		unwind_goto(1)
> +		"exit;"
> +		"r0 = *(u64 *)(r0 + 0);"
> +		"exit;"
> +		:
> +		: __imm_const(len, offsetof(struct __sk_buff, len)),
> +		  __imm_unwind_goto(1)
> +		: __clobber_all
> +	);
> +}

Does this test exercise the new check in states_equal()?

    if (old->unwound != cur->unwound)
        return false;

It looks like it passes the same way with or without that check, because
the verifier never saves a checkpoint at the insn after the call in this
program.

unwind_goto is a prune point, but not a force checkpoint.  visit_insn()
only calls mark_force_checkpoint() for may_goto:

    mark_prune_point(env, t);
    if (bpf_is_may_goto_insn(insn))
        mark_force_checkpoint(env, t);

Without a force checkpoint, bpf_is_state_visited() only adds a new state
after at least 2 jumps and 8 insns since the last checkpoint:

    add_new_state = force_new_state;
    if (env->jmps_processed - env->prev_jmps_processed >= 2 &&
        env->insn_processed - env->prev_insn_processed >= 8)
        add_new_state = true;

With same_regs added by libbpf, the program is:

    0: r1 = *(u32 *)(r1 + len)
    1: call same_regs
    2: unwind_goto +1
    3: exit
    4: r0 = *(u64 *)(r0 + 0)
    5: exit
    6: r0 = 0               (same_regs)
    7: if r1 != 0 goto +1
    8: exit
    9: unwind_exit

check_cond_jmp_op() pushes the taken branch (insn 9) and continues with the
fall-through, so the first path reaches insn 2 after only 6 processed insns
(0, 1, 6, 7, 8, 2).  That is below 8, so no state is saved at insn 2, and
nothing before it saved one either.

When the unwound path comes back from insn 9 to insn 2, the explored state
list for insn 2 is empty.  Nothing can prune that path, so the verifier
always walks into the pad and reports "R0 invalid mem access 'scalar'".

If the unwound check were removed from states_equal(), would this test
still pass?  Today only pad_does_not_unlock would catch that, and only
because its first path happens to reach 8 insns before unwind_goto.

test_loader's default flags from testing_prog_flags() are
BPF_F_TEST_RND_HI32 and BPF_F_TEST_REG_INVARIANTS, not
BPF_F_TEST_STATE_FREQ.  The callx pruning tests in verifier_callx.c add
__flag(BPF_F_TEST_STATE_FREQ) for this reason.  Would adding the same flag
here save a state at insn 2 on the first path, so that a kernel without the
states_equal() check prunes the second path and the test fails as intended?

[ ... ]

> +/*
> + * x86-64 keeps the pointer to the private stack in r9 and saves it around a call.
> + * The value that is read back in the pad is there only when r9 is restored.
> + */
> +SEC("raw_tp")
> +__description("unwind: private stack, the callee unwinds")
> +__success __retval(47)
> +__load_if_JITed()
> +__arch_x86_64
> +__jited("	pushq	%r9")
> +__jited("	callq	0x{{.*}}")
> +__jited("	popq	%r9")
> +__jited("	jmp	L{{.*}}")
> +__jited("	popq	%r9")
> +__jited("	jmp	L{{.*}}")
> +__arch_arm64
> +__naked int private_stack_unwinds(void)

Can the x86-64 __jited() expectations fail on hosts that use call depth
accounting?

They require callq to be on the line right after pushq %r9.  In do_jit()
the BPF_CALL case emits push_r9(), then x86_call_depth_emit_accounting(),
then emit_call():

    if (priv_frame_ptr) {
        push_r9(&prog);
        ip += 2;
    }
    ip += x86_call_depth_emit_accounting(&prog, func, ip);
    if (emit_call(&prog, func, ip))

With CONFIG_MITIGATION_CALL_DEPTH_TRACKING=y and retbleed=stuff
(thunks_initialized), x86_call_depth_emit_accounting() adds a 'sarq $0x5,
%gs:...' instruction before calls to a BPF subprog.  A JITed subprog like
may_unwind_big_stack has no call thunk in its padding, so is_callthunk()
returns false and the accounting is emitted.

The disassembly then reads 'pushq %r9; sarq ...; callq ...', and
validate_msgs() reports a line mismatch for the callq pattern, so this
subtest fails on retbleed-affected machines such as Skylake booted with
retbleed=stuff.  The kernel itself is fine there.

Commit bf29346fc393 ("selftests/bpf: ignore call depth accounting for
retbleed in verifier tests") fixed the same pattern in
verifier_private_stack.c by putting __jited("...") between "pushq %r9" and
"callq".  Should this test do the same?

    __jited("\tpushq\t%r9")
    __jited("...")
    __jited("\tcallq\t0x{{.*}}")

---

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

In-Reply-To-Subject: selftests/bpf: Add tests for unwind_goto and unwind_exit
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39205301464

[kernel-patches-daemon-bpf]

Forwarding comment 6260714011 via email
In-Reply-To: 20261019051846.3012089-5-alexei.starovoitov@gmail.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20261019051846.3012089-5-alexei.starovoitov@gmail.com/

[kernel-patches-review-bot]

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

bpf: Add unwind_goto and unwind_exit insns

This commit adds two new BPF instructions, unwind_goto (a BPF_JCOND form)
and unwind_exit (a BPF_EXIT form), so that a callee can leave through
unwind_exit and the caller continues at a landing pad via the unwind_goto
that follows the call.  The verifier tracks how the callee left in a new
per-state "unwound" bit, and JITs implement the transfer with an extable
entry plus a rewritten return address instead of a branch.

> bpf: Add unwind_goto and unwind_exit insns

TL;DR:
- Possible build failure on MIPS with CONFIG_BPF_JIT=y: bpf_unwind_target()
  uses e->fixup, which MIPS lacks
- Verifier may skip checking the fall-through after unwind_goto, allowing
  possible Spectre-v1-style speculative execution

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 6e32db9777c2..da3691b47cc7 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -804,6 +804,24 @@ const struct exception_table_entry *search_bpf_extables(unsigned long addr)
>  	return e;
>  }
>
> +/*
> + * JITs don't emit a branch for unwind_goto. They add an extable entry
> + *   { .insn = ret_addr - 1, .fixup = pad - ret_addr }
> + * where ret_addr is the return address of the call in front of it. unwind_exit
> + * replaces the return address of its frame with what this function returns.
> + * ret_addr - 1, because what follows the call can have an entry of its own.
> + * No insn starts there, so a fault cannot hit the entry.
> + */
> +u64 bpf_unwind_target(u64 ret_addr)
> +{
> +	const struct exception_table_entry *e = search_bpf_extables(ret_addr - 1);
> +
> +	/* The verifier made sure that unwind_goto follows the call */
> +	if (WARN_ON_ONCE(!e))
> +		return ret_addr;
> +	return ret_addr + e->fixup;
                          ^^^^^^^^

Does this still build on MIPS with CONFIG_BPF_JIT=y?

bpf_unwind_target() sits in the CONFIG_BPF_JIT section of kernel/bpf/core.c,
so it is compiled for every architecture that has a JIT, and it reads
e->fixup.  On MIPS, struct exception_table_entry has no fixup member:

arch/mips/include/asm/extable.h:
    struct exception_table_entry
    {
            unsigned long insn;
            unsigned long nextinsn;
    };

MIPS selects HAVE_EBPF_JIT and some defconfigs (loongson3_defconfig for
example) set CONFIG_BPF_JIT=y, so this looks like it would fail with
"'const struct exception_table_entry' has no member named 'fixup'".  Until
now generic code only used e->insn.

None of the later patches in this series touch kernel/bpf/core.c or
arch/mips, so the build would stay broken through the end of the series.

Should bpf_unwind_target() only be built for JITs that opt in through
bpf_jit_supports_unwind(), or should the arch decode the target (a helper
next to bpf_jit_supports_unwind()), or should the JIT do the lookup itself?

> +}
> +
>  int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
>  		    char *sym)
>  {

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d13868846823..dd2d200a73c3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -18697,6 +18763,13 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		return -EINVAL;
>  	}
>
> +	if (bpf_is_unwind_goto(insn)) {
> +		if (env->cur_state->unwound)
> +			*insn_idx += insn->off;
> +		env->cur_state->unwound = false;
> +		return 0;
> +	}
> +
>  	if (opcode == BPF_JCOND) {
>  		struct bpf_verifier_state *cur_st = env->cur_state, *queued_st, *prev_st;
>  		int idx = *insn_idx;

When the callee left with unwind_exit, this only follows the pad.  The
fall-through insn after the call is never verified with the unwound state,
not even as a speculative path.

Is that enough given how the JITs implement it?  The x86 and arm64 patches
later in this series emit no branch for unwind_goto.  unwind_exit rewrites
the saved return address and then executes a normal ret, so the CPU's
return-address predictor still points at the insn after the call.  Each
unwind would then run the fall-through code speculatively before it is
redirected to the pad.

For a static subprog, prepare_func_exit() copies the callee's r0/r2 at
unwind_exit into the caller, and the callee may also have written caller
stack slots differently on that path.  So the fall-through code can run
speculatively with register and stack state the verifier never checked for
it.

check_cond_jmp_op() already handles the equivalent case for branches whose
outcome is known when !env->bypass_spec_v1:

    if (pred == 1) {
            ...
            if (!env->bypass_spec_v1) {
                    err = sanitize_speculative_path(env, insn, *insn_idx + 1, *insn_idx);
                    ...

but the unwind_goto path returns before reaching that logic and has no
equivalent.  This would matter for loaders with CAP_BPF but without
CAP_PERFMON (including BPF token setups) on x86-64 and arm64, where
bpf_bypass_spec_v1() is false and bpf-to-bpf calls are allowed.  For global
subprogs the gap does not arise, since check_func_call() gives both paths
the same state apart from the unwound bit.

Should the unwound case push a speculative fall-through state when
!env->bypass_spec_v1, for example with:

    sanitize_speculative_path(env, NULL, *insn_idx + 1, *insn_idx);

Alternatively, could the commit message explain why speculative execution
of the fall-through after a return mispredict is not a concern here?
Neither the commit message nor the selftests later in the series cover
speculation, and no later patch in the series changes this.

---

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

In-Reply-To-Subject: bpf: Add unwind_goto and unwind_exit insns
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39205301464

[kernel-patches-daemon-bpf]

Forwarding comment 6260757822 via email
In-Reply-To: 20261019051846.3012089-2-alexei.starovoitov@gmail.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20261019051846.3012089-2-alexei.starovoitov@gmail.com/

[kernel-patches-daemon-bpf]

Upstream branch: 868e56d
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-daemon-bpf]

Upstream branch: 4f84263
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-daemon-bpf]

Upstream branch: 25bb03f
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-daemon-bpf]

Upstream branch: 0867b7b
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-daemon-bpf]

Upstream branch: 1907a7d
series: https://patchwork.kernel.org/series/1188607/
version: 2

[kernel-patches-daemon-bpf]

Upstream branch: f25f631
series: https://patchwork.kernel.org/series/1188607/
version: 2