Re: [PATCH bpf-next 2/5] bpf, x86: Replace 'if r3 != 0 goto pad' after a call with an extable entry

[sashiko-bot@kernel.org]

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

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

bpf, x86: Replace 'if r3 != 0 goto pad' after a call with an extable entry

This commit optimizes unwinding subprogram calls on x86-64 by replacing the
post-call 'if r3 != 0 goto pad' branch with an exception table entry and
redirecting the caller's return address at subprogram exit when unwinding.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index b16d74d2681a9..14696555e8a51 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -3331,6 +3331,50 @@ bool __weak bpf_jit_supports_unwind(void)
>  	return false;
>  }
>  
> +/*
> + * A subprog that can unwind says so in r3 and its caller does
> + * 'if r3 != 0 goto pad' after the call. A JIT can keep that off the normal path.
> + * It doesn't emit the branch and adds an extable entry instead:
> + *   insn  = return address of the call - 1
> + *   fixup = pad - return address of the call
> + * - 1, because what follows the call can have an entry of its own.
> + * An exit with r3 != 0 looks its own return address up and returns to the pad.
> + * When there is no entry it returns as usual and the branch is there.
> + *
> + * Is insn i of the prog such a call?
> + */
> +bool bpf_jit_unwind_site(const struct bpf_verifier_env *env, const struct bpf_prog *prog, int i)
> +{
> +	const struct bpf_insn *insn = &prog->insnsi[i];
> +
> +	if (!env || i + 1 >= prog->len)
> +		return false;
> +	if (!bpf_pseudo_call(insn) && !bpf_is_callx(insn))
> +		return false;
> +	/*
> +	 * No need to know the callee: the verifier lets r3 be read after
> +	 * a call only when every callee seen there says something in it.
> +	 */
> +	return insn[1].code == (BPF_JMP | BPF_JNE | BPF_K) && !insn[1].imm &&
> +	       insn[1].dst_reg == BPF_REG_UNWIND &&
> +	       !bpf_is_jump_target(env, prog->aux->subprog_start + i + 1);
> +}
> +
> +/* What does 'exit' at insn i of the prog do? */
> +enum bpf_unwind_exit bpf_jit_unwind_exit(const struct bpf_verifier_env *env,
> +					 const struct bpf_prog *prog, int i)
> +{
> +	const struct bpf_insn *insn = &prog->insnsi[i];
> +
> +	if (!env || !prog->aux->ret_unwind)
> +		return BPF_EXIT_RETURNS;
> +	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;
> +}

[Severity: Critical]
Is env->insn_aux_data[].jump_target still accurate when bpf_jit_unwind_site()
and bpf_jit_unwind_exit() query bpf_is_jump_target() during JIT compilation?

env->insn_aux_data[].jump_target is populated once during bpf_check_cfg()
before main verification, and several post-verification passes in
kernel/bpf/fixups.c and kernel/bpf/const_fold.c modify instructions without
preserving jump_target on the resulting jump targets:

1. NOP removal in bpf_opt_remove_nops() -> verifier_remove_insns():
   Dead conditional branches folded to 'ja +0' (by bpf_prune_dead_branches()
   or bpf_opt_hard_wire_dead_code_branches()) and removed fastcall
   spills/fills (by bpf_remove_fastcall_spills_fills()) are deleted unless
   aux[i].indirect_target is set:

   kernel/bpf/fixups.c:bpf_opt_remove_nops() {
       ...
       if (aux[i].indirect_target)
           continue;

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

   verifier_remove_insns() redirects incoming jumps from off to the next
   instruction, but overwrites aux_data[off] with aux_data[off + cnt]:

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

   If a jump targeted the removed NOP at k (aux_data[k].jump_target == true)
   while k + 1 was only reached by fallthrough (aux_data[k + 1].jump_target ==
   false), the instruction shifting into k inherits the incoming jump with
   jump_target == false.

   In an unwinding subprogram such as:

       r3 = 1
       if r1 == 0 goto L
       r3 = 0
   L:  if r2 == 0 goto dead   /* folded to ja +0 and removed */
       exit

   removing the NOP at L leaves exit at L with jump_target == false and
   insn[-1] == 'r3 = 0'. bpf_jit_unwind_exit() then returns BPF_EXIT_RETURNS
   instead of BPF_EXIT_LOOK_AT_R3, so do_jit() omits emit_unwind_ret_addr().
   When the 'r1 == 0' branch is taken with r3 = 1, the subprogram returns to
   the caller's normal post-call instruction (where do_jit() set
   skip_branch = true and stripped 'if r3 != 0 goto pad') instead of the
   landing pad, bypassing cleanup on the unwind path. Conversely, if
   insn[-1] is 'r3 = 1', bpf_jit_unwind_exit() returns BPF_EXIT_UNWINDS and
   unwinds unconditionally even when a jumping path had r3 = 0.

   Similarly, if a jump-targeted NOP between a call and 'if r3 != 0 goto pad'
   is removed, 'if r3 != 0 goto pad' shifts into insn[1] after the call with
   jump_target == false, causing bpf_jit_unwind_site() to return true and
   do_jit() to elide the branch via skip_branch = true even though another
   jump targets it.

2. PROBE_MEM fixup in bpf_do_misc_fixups():
   When bpf_arch_uaddress_limit() is non-zero (such as on x86-64), a
   BPF_LDX | BPF_PROBE_MEM (or BPF_PROBE_MEMSX) load into r3 immediately
   before exit is expanded via bpf_patch_insn_data():

   kernel/bpf/fixups.c:bpf_do_misc_fixups() {
       ...
       *patch++ = BPF_JMP_IMM(BPF_JLE, BPF_REG_AX, uaddress_limit >> 32, 2);
       *patch++ = *insn;
       *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
       *patch++ = BPF_MOV64_IMM(insn->dst_reg, 0);

       cnt = patch - insn_buf;
       new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
   }

   The synthesized 'goto +1' jumps over 'r3 = 0' to exit without marking
   exit as a jump target, and places 'r3 = 0' at insn[-1].
   bpf_jit_unwind_exit() then sees jump_target == false and insn[-1] ==
   'r3 = 0', returning BPF_EXIT_RETURNS instead of BPF_EXIT_LOOK_AT_R3.

3. Instruction patching in adjust_insn_aux_data():
   When expanding an instruction in BPF_PATCH_KEEP_TARGET mode,
   indirect_target is moved from data[off + cnt - 1] back to data[off],
   whereas jump_target is left on data[off + cnt - 1] and cleared on
   data[off]:

   kernel/bpf/fixups.c:adjust_insn_aux_data() {
       ...
       if (mode == BPF_PATCH_KEEP_TARGET && data[off + cnt - 1].indirect_target) {
           data[off].indirect_target = 1;
           data[off + cnt - 1].indirect_target = 0;
       }
   }

Should jump_target be maintained across verifier_remove_insns(),
adjust_insn_aux_data(), and fixup patches (or recomputed after all instruction
rewrites before JIT compilation)?

>  /*
>   * How this arch places a by-value kfunc argument, or NULL for one that has
>   * not opted in and so only takes an argument of a single eightbyte, which

[ ... ]

>
endasi

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