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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5465 bytes --]

> 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.

> 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.

> 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.


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