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

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Apr 17, 2026 at 08:22:36PM +0000, bot+bpf-ci@kernel.org 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