Re: [PATCH bpf v4 2/3] bpf: Skip detached progs in trampoline images that are still in use

[bot+bpf-ci]

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index c5f55d6161fe..bd73179c00ec 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -2431,6 +2432,10 @@ static void invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_node *node,
>  	enter_prog = (u64)bpf_trampoline_enter(p);
>  	exit_prog = (u64)bpf_trampoline_exit(p);
>
> +	/* nop, patched to skip this prog when it is detached */
> +	skip = ctx->ro_image + ctx->idx;
> +	emit(A64_NOP, ctx);
> +

Is a context synchronization event needed on the other CPUs after the skip
NOP is patched?

A subsystem pattern flags this as potentially concerning: the new per-prog
NOP becomes a B when the prog is detached. bpf_trampoline_skip_prog() ->
bpf_arch_text_poke() -> aarch64_insn_patch_text_nosync() does:

    ret = aarch64_insn_write(tp, insn);
    if (ret == 0)
        caches_clean_inval_pou((uintptr_t)tp,
                               (uintptr_t)tp + AARCH64_INSN_SIZE);

Nothing forces an isb or exception on the other CPUs. Jump label patching
of the same NOP<->B kind does this through
arch_jump_label_transform_apply() -> kick_all_cpus_sync().

The new site is a correctness point: after the detach, the trampoline
relies on this jump so that a task still in the image does not call into
the prog, which is freed after the grace periods. A CPU that has not taken
a CSE may still execute the old NOP and call the prog.

The existing comment in bpf_arch_text_poke() justifies the missing sync
only for trampoline images, by pointing at bpf_tramp_image_put(). It does
not cover the progs called from a live image.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 4da278900938..df8abfa055d1 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c

[ ... ]

> @@ -1696,13 +1696,19 @@ static void restore_stk_args(struct jit_ctx *ctx, int nr_stk_args, int args_off,
>  	}
>  }
>
> -static int invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_node *n,
> -			   int args_off, int retval_off, int run_ctx_off, bool save_ret)
> +static int invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_image *im,
> +			   struct bpf_tramp_node *n, int args_off, int retval_off,
> +			   int run_ctx_off, bool save_ret)
>  {
> -	int ret;
> +	int i, ret;
>  	u32 *branch;
>  	struct bpf_prog *p = n->link->prog;
>  	int cookie_off = offsetof(struct bpf_tramp_run_ctx, bpf_cookie);
> +	void *skip = ctx->ro_image + ctx->idx;
> +
> +	/* nops for move_imm+jirl, patched to skip this prog when it is detached */
> +	for (i = 0; i < LOONGARCH_LONG_JUMP_NINSNS; i++)
> +		emit_insn(ctx, nop);

Can a task preempted inside the skip sled resume into half of the new jump
sequence and jump to a garbage address?

The skip site is a 5-instruction nop sled. bpf_trampoline_skip_prog() turns
it into a multi-instruction sequence with bpf_arch_text_poke(skip->nop,
BPF_MOD_NOP, BPF_MOD_JUMP, NULL, skip->target). That call goes through
emit_jump_or_nops() -> move_imm(T1, target) + 'jirl zero, t1, 0' and
writes the result with:

    ret = larch_insn_text_copy(ip, new_insns, LOONGARCH_LONG_JUMP_NBYTES);

This runs on detach, while the current image and any older images still on
tr->images are live. Every call of the traced function runs through the
sled. larch_insn_text_copy() uses stop_machine_cpuslocked(), which only
parks CPUs that are running. It cannot help a task that was preempted
between two sled instructions.

The trampoline does not disable preemption before __bpf_prog_enter*(). With
CONFIG_PREEMPT, PREEMPT_LAZY (ARCH_HAS_PREEMPT_LAZY is selected in
arch/loongarch/Kconfig) or PREEMPT_RT, waking the stopper thread preempts
whatever is running on each CPU at IPI return. That includes a task that
has executed only some of the nops.

Concrete path:

  CPU0 (task A): traced func -> trampoline -> sled nop0, nop1
                 | IPI, preempted
  CPU1: bpf_link release -> bpf_trampoline_remove_prog()
        -> bpf_trampoline_skip_prog() -> bpf_arch_text_poke()
        -> larch_insn_text_copy() writes
           lu12i.w t1 / ori t1 / lu32i.d t1 / jirl zero,t1,0 / nop
  CPU0: A resumes at sled+8 and executes 'lu32i.d t1; jirl zero, t1, 0'

Bits 31:0 of t1 still hold whatever the register contained before, for
example a value left behind by the previous prog's __bpf_prog_exit call.
The indirect jump goes to an arbitrary kernel address, causing an oops or
running unintended code.

Before this change, only the prog-entry sled and ip_after_call had this
5-instruction live poke. fentry-only trampolines had no patched site inside
the image at all. Now every detach patches one sled per image on the
hottest path. The x86, arm64 and s390 skip sites are patched with a single
atomic instruction change, so they don't have this problem.

Could the LoongArch skip site use a single instruction instead? For
example, a 'b' to the skip target, which is always within the +/-128MB
range of the same image, patched from nop to 'b'. The rest of the sled
would then be unnecessary.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 9d69c066a817..ea8e67a38919 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -1246,11 +1289,8 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
>  		if (WARN_ON_ONCE(!hlist_empty(&tr->progs_hlist[i])))
>  			goto out;
>
> -	/* This code will be executed even when the last bpf_tramp_image
> -	 * is alive. All progs are detached from the trampoline and the
> -	 * trampoline image is patched with jmp into epilogue to skip
> -	 * fexit progs. The fentry-only trampoline will be freed via
> -	 * multiple rcu callbacks.
> +	/* All progs are detached and the last image has been freed, images
> +	 * hold a reference on the trampoline until then.
>  	 */

This isn't a bug, but the rewritten comment puts its text on the same line
as the opening /*:

	/* All progs are detached and the last image has been freed, images
	 * hold a reference on the trampoline until then.
	 */

The BPF subsystem's comment style asks for the opening /* of a multi-line
comment to be on its own line in kernel/bpf/, even where nearby code still
uses the old style. The patch replaces this comment's text, so the line
counts as modified. The other new multi-line comment in this file, above
bpf_trampoline_skip_prog(), already uses the preferred style. The same
comment in that style would be:

	/*
	 * All progs are detached and the last image has been freed, images
	 * hold a reference on the trampoline until then.
	 */


---
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/36122863248