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

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, May 27, 2026 at 12:21:43PM +0000, sashiko-bot@kernel.org wrote:

SNIP

> > +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?

ok

> 
> [ ... ]
> 
> >  	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?

we have comments like that in this function and plenty all over the
file, I'll keep it


> 
> [ ... ]
> 
> >  	 * 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?

I addressed that in cover letter:

- after discussing with Song, it was determined that cleaning up FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER
  is not strictly necessary — keeping the trampoline in the ipmodify_enabled state is acceptable.
  The race condition this introduces remains unlikely, so the concern raised in [1] will not be
  addressed at this time.
  [1] https://lore.kernel.org/bpf/aec7bAbGlnEo3R1g@krava/


jirka