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

[Florent Revest]

On Sat Sep 12, 2026 at 11:11 AM UTC,  wrote:
> > diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> > index 4da278900938..0e6f1ad36c2d 100644
> > --- a/arch/loongarch/net/bpf_jit.c
> > +++ b/arch/loongarch/net/bpf_jit.c
>
> [ ... ]
>
> > @@ -2178,7 +2189,7 @@ int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
> >  {
> >  	int ret;
> >  	struct jit_ctx ctx;
> > -	struct bpf_tramp_image im;
> > +	struct bpf_tramp_image im = {};
> >
> >  	ctx.image = NULL;
> >  	ctx.idx = 0;
>
> Does arch_bpf_trampoline_size() need to initialize ctx.ro_image?
> invoke_bpf_prog() now reads ctx->ro_image unconditionally:
>
> arch/loongarch/net/bpf_jit.c:invoke_bpf_prog()
>     void *skip = ctx->ro_image + ctx->idx;
>
> but arch_bpf_trampoline_size() only assigns ctx.image and ctx.idx. Before
> this patch the sizing path read ctx->ro_image only under
> BPF_TRAMP_F_CALL_ORIG, so fentry-only trampolines never touched it. The
> result is discarded (bpf_tramp_image_add_skip() returns immediately
> because im.skips == NULL), so there is no functional misbehaviour, but it
> is a read of an uninitialized automatic variable.
>
> The arm64 side of the same patch already uses a designated initializer:
>
> arch/arm64/net/bpf_jit_comp.c:arch_bpf_trampoline_size()
>     struct jit_ctx ctx = { .image = NULL, .idx = 0, };
>
> Making the loongarch declaration 'struct jit_ctx ctx = {};' would avoid
> the issue.

Fair, will address in v3.

> > diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> > index 151031e97a24..21f8ce2ec89a 100644
> > --- a/arch/riscv/net/bpf_jit_comp64.c
> > +++ b/arch/riscv/net/bpf_jit_comp64.c
> > @@ -904,12 +904,18 @@ static void emit_store_stack_imm64(u8 reg, int stack_off, u64 imm64,
> >  	emit_sd(RV_REG_FP, stack_off, reg, ctx);
> >  }
> >
> > -static int invoke_bpf_prog(struct bpf_tramp_node *node, int args_off, int retval_off,
> > -			   int run_ctx_off, bool save_ret, struct rv_jit_context *ctx)
> > +static int invoke_bpf_prog(struct bpf_tramp_image *im, struct bpf_tramp_node *node,
> > +			   int args_off, int retval_off, int run_ctx_off, bool save_ret,
> > +			   struct rv_jit_context *ctx)
> >  {
> >  	int ret, branch_off;
> >  	struct bpf_prog *p = node->link->prog;
> >  	int cookie_off = offsetof(struct bpf_tramp_run_ctx, bpf_cookie);
> > +	void *skip = ctx->ro_insns + ctx->ninsns;
> > +
> > +	/* 2 nops for auipc+jalr, patched to skip this prog when it is detached */
> > +	emit(rv_nop(), ctx);
> > +	emit(rv_nop(), ctx);
>
> This isn't a bug, but should BPF_MAX_TRAMP_LINKS be lowered for riscv64?
>
> The commit message states "With the extra nops, BPF_MAX_TRAMP_LINKS progs
> no longer fit in a page on arm64 and loongarch ... so lower the limit
> there like s390 does." The limits in include/linux/bpf.h were changed:
>
>   arch       added per prog   BPF_MAX_TRAMP_LINKS change
>   arm64      4 bytes            38 -> 37
>   loongarch  20 bytes           38 -> 33
>   powerpc64  4 bytes            38 -> 27
>   s390x      6 bytes            27 (already lowered)
>   riscv64    8 bytes            38 (unchanged)
>   x86_64     5 bytes            38 (unchanged)
>
> riscv64 gains 8 bytes per prog (two rv_nop()s) but its limit is
> unchanged. For 38 progs that is 304 extra bytes, which could push
> trampolines close to the PAGE_SIZE limit over the boundary.
> bpf_trampoline_update() rejects the update with -E2BIG if size exceeds
> PAGE_SIZE, so this is a graceful reduction in the maximum attachable
> progs on riscv64 rather than memory corruption.

38 progs still fit on riscv64 with the extra nops so that's ok.

> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 90b70ea0d370..b82d8592319a 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
>
> [ ... ]
>
> > @@ -565,14 +566,21 @@ static void bpf_tramp_image_free(struct bpf_tramp_image *im)
> >  	arch_free_bpf_trampoline(im->image, im->size);
> >  	bpf_jit_uncharge_modmem(im->size);
> >  	percpu_ref_exit(&im->pcref);
> > +	kfree(im->skips);
> >  	kfree_rcu(im, rcu);
> >  }
> >
> >  static void __bpf_tramp_image_put_deferred(struct work_struct *work)
> >  {
> > +	struct bpf_trampoline *tr;
> >  	struct bpf_tramp_image *im;
> >
> >  	im = container_of(work, struct bpf_tramp_image, work);
> > +	tr = im->tr;
> > +	trampoline_lock(tr);
> > +	list_del(&im->list);
> > +	trampoline_unlock(tr);
> > +	bpf_trampoline_put(tr);
> >  	bpf_tramp_image_free(im);
> >  }
>
> [ ... ]
>
> This isn't a bug, but the comment at the call site of
> __bpf_tramp_image_put_deferred() appears stale. bpf_trampoline_update()
> now does:
>
> kernel/bpf/trampoline.c:bpf_trampoline_update()
>     /* track the image until it is freed, for bpf_trampoline_skip_prog() */
>     refcount_inc(&tr->refcnt);
>     im->tr = tr;
>     list_add(&im->list, &tr->images);
>
> so every live bpf_tramp_image holds a reference on the trampoline and
> tr->refcnt cannot reach 0 while any image is alive. The comment in
> bpf_trampoline_put() says:
>
> kernel/bpf/trampoline.c:bpf_trampoline_put()
>     /* 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.
>      */
>     hlist_del(&tr->hlist_key);
>
> but the guarded block is now reached only after the last image has been
> unlinked, typically from __bpf_tramp_image_put_deferred() rather than
> from the detach syscall.

Ok, will update in v3 if we keep the list.