Re: [PATCHv4 bpf-next 09/25] bpf: Add bpf_trampoline_multi_attach/detach functions

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Mar 24, 2026 at 08:58:41AM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index a9e328c0a1b3..2986e5cac743 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> 
> [ ... ]
> 
> > +int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_link *link)
> > +{
> > +	struct bpf_tracing_multi_node *mnode;
> > +	struct fentry_multi_data data = {};
> > +	int i, cnt = link->nodes_cnt;
> > +
> > +	data.unreg  = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > +	data.modify = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > +
> > +	if (!data.unreg || !data.modify) {
> > +		free_fentry_multi_data(&data);
> > +		return -ENOMEM;
> > +	}
> 
> If alloc_ftrace_hash() fails here under memory pressure, the function
> returns -ENOMEM without detaching any trampolines or releasing their
> refcounts.
> 
> The caller in the next patch (bpf_tracing_multi_link_release) does:
> 
>     WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
> 
> This drops the error on the floor. The link struct is then freed by
> bpf_tracing_multi_link_dealloc(), but the trampolines remain attached
> with elevated refcounts, permanently leaking them and their ftrace
> hooks.
> 
> Would it make sense to pre-allocate these hashes during
> bpf_trampoline_multi_attach() and store them in the link struct so
> that the detach path cannot fail?

it's not just hash objects but we will need allocate also entries,
I can do follow up with this if others think it's needed

> 
> > +
> > +	trampoline_lock_all();
> 
> [ ... ]
> 
> > +rollback_unlink:
> > +	/*
> > +	 * We can end up in here from 3 points from above code:
> > +	 *
> > +	 * - __bpf_trampoline_link_prog or update_ftrace_direct_add failed and
> > +	 *   we have some portion of linked trampolines without ftrace update
> > +	 *
> > +	 * - update_ftrace_direct_mod failed and we have all trampolines linked
> > +	 *   plus we already un-attached all new trampolines
> > +	 *
> > +	 * In both cases we need to unlink all trampolines from the new program
> > +	 * and update modified (data.modify) sites, because those have previously
> > +	 * some programs attached and the new trampoline needs to get attached.
> > +	 */
> 
> This isn't a bug, but the comment says "3 points" and then describes
> two scenarios in two bullet points. Maybe say "2 scenarios" or split
> the first bullet into two to match?

first two points are covered in the first bullets, I think it's ok

jirka

> 
> > +	ftrace_hash_clear(data.modify);
> 
> [ ... ]
> 
> 
> ---
> 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/23480161822