Re: [PATCH bpf-next 07/17] bpf: Add bpf_trampoline_multi_attach/detach functions

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Feb 20, 2026 at 10:57:54AM +0000, bot+bpf-ci@kernel.org 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