Re: [PATCHv4 bpf-next 09/25] bpf: Add bpf_trampoline_multi_attach/detach functions
Hi Jiri, kernel test robot noticed the following build warnings: [auto build test WARNING on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Jiri-Olsa/ftrace-Add-ftrace_hash_count-function/20260326-101836 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20260324081846.2334094-10-jolsa%40kernel.org patch subject: [PATCHv4 bpf-next 09/25] bpf: Add bpf_trampoline_multi_attach/detach functions config: x86_64-randconfig-015-20260327 (https://download.01.org/0day-ci/archive/20260327/[email protected]/config) compiler: gcc-14 (Debian 14.2.0-19) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260327/[email protected]/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ All warnings (new ones prefixed by >>): >> kernel/bpf/trampoline.c:100:13: warning: 'trampoline_unlock_all' defined but >> not used [-Wunused-function] 100 | static void trampoline_unlock_all(void) | ^ >> kernel/bpf/trampoline.c:92:13: warning: 'trampoline_lock_all' defined but >> not used [-Wunused-function] 92 | static void trampoline_lock_all(void) | ^~~ vim +/trampoline_unlock_all +100 kernel/bpf/trampoline.c 91 > 92 static void trampoline_lock_all(void) 93 { 94 int i; 95 96 for (i = 0; i < TRAMPOLINE_LOCKS_TABLE_SIZE; i++) 97 mutex_lock(&trampoline_locks[i].mutex); 98 } 99 > 100 static void trampoline_unlock_all(void) 101 { 102 int i; 103 104 for (i = 0; i < TRAMPOLINE_LOCKS_TABLE_SIZE; i++) 105 mutex_unlock(&trampoline_locks[i].mutex); 106 } 107 #else 108 static struct bpf_trampoline *direct_ops_ip_lookup(struct ftrace_ops *ops, unsigned long ip) 109 { 110 return ops->private; 111 } 112 #endif /* CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS */ 113 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCHv4 bpf-next 09/25] bpf: Add bpf_trampoline_multi_attach/detach functions
On Tue, Mar 24, 2026 at 08:58:41AM +, [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
Re: [PATCHv4 bpf-next 09/25] bpf: Add bpf_trampoline_multi_attach/detach functions
> 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?
> +
> + 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?
> + 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
