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

On Fri, Feb 20, 2026 at 10:57:54AM +0000, bot+bpf-ci@kernel.org wrote:

quoted

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

[ ... ]

quoted

+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;
+}

[ ... ]

quoted

+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

quoted

+
+	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.

quoted

+
+	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;
+	}

[ ... ]

quoted

+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