Re: [PATCHv5 bpf-next 12/28] bpf: Add bpf_trampoline_multi_attach/detach functions

[Jiri Olsa <olsajiri@gmail.com>]

On Sat, Apr 18, 2026 at 06:10:57AM +0000, bot+bpf-ci@kernel.org wrote:

SNIP

> > +		err = __bpf_trampoline_link_prog(&mnode->node, mnode->trampoline, NULL,
> > +						 &trampoline_multi_ops, data);
> > +		if (err) {
> > +			rollback_cnt = i;
> > +			goto rollback_unlink;
> > +		}
> > +	}
> 
> When user-provided ids[] contains duplicate BTF IDs (or distinct IDs
> that resolve to the same trampoline key), multiple nodes point to the
> same struct bpf_trampoline. The link loop above then calls
> bpf_trampoline_multi_attach_init() more than once on that trampoline,
> overwriting the saved old_image with the newly assigned cur_image from
> the previous iteration.
> 
> Scenario with ids[0] == ids[1] and trampoline X starting with OLD_X:
> 
>   i=0: attach_init(X) saves old_image=OLD_X
>        __bpf_trampoline_link_prog() -> modify_fentry_multi() sets
>        X->cur_image=NEW_X (OLD_X refcount not dropped, intent is for
>        multi_attach_free() to release it later).
> 
>   i=1: attach_init(X) re-runs on the same trampoline and overwrites
>        old_image=NEW_X (the only saved reference to OLD_X is lost).
>        __bpf_trampoline_link_prog() returns -EBUSY (duplicate prog).
>        rollback_cnt=i=1; goto rollback_unlink.
> 
>   rollback_unlink calls bpf_trampoline_multi_attach_rollback(X) once:
> 
>     static void bpf_trampoline_multi_attach_rollback(struct bpf_trampoline *tr)
>     {
>         if (tr->cur_image)
>             bpf_tramp_image_put(tr->cur_image);    /* puts NEW_X */
>         tr->cur_image = tr->multi_attach.old_image;  /* = NEW_X (stale!) */
>         ...
>     }
> 
> Result: OLD_X leaks (its refcount is never decremented). cur_image
> points at NEW_X which was just released via bpf_tramp_image_put().
> 
> The rollback_put loop calls bpf_trampoline_put() which frees the
> trampoline (prog was removed, progs_hlist is empty), so the dangling
> NEW_X pointer is cleaned up. However OLD_X remains allocated via
> module_alloc forever.
> 
> Reachability: commit dbf2afe2f603 ("bpf: Add support for tracing multi
> link") copies ids[] from userspace without deduplication. The series'
> selftests (commit 411fb40d4b2a "selftests/bpf: Add tracing multi attach
> fails test", 'fail#7 (kernel) attach with duplicate id') exercise this
> path explicitly, expecting -EBUSY.
> 
> The trigger requires the shared trampoline to already have cur_image !=
> NULL when the first iteration runs (i.e., another program attached to
> the same function), so modify_fentry_multi() is used and there is a
> real OLD_X to leak. The first-time-attach case (register_fentry_multi,
> OLD_X=NULL) only leaves cur_image dangling briefly until the trampoline
> is freed.
> 
> Can the code save old_image/old_flags once per trampoline instead of
> once per node, or store them per-mnode to avoid overwriting the saved
> state?

I think it's better to ensure the btf ids array does not contain
duplicate entries.. wil check

> 
> > +
> > +	rollback_cnt = link->nodes_cnt;
> > +	if (ftrace_hash_count(data->reg)) {
> > +		err = update_ftrace_direct_add(&direct_ops, data->reg);
> > +		if (err)
> > +			goto rollback_unlink;
> > +	}
> > +
> > +	if (ftrace_hash_count(data->modify)) {
> > +		err = update_ftrace_direct_mod(&direct_ops, data->modify, true);
> > +		if (err) {
> > +			WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->reg));
> > +			                                                       ^^^^^^^^^^
> > +			goto rollback_unlink;
> > +		}
> > +	}
> 
> The recovery path unconditionally calls update_ftrace_direct_del with
> data->reg, but update_ftrace_direct_add is only invoked when data->reg
> is non-empty:
> 
>     if (ftrace_hash_count(data->reg)) {
>         err = update_ftrace_direct_add(&direct_ops, data->reg);
>         ...
>     }
> 
> When update_ftrace_direct_mod fails and all trampolines already had
> cur_image != NULL (so nothing was added to data->reg),
> update_ftrace_direct_del is called with an empty hash.
> update_ftrace_direct_del at kernel/trace/ftrace.c:6511 checks:
> 
>     if (!hash_count(hash))
>         return -EINVAL;
> 
> so it returns -EINVAL and the WARN_ON_ONCE fires spuriously. Should
> this guard match the add path?
> 
>     if (ftrace_hash_count(data->reg))
>         WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->reg));

