On Fri, Apr 17, 2026 at 08:22:36PM +0000, [email protected] wrote:
SNIP
> > + 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;
> > + }
> > + }
>
> Does update_ftrace_direct_del need a guard here?
>
> When update_ftrace_direct_mod fails, the recovery code unconditionally
> calls update_ftrace_direct_del(&direct_ops, data->reg). However,
> update_ftrace_direct_add is itself only invoked when data->reg is
> non-empty:
>
> if (ftrace_hash_count(data->reg)) {
> err = update_ftrace_direct_add(&direct_ops, data->reg);
> ...
> }
>
> If all trampolines being multi-attached already had cur_image != NULL,
> nothing is ever added to data->reg, and the add call is skipped. Yet
> the recovery path still calls update_ftrace_direct_del with the empty
> data->reg.
>
> update_ftrace_direct_del() at kernel/trace/ftrace.c:6511 does:
>
> if (!hash_count(hash))
> return -EINVAL;
>
> so it returns -EINVAL, and WARN_ON_ONCE fires. No further damage is
> done since the rollback_unlink path still unwinds correctly, but this
> is a spurious warning that becomes visible whenever
> update_ftrace_direct_mod fails and data->reg happens to be empty.
>
> The undo should match the guard used for the add:
>
> if (err) {
> if (ftrace_hash_count(data->reg))
> WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->reg));
> goto rollback_unlink;
> }
right, that's the fix
>
> > +
> > + for_each_mnode(mnode, link)
> > + bpf_trampoline_multi_attach_free(mnode->trampoline);
> > +
> > + trampoline_unlock_all();
> > +
> > + remove_tracing_multi_data(data);
> > + return 0;
> > +
> > +rollback_unlink:
> > + for_each_mnode_cnt(mnode, link, rollback_cnt) {
> > + bpf_trampoline_remove_prog(mnode->trampoline, &mnode->node);
> > + bpf_trampoline_multi_attach_rollback(mnode->trampoline);
> > + }
> > +
> > + trampoline_unlock_all();
> > +
> > + clear_tracing_multi_data(data);
> > + rollback_cnt = link->nodes_cnt;
> > +
> > +rollback_put:
> > + for_each_mnode_cnt(mnode, link, rollback_cnt)
> > + bpf_trampoline_put(mnode->trampoline);
> > +
> > + return err;
> > +}
SNIP
> > + /* Check noreturn attachment. */
> > + if (prog->expected_attach_type == BPF_TRACE_FEXIT_MULTI ||
> > + btf_id_set_contains(&noreturn_deny, btf_id))
> > + return -EINVAL;
>
> Should this be && instead of ||?
>
> This condition rejects all BPF_TRACE_FEXIT_MULTI programs regardless
> of whether the target is on the noreturn_deny list. The C operator
> precedence makes this `A || B`, so if the program's expected_attach_type
> is BPF_TRACE_FEXIT_MULTI, the entire condition is TRUE and the
> function returns -EINVAL.
>
> This breaks FEXIT_MULTI attachment support. The caller
> bpf_trampoline_multi_attach is the multi-attach path that supports both
> BPF_TRACE_FENTRY_MULTI and BPF_TRACE_FEXIT_MULTI (see is_tracing_multi()
> in include/linux/bpf.h).
>
> The equivalent check in the single-target path, 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)) {
> ...
> }
>
> The intended semantics are to reject FEXIT_MULTI attachments only when
> the target is on noreturn_deny. The correct operator is &&:
>
> if (prog->expected_attach_type == BPF_TRACE_FEXIT_MULTI &&
> btf_id_set_contains(&noreturn_deny, btf_id))
> return -EINVAL;
>
> Additional evidence:
> 1. The comment says "Check noreturn attachment" but the current code
> rejects all FEXIT_MULTI regardless of noreturn status.
> 2. The indentation of the second line matches the alignment pattern
> used when lines are joined by && rather than ||.
> 3. bpf_check_attach_target handles case BPF_TRACE_FEXIT_MULTI as a
> valid attach type in its switch.
>
> Note: In this commit, bpf_trampoline_multi_attach has no callers yet;
> it is infrastructure wired up by a later commit. Once the link creation
> path invokes bpf_trampoline_multi_attach for a FEXIT_MULTI program,
> every such attach will fail with -EINVAL.
guilty as charged, should be &&, will fix
jirka
