On Fri, Feb 20, 2026 at 10:57:54AM +0000, [email protected] wrote:
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 2d701bc6e1a5..c32205adfebe 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
>
> [ ... ]
>
> > +static int unregister_fentry_multi(struct bpf_trampoline *tr, u32
> > orig_flags, void *old_addr,
> > + void *ptr)
> > +{
> > + unsigned long addr = (unsigned long) old_addr;
> > + unsigned long ip = ftrace_location(tr->ip);
> > + struct fentry_multi_data *data = ptr;
> > +
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > + return add_ftrace_hash_entry_direct(data->unreg, ip, addr) ? 0 :
> > -ENOMEM;
> > +}
>
> [ ... ]
>
> > +int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
> > + struct bpf_tracing_multi_link *link)
> > +{
> > + struct bpf_attach_target_info tgt_info = {};
> > + struct bpf_tracing_multi_node *mnode;
> > + int j, i, err, cnt = link->nodes_cnt;
> > + struct fentry_multi_data data = {};
> > + struct bpf_trampoline *tr;
> > + u64 key;
> > +
> > + data.reg = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > + if (!data.reg)
> > + return -ENOMEM;
> > +
> > + data.modify = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > + if (!data.modify) {
> > + free_ftrace_hash(data.reg);
> > + return -ENOMEM;
> > + }
>
> data.unreg is left as NULL here. In the forward path this is
> fine since only data.reg and data.modify are used, but the
> rollback path can reach unregister_fentry_multi() which calls
> add_ftrace_hash_entry_direct(data->unreg, ...) with a NULL hash.
>
> This happens when rollback_unlink calls
> __bpf_trampoline_unlink_prog() on a trampoline that had no
> other BPF programs before this attach. Removing the only
> program brings total to 0, so bpf_trampoline_update_ops()
> calls ops->unregister_fentry, reaching:
>
> unregister_fentry_multi()
> add_ftrace_hash_entry_direct(data->unreg, ip, addr)
> __add_hash_entry(hash, entry)
> ftrace_hash_key(hash, ...) <-- NULL dereference
>
> Should data.unreg also be allocated here to cover the rollback
> path?
I knew I should have added rollback tests ;-) right, we need the unreg
hash as well.. it's on my todo, will add fix and add tests next version
>
> > +
> > + for (i = 0; i < cnt; i++) {
> > + mnode = &link->nodes[i];
> > + err = bpf_check_attach_target(NULL, prog, NULL, ids[i],
> > &tgt_info);
> > + if (err)
> > + goto rollback_put;
> > +
> > + key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf,
> > ids[i]);
> > +
> > + tr = bpf_trampoline_get(key, &tgt_info);
> > + if (!tr) {
> > + err = -ENOMEM;
> > + goto rollback_put;
> > + }
> > +
> > + mnode->trampoline = tr;
> > + mnode->node.link = &link->link;
> > + }
>
> After this loop completes, all cnt trampolines have been
> obtained via bpf_trampoline_get() with refcounts incremented.
>
> > +
> > + trampoline_lock_all();
> > +
> > + for (i = 0; i < cnt; i++) {
> > + mnode = &link->nodes[i];
> > + err = __bpf_trampoline_link_prog(&mnode->node,
> > mnode->trampoline, NULL,
> > + &trampoline_multi_ops, &data);
> > + if (err)
> > + goto rollback_unlink;
> > + }
>
> [ ... ]
>
> > +rollback_unlink:
> > + for (j = 0; j < i; j++) {
> > + mnode = &link->nodes[j];
> > + WARN_ON_ONCE(__bpf_trampoline_unlink_prog(&mnode->node,
> > mnode->trampoline,
> > + NULL, &trampoline_multi_ops, &data));
> > + }
> > + trampoline_unlock_all();
> > +
> > +rollback_put:
> > + for (j = 0; j < i; j++)
> > + bpf_trampoline_put(link->nodes[j].trampoline);
>
> When __bpf_trampoline_link_prog() fails at index i in the
> second loop, this falls through from rollback_unlink to
> rollback_put with i still set to the failing index. But all
> cnt trampolines were obtained in the first loop above.
>
> The rollback_put loop only puts trampolines 0..i-1, leaving
> trampolines i..cnt-1 with elevated refcounts that are never
> released.
>
> When rollback_put is reached directly from the first loop
> (bpf_trampoline_get or bpf_check_attach_target failure), the
> j < i bound is correct since only 0..i-1 were obtained.
>
> Could the rollback_unlink path set i = cnt before falling
> through, or use a separate loop bound for the put?
good catch, I think that's a good fix, thnx
jirka
