Re: [PATCHv6 bpf-next 04/29] bpf: Use mutex lock pool for bpf trampolines

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hash collisions in the new global trampoline lock pool cause unrelated kprobe, fprobe, and BPF link registrations to spuriously fail with -EAGAIN.
- [Low] Suboptimal pointer hashing API used for trampoline pointers.
- [Low] Multi-line comments modified by the patch do not follow the modern kernel comment style for the BPF subsystem.
--

commit db06fb69cc58248a60a56902b3ac29b2878a30f9
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Use mutex lock pool for bpf trampolines

This commit replaces the per-trampoline mutex with a global pool of 32
mutexes to reduce lock depth during multi-trampoline attachment.
Trampolines hash into this pool to acquire locks.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index a4298a25d4bae..df7634de0ca2c 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> +static struct mutex *select_trampoline_lock(struct bpf_trampoline *tr)
> +{
> +	return &trampoline_locks[hash_64((u64)(uintptr_t) tr, TRAMPOLINE_LOCKS_BITS)].mutex;
> +}

[Severity: Low]
This isn't a bug, but could we use the dedicated hash_ptr() macro from
linux/hash.h here instead of casting the pointer to u64?

[ ... ]

>  	if (cmd == FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF) {
>  		/* This is called inside register_ftrace_direct_multi(), so
> -		 * tr->mutex is already locked.
> +		 * trampoline's mutex is already locked.
>  		 */

[Severity: Low]
This isn't a bug, but since this multi-line comment is being modified,
should the opening /* be placed on its own line to match the modern
BPF subsystem comment style?

[ ... ]

>  	 * In both cases, direct_mutex is already locked. Use
> -	 * mutex_trylock(&tr->mutex) to avoid deadlock in race condition
> -	 * (something else is making changes to this same trampoline).
> +	 * mutex_trylock(select_trampoline_lock(tr)) to avoid deadlock in race condition
> +	 * (something else holds the same pool lock).
>  	 */
> -	if (!mutex_trylock(&tr->mutex)) {
> -		/* sleep 1 ms to make sure whatever holding tr->mutex makes
> -		 * some progress.
> +	if (!mutex_trylock(select_trampoline_lock(tr))) {

[Severity: High]
Could this shared lock pool cause unrelated probe registrations to spuriously
fail with -EAGAIN?

If a tracing tool registers an ftrace function with FTRACE_OPS_FL_IPMODIFY,
ftrace calls the trampoline's ops_func callback.

If another unrelated BPF trampoline is concurrently being modified and
happens to hash to the same bucket in trampoline_locks, this mutex_trylock
will fail.

Since upstream callers do not retry on -EAGAIN, will this false sharing
lead to unpredictable probe attachment failures under load?

> +		/* sleep 1 ms to make sure whatever holding select_trampoline_lock(tr)
> +		 * makes some progress.
>  		 */
>  		msleep(1);
>  		return -EAGAIN;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527113951.46265-5-jolsa@kernel.org?part=1