On Sat, Apr 18, 2026 at 06:10:57AM +0000, [email protected] 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