yes, replied in the other email

SNIP

> > +int bpf_check_attach_btf_id_multi(struct btf *btf, struct bpf_prog *prog, u32 btf_id,
> > +				  struct bpf_attach_target_info *tgt_info)
> > +{
> > +	const struct btf_type *t;
> > +	unsigned long addr;
> > +	const char *tname;
> > +	int err;
> > +
> > +	if (!btf_id || !btf)
> > +		return -EINVAL;
> > +
> > +	/* Check noreturn attachment. */
> > +	if (prog->expected_attach_type == BPF_TRACE_FEXIT_MULTI ||
> > +	     btf_id_set_contains(&noreturn_deny, btf_id))
> > +		return -EINVAL;
> 
> Operator precedence issue in the noreturn check. The two subexpressions
> are joined by ||, so the function returns -EINVAL whenever EITHER:
> 
>   (a) the program's expected_attach_type is BPF_TRACE_FEXIT_MULTI
>       (regardless of whether btf_id targets a noreturn function), OR
> 
>   (b) the btf_id is in the noreturn_deny set (regardless of program
>       attach type, rejecting FENTRY_MULTI attachments to noreturn
>       functions as well).
> 
> The intent is to reject only FEXIT-like multi attachments to noreturn
> functions. The non-multi path in check_attach_btf_id() correctly uses
> &&:
> 
>     } else if ((prog->expected_attach_type == BPF_TRACE_FEXIT ||
>                prog->expected_attach_type == BPF_TRACE_FSESSION ||
>                prog->expected_attach_type == BPF_MODIFY_RETURN) &&
>                btf_id_set_contains(&noreturn_deny, btf_id)) {
>         ...
>         return -EINVAL;
>     }
> 
> Consequences once bpf_trampoline_multi_attach() is wired up from
> user-space (commit dbf2afe2f603 "bpf: Add support for tracing multi
> link"):
> 
>   1. Every BPF_TRACE_FEXIT_MULTI program attach request is rejected
>      with -EINVAL, even when the target function returns normally. This
>      disables BPF_TRACE_FEXIT_MULTI entirely.
> 
>   2. Any tracing_multi program (including BPF_TRACE_FENTRY_MULTI) is
>      rejected when its btf_id is in noreturn_deny (do_exit,
>      __x64_sys_exit, make_task_dead, etc.). Rejecting FENTRY_MULTI on
>      noreturn functions is an unintended behavioral restriction.
> 
> Forward search in the git range shows commit bfcad202ce99 ("bpf: Add
> support for tracing_multi link session") rewrites this hunk to:
> 
>     if ((prog->expected_attach_type == BPF_TRACE_FEXIT_MULTI ||
>          prog->expected_attach_type == BPF_TRACE_FSESSION_MULTI) &&
>          btf_id_set_contains(&noreturn_deny, btf_id))
>         return -EINVAL;
> 
> The parenthesization + && in the later commit confirms this is a
> precedence bug. Should this use && instead?

yes, replied in the other email

jirka