> bpf: Add bpf_trampoline_multi_prog_attach/detach()
TL;DR: bpf_trampoline_multi_prog_detach() may, if one unlink fails (e.g.
-ENOMEM), still commit and free old images after a failed batch poke, so other
targets could run freed trampoline memory.
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
[ ... ]
> @@ -1918,6 +1923,217 @@ static int bpf_text_poke_batch(struct bpf_text_poke
> **pokes, u32 cnt)
> +static void bpf_trampoline_multi_prog_commit(struct bpf_trampoline *tr)
> +{
> + struct bpf_tramp_image *old_image = tr->multi_attach.old_image;
> +
> + tr->cur_image = tr->multi_attach.new_image;
> + tr->multi_attach.old_image = NULL;
> + tr->multi_attach.new_image = NULL;
> + tr->multi_attach.old_flags = 0;
> +
> + if (old_image)
> + bpf_tramp_image_put(old_image);
> +}
> +
> +static void bpf_trampoline_multi_prog_rollback(struct bpf_trampoline *tr)
> +{
> + if (tr->multi_attach.new_image)
> + bpf_tramp_image_put(tr->multi_attach.new_image);
> +
> + tr->flags = tr->multi_attach.old_flags;
> + tr->multi_attach.old_image = NULL;
> + tr->multi_attach.new_image = NULL;
> + tr->multi_attach.old_flags = 0;
> +}
[ ... ]
> +void bpf_trampoline_multi_prog_detach(struct bpf_tracing_multi_link *link)
> +{
> + struct bpf_tracing_multi_node *mnode;
> + int i, err;
> +
> + trampoline_lock_all();
> +
> + for_each_mnode(mnode, link) {
> + bpf_trampoline_multi_attach_init(mnode->trampoline);
> + err = __bpf_trampoline_unlink_prog(&mnode->node,
> mnode->trampoline, NULL,
> + &trampoline_multi_prog_ops,
> NULL);
> + if (WARN_ONCE(err, "__bpf_trampoline_unlink_prog failed: %d\n",
> err))
> + bpf_trampoline_multi_prog_rollback(mnode->trampoline);
> + }
If __bpf_trampoline_unlink_prog() fails for one trampoline here, can this
leave other targets calling freed trampoline images?
The failure is reachable. bpf_trampoline_update() returns an error when
bpf_trampoline_get_progs() or bpf_tramp_image_alloc() fails (-ENOMEM), or
when bpf_jit_charge_modmem() returns -EPERM because bpf_jit_limit is
exceeded and the task closing the link fd is not bpf_capable(). The comment
in bpf_trampoline_multi_attach_free() already treats this failure as
reachable on the ftrace detach path.
Say the unlink fails for trampoline B:
1. bpf_trampoline_multi_attach_init() has set old_image = cur_image, which
is still live in the text, and new_image = NULL.
2. bpf_trampoline_multi_prog_rollback() then clears old_image to NULL as
well.
3. bpf_text_poke_init() therefore builds a poke for B with old_addr and
new_addr both NULL (BPF_MOD_NOP to BPF_MOD_NOP), while B's entry still
holds a call or jump to its live image.
4. On x86, bpf_arch_text_poke_batch() checks every poke before patching
anything:
if (memcmp(poke->ip, old_insn, X86_PATCH_SIZE))
return -EBUSY;
B's check fails, so no text is patched for any target in the link. The
generic fallback rolls back the pokes it already applied, with the same
result.
> +
> + for_each_mnode(mnode, link) {
> + link->pokes[i] = &mnode->poke;
> + bpf_text_poke_init(link->pokes[i], mnode->trampoline);
> + }
> +
> + WARN_ON_ONCE(bpf_text_poke_batch(link->pokes, link->nodes_cnt));
> +
> + for_each_mnode(mnode, link)
> + bpf_trampoline_multi_prog_commit(mnode->trampoline);
The batch error only triggers WARN_ON_ONCE(), and
bpf_trampoline_multi_prog_commit() still runs for every node.
For every other target A in the link, old_A is passed to
bpf_tramp_image_put() and freed after a tasks-rcu grace period. A's entry
still calls old_A, because it was never repointed, so the next run of target
A executes freed trampoline memory. Meanwhile new_A becomes cur_image
without ever having been installed.
For B itself, commit sets cur_image to NULL while the text still calls the
old image. That image is never put, so it leaks along with its trampoline
reference. If another prog (for example a plain fentry) is still attached
to B and is later detached, bpf_trampoline_update() reaches total == 0 and
calls unregister_fentry(), which dereferences tr->cur_image->image while
cur_image is NULL. Any new attach to B instead goes through
register_fentry(), expects a nop at the entry, and fails with -EBUSY.
The ftrace-based bpf_trampoline_multi_detach() avoids this. On unlink
failure it leaves cur_image unchanged, and bpf_trampoline_multi_attach_free()
skips freeing old_image when old_image == cur_image. Could the prog detach
path get the same treatment?
For example, on unlink failure the trampoline could be kept on its current
image (new_image = old_image, so the poke is an identity poke and commit
neither puts nor drops cur_image), or the node could be left out of the
batch. It also seems that images should not be committed when
bpf_text_poke_batch() fails.
This path is not called by this commit. It is wired up later in the series
by b56b0344ff12 ("bpf: Add tracing_multi link support for bpf progs"), where
bpf_tracing_multi_link_release() calls bpf_trampoline_multi_prog_detach().
The function is unchanged at the end of the series, so no later commit
fixes 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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38273555988
