On Tue, Mar 24, 2026 at 08:58:41AM +0000, [email protected] 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
