Re: [PATCH 3/5] locking/qspinlock: Add contended_release tracepoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The inline function `queued_spin_unlock()` references a GPL-only tracepoint symbol, inadvertently turning `spin_unlock()` into a GPL-only API and breaking non-GPL out-of-tree modules.
- [High] The lock release tracepoint bypasses the RCU watching check, allowing trace probes to execute unsafely in idle/offline contexts.
--

--- Patch [3]: [PATCH 3/5] locking/qspinlock: Add contended_release tracepoint ---
commit afc21b7e9fbc296817ffabe3684c2756069801e5
Author: Dmitry Ilvokhin <d@ilvokhin.com>

locking/qspinlock: Add contended_release tracepoint

This commit adds a contended_release tracepoint to qspinlock to allow
attributing a contended spinlock to its holder. It combines the contention
check and trace call in an out-of-line function to minimize overhead on
the hot path. The feature is gated behind a new configuration option.

Link: https://lore.kernel.org/all/aiphFXe_TPNPxZ_n@shell.ilvokhin.com/

> diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> index ae45289e8ec7f..2ca94e41823b9 100644
> --- a/include/asm-generic/qspinlock.h
> +++ b/include/asm-generic/qspinlock.h
[ ... ]
> @@ -130,12 +131,32 @@ static __always_inline void queued_spin_release(struct qspinlock *lock)
>  #endif
>  
>  #ifndef queued_spin_unlock
> +
> +DECLARE_TRACEPOINT(contended_release);
> +
> +extern void queued_spin_release_traced(struct qspinlock *lock);
> +
>  /**
>   * queued_spin_unlock - unlock a queued spinlock
>   * @lock : Pointer to queued spinlock structure
> + *
> + * Generic tracing wrapper around the arch-overridable
> + * queued_spin_release().
>   */
>  static __always_inline void queued_spin_unlock(struct qspinlock *lock)
>  {
> +	/*
> +	 * Trace and release are combined in queued_spin_release_traced() so
> +	 * the compiler does not need to preserve the lock pointer across the
> +	 * function call, avoiding callee-saved register save/restore on the
> +	 * hot path. queued_spin_release() is therefore called both here and in
> +	 * queued_spin_release_traced(). Keep the two in sync.
> +	 */
> +	if (IS_ENABLED(CONFIG_QUEUED_SPINLOCKS_TRACE_CONTENDED_RELEASE) &&
> +	    tracepoint_enabled(contended_release)) {

[Severity: High]
Does this inadvertently make spin_unlock() a GPL-only API?

The contended_release tracepoint is exported via EXPORT_TRACEPOINT_SYMBOL_GPL
in mutex.c.

When spin_unlock() is inlined, this injects the GPL-only symbol into every
module calling spin_unlock().

Would this break the compilation or loading of non-GPL out-of-tree modules
due to referencing a GPL-only symbol?

> +		queued_spin_release_traced(lock);
> +		return;
> +	}
>  	queued_spin_release(lock);
>  }
>  #endif
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index af8d122bb649e..33fe6d437c8f6 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
[ ... ]
> @@ -104,6 +104,28 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
>  #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
>  #endif
>  
> +#if !defined(queued_spin_unlock) && \
> +	IS_ENABLED(CONFIG_QUEUED_SPINLOCKS_TRACE_CONTENDED_RELEASE)
> +/*
> + * Out-of-line trace-and-release path for queued_spin_unlock(), used when
> + * the contended_release tracepoint is enabled.
> + *
> + * queued_spin_release() is duplicated here on purpose: doing the release
> + * in this function (rather than tracing here and releasing in the caller)
> + * lets queued_spin_unlock() return right after the call, so the
> + * tracepoint-disabled hot path never has to keep lock live across a call
> + * in a callee-saved register. Keep this release in sync with the one in
> + * queued_spin_unlock().
> + */
> +void __lockfunc queued_spin_release_traced(struct qspinlock *lock)
> +{
> +	if (queued_spin_is_contended(lock))
> +		trace_call__contended_release(lock);

[Severity: High]
Does calling trace_call__contended_release() directly bypass the RCU
watching checks?

Raw spinlocks are often used in the scheduler idle loop or CPU hotplug
paths where RCU is explicitly not watching (for example, after
rcu_idle_enter() is called).

The standard trace_contended_release() wrapper includes a lockdep safety
check to warn if RCU is not watching.

Could this direct call allow trace probes to execute unsafely in idle or
offline contexts, potentially causing RCU splats or use-after-free bugs if
attached BPF programs or ftrace plugins dereference RCU-protected data?

> +	queued_spin_release(lock);
> +}
> +EXPORT_SYMBOL(queued_spin_release_traced);
> +#endif
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785778551.git.d@ilvokhin.com?part=3