Re: [PATCHv3 bpf-next 08/24] bpf: Add bpf_trampoline_multi_attach/detach functions
Hi Jiri, kernel test robot noticed the following build errors: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Jiri-Olsa/ftrace-Add-ftrace_hash_count-function/20260316-160117 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20260316075138.465430-9-jolsa%40kernel.org patch subject: [PATCHv3 bpf-next 08/24] bpf: Add bpf_trampoline_multi_attach/detach functions config: x86_64-randconfig-075-20260320 (https://download.01.org/0day-ci/archive/20260320/[email protected]/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) rustc: rustc 1.88.0 (6b00bc388 2025-06-23) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260320/[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 errors (new ones prefixed by >>): >> kernel/bpf/trampoline.c:1520:8: error: call to undeclared function >> 'btf_distill_func_proto'; ISO C99 and later do not support implicit function >> declarations [-Wimplicit-function-declaration] 1520 | err = btf_distill_func_proto(NULL, btf, t, tname, &tgt_info->fmodel); | ^ 1 error generated. vim +/btf_distill_func_proto +1520 kernel/bpf/trampoline.c 1498 1499 static int bpf_get_btf_id_target(struct btf *btf, struct bpf_prog *prog, u32 btf_id, 1500 struct bpf_attach_target_info *tgt_info) 1501 { 1502 const struct btf_type *t; 1503 unsigned long addr; 1504 const char *tname; 1505 int err; 1506 1507 if (!btf_id || !btf) 1508 return -EINVAL; 1509 t = btf_type_by_id(btf, btf_id); 1510 if (!t) 1511 return -EINVAL; 1512 tname = btf_name_by_offset(btf, t->name_off); 1513 if (!tname) 1514 return -EINVAL; 1515 if (!btf_type_is_func(t)) 1516 return -EINVAL; 1517 t = btf_type_by_id(btf, t->type); 1518 if (!btf_type_is_func_proto(t)) 1519 return -EINVAL; > 1520 err = btf_distill_func_proto(NULL, btf, t, tname, > &tgt_info->fmodel); 1521 if (err < 0) 1522 return err; 1523 if (btf_is_module(btf)) { 1524 /* The bpf program already holds refference to module. */ 1525 if (WARN_ON_ONCE(!prog->aux->mod)) 1526 return -EINVAL; 1527 addr = find_kallsyms_symbol_value(prog->aux->mod, tname); 1528 } else { 1529 addr = kallsyms_lookup_name(tname); 1530 } 1531 if (!addr || !ftrace_location(addr)) 1532 return -ENOENT; 1533 tgt_info->tgt_addr = addr; 1534 return 0; 1535 } 1536 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCHv3 bpf-next 08/24] bpf: Add bpf_trampoline_multi_attach/detach functions
On Mon, Mar 16, 2026 at 08:35:15AM +, [email protected] wrote: > > The detach works as follows: > > - we lock all the needed trampolines > > - remove the program from each (__bpf_trampoline_unlink_prog) > > - the trampoline_multi_ops passed in __bpf_trampoline_link_prog gathers > > ftrace_hash (ip -> trampoline) objects > > The detach description references __bpf_trampoline_link_prog, but the > detach path calls __bpf_trampoline_unlink_prog. ack > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > index d55651b13511..9331cca8c0b4 100644 > > --- a/kernel/bpf/trampoline.c > > +++ b/kernel/bpf/trampoline.c > > [ ... ] > > > +static int modify_fentry_multi(struct bpf_trampoline *tr, u32 orig_flags, > > void *old_addr, > > + void *new_addr, bool lock_direct_mutex, void > > *ptr) > > +{ > > + unsigned long addr = (unsigned long) new_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->modify, ip, addr) ? 0 : > > -ENOMEM; > > +} > > Unlike the standard modify_fentry() which calls direct_ops_mod() -> > update_ftrace_direct_mod() to update ftrace immediately, > modify_fentry_multi() only adds a hash entry. The actual ftrace > update is deferred until after the link loop. > > This matters because bpf_trampoline_update() unconditionally calls > bpf_tramp_image_put() on the old cur_image after a successful > modify_fentry callback: > > bpf_trampoline_update() { > ... > err = ops->modify_fentry(tr, ...); > ... > if (tr->cur_image) > bpf_tramp_image_put(tr->cur_image); > tr->cur_image = im; > } > > With the standard ops, the old image is already unreachable when put. > With multi ops, ftrace still points to the old image when it is put. > bpf_tramp_image_put() text-pokes the old image to skip fexit programs > and starts deferred freeing. > > In the success path, the window between image_put and the batched > update_ftrace_direct_mod is brief (bounded by trampoline_lock_all). > > However, in the rollback path below, ftrace is never updated at all: > > [ ... ] > > > + 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(); > > + > > + i = cnt; > > If a trampoline at index 0 already had programs attached through the > standard single-trampoline path, and __bpf_trampoline_link_prog > succeeds for it but fails for a later trampoline, the rollback path > restores tr->cur_image to a correct new image but never calls > update_ftrace_direct_mod. > > At that point, ftrace still routes calls through the old image that > was put during the link phase. bpf_tramp_image_put() has already > patched that old image to skip fexit programs and started the deferred > freeing sequence (percpu_ref_kill). > > Could this leave existing fexit/fmod_ret programs on that trampoline > non-functional? And once the percpu_ref drains during a quiet period, > would the old image be freed while ftrace still references it? good catch, we need the update_ftrace_direct_mod call after the unlink, I changed the rollback test to hit the described issue, will fix > > [ ... ] > > > +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; > > + } > > bpf_trampoline_multi_detach can return -ENOMEM here without detaching > anything. In e80a839e3d2c ("bpf: Add support for tracing multi > link"), this is called from bpf_tracing_multi_link_release(): > > WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link)); > > If detach fails, the programs remain linked to the trampolines via > mnode->node.tramp_hlist, but the link (and its embedded nodes array) > is freed by bpf_tracing_multi_link_dealloc(). Would this leave > dangling hlist entries in each trampol
Re: [PATCHv3 bpf-next 08/24] bpf: Add bpf_trampoline_multi_attach/detach functions
> The detach works as follows:
> - we lock all the needed trampolines
> - remove the program from each (__bpf_trampoline_unlink_prog)
> - the trampoline_multi_ops passed in __bpf_trampoline_link_prog gathers
> ftrace_hash (ip -> trampoline) objects
The detach description references __bpf_trampoline_link_prog, but the
detach path calls __bpf_trampoline_unlink_prog.
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index d55651b13511..9331cca8c0b4 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
[ ... ]
> +static int modify_fentry_multi(struct bpf_trampoline *tr, u32 orig_flags,
> void *old_addr,
> +void *new_addr, bool lock_direct_mutex, void
> *ptr)
> +{
> + unsigned long addr = (unsigned long) new_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->modify, ip, addr) ? 0 :
> -ENOMEM;
> +}
Unlike the standard modify_fentry() which calls direct_ops_mod() ->
update_ftrace_direct_mod() to update ftrace immediately,
modify_fentry_multi() only adds a hash entry. The actual ftrace
update is deferred until after the link loop.
This matters because bpf_trampoline_update() unconditionally calls
bpf_tramp_image_put() on the old cur_image after a successful
modify_fentry callback:
bpf_trampoline_update() {
...
err = ops->modify_fentry(tr, ...);
...
if (tr->cur_image)
bpf_tramp_image_put(tr->cur_image);
tr->cur_image = im;
}
With the standard ops, the old image is already unreachable when put.
With multi ops, ftrace still points to the old image when it is put.
bpf_tramp_image_put() text-pokes the old image to skip fexit programs
and starts deferred freeing.
In the success path, the window between image_put and the batched
update_ftrace_direct_mod is brief (bounded by trampoline_lock_all).
However, in the rollback path below, ftrace is never updated at all:
[ ... ]
> + 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();
> +
> + i = cnt;
If a trampoline at index 0 already had programs attached through the
standard single-trampoline path, and __bpf_trampoline_link_prog
succeeds for it but fails for a later trampoline, the rollback path
restores tr->cur_image to a correct new image but never calls
update_ftrace_direct_mod.
At that point, ftrace still routes calls through the old image that
was put during the link phase. bpf_tramp_image_put() has already
patched that old image to skip fexit programs and started the deferred
freeing sequence (percpu_ref_kill).
Could this leave existing fexit/fmod_ret programs on that trampoline
non-functional? And once the percpu_ref drains during a quiet period,
would the old image be freed while ftrace still references it?
[ ... ]
> +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;
> + }
bpf_trampoline_multi_detach can return -ENOMEM here without detaching
anything. In e80a839e3d2c ("bpf: Add support for tracing multi
link"), this is called from bpf_tracing_multi_link_release():
WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
If detach fails, the programs remain linked to the trampolines via
mnode->node.tramp_hlist, but the link (and its embedded nodes array)
is freed by bpf_tracing_multi_link_dealloc(). Would this leave
dangling hlist entries in each trampoline's progs_hlist?
---
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/23133791558
