Re: [PATCH v3] locking: Fix __clear_task_blocked_on() warning from __ww_mutex_wound() path

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Den 2025-08-05 kl. 02:10, skrev John Stultz:
> The __clear_task_blocked_on() helper added a number of sanity
> checks ensuring we hold the mutex wait lock and that the task
> we are clearing blocked_on pointer (if set) matches the mutex.
> 
> However, there is an edge case in the _ww_mutex_wound() logic
> where we need to clear the blocked_on pointer for the task that
> owns the mutex, not the task that is waiting on the mutex.
> 
> For this case the sanity checks aren't valid, so handle this
> by allowing a NULL lock to skip the additional checks.
> 
> K Prateek Nayak and Maarten Lankhorst also pointed out that in
> this case where we don't hold the owner's mutex wait_lock, we
> need to be a bit more careful using READ_ONCE/WRITE_ONCE in both
> the __clear_task_blocked_on() and __set_task_blocked_on()
> implementations to avoid accidentally tripping WARN_ONs if two
> instances race. So do that here as well.
> 
> This issue was easier to miss, I realized, as the test-ww_mutex
> driver only exercises the wait-die class of ww_mutexes. I've
> sent a patch[1] to address this so the logic will be easier to
> test.
> 
> [1]: https://lore.kernel.org/lkml/20250801023358.562525-2-jstultz@google.com/
> 
> Fixes: a4f0b6fef4b0 ("locking/mutex: Add p->blocked_on wrappers for correctness checks")
> Reported-by: syzbot+602c4720aed62576cd79@syzkaller.appspotmail.com
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Closes: https://lore.kernel.org/lkml/68894443.a00a0220.26d0e1.0015.GAE@google.com/
> Signed-off-by: John Stultz <jstultz@google.com>
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> v2:
> * Rewording of "lock" to "mutex" in commit and comment for
>   clarity
> * Rework __clear_task_blocked_on() to use READ_ONCE and WRITE_ONCE
>   so we don't trip over the WARNING if two instances race, as suggested
>   by K Prateek Nayak and Maarten Lankhorst
> v3:
> * Add READ_ONCE and WRITE_ONCE to __set_task_blocked_on(), to avoid
>   tripping similar warnings as suggested by K Prateek Nayak
> 
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: K Prateek Nayak <kprateek.nayak@amd.com>
> Cc: Suleiman Souhlal <suleiman@google.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: airlied@gmail.com
> Cc: mripard@kernel.org
> Cc: simona@ffwll.ch
> Cc: tzimmermann@suse.de
> Cc: dri-devel@lists.freedesktop.org
> Cc: kernel-team@android.com
> ---
>  include/linux/sched.h     | 29 +++++++++++++++++------------
>  kernel/locking/ww_mutex.h |  6 +++++-
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 40d2fa90df425..62103dd6a48e0 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2144,6 +2144,8 @@ static inline struct mutex *__get_task_blocked_on(struct task_struct *p)
>  
>  static inline void __set_task_blocked_on(struct task_struct *p, struct mutex *m)
>  {
> +	struct mutex *blocked_on = READ_ONCE(p->blocked_on);
> +
>  	WARN_ON_ONCE(!m);
>  	/* The task should only be setting itself as blocked */
>  	WARN_ON_ONCE(p != current);
> @@ -2154,8 +2156,8 @@ static inline void __set_task_blocked_on(struct task_struct *p, struct mutex *m)
>  	 * with a different mutex. Note, setting it to the same
>  	 * lock repeatedly is ok.
>  	 */
> -	WARN_ON_ONCE(p->blocked_on && p->blocked_on != m);
> -	p->blocked_on = m;
> +	WARN_ON_ONCE(blocked_on && blocked_on != m);
> +	WRITE_ONCE(p->blocked_on, m);
>  }
>  
>  static inline void set_task_blocked_on(struct task_struct *p, struct mutex *m)
> @@ -2166,16 +2168,19 @@ static inline void set_task_blocked_on(struct task_struct *p, struct mutex *m)
>  
>  static inline void __clear_task_blocked_on(struct task_struct *p, struct mutex *m)
>  {
> -	WARN_ON_ONCE(!m);
> -	/* Currently we serialize blocked_on under the mutex::wait_lock */
> -	lockdep_assert_held_once(&m->wait_lock);
> -	/*
> -	 * There may be cases where we re-clear already cleared
> -	 * blocked_on relationships, but make sure we are not
> -	 * clearing the relationship with a different lock.
> -	 */
> -	WARN_ON_ONCE(m && p->blocked_on && p->blocked_on != m);
> -	p->blocked_on = NULL;
> +	if (m) {
> +		struct mutex *blocked_on = READ_ONCE(p->blocked_on);
> +
> +		/* Currently we serialize blocked_on under the mutex::wait_lock */
> +		lockdep_assert_held_once(&m->wait_lock);
> +		/*
> +		 * There may be cases where we re-clear already cleared
> +		 * blocked_on relationships, but make sure we are not
> +		 * clearing the relationship with a different lock.
> +		 */
> +		WARN_ON_ONCE(blocked_on && blocked_on != m);
> +	}
> +	WRITE_ONCE(p->blocked_on, NULL);
>  }
>  
>  static inline void clear_task_blocked_on(struct task_struct *p, struct mutex *m)
> diff --git a/kernel/locking/ww_mutex.h b/kernel/locking/ww_mutex.h
> index 086fd5487ca77..31a785afee6c0 100644
> --- a/kernel/locking/ww_mutex.h
> +++ b/kernel/locking/ww_mutex.h
> @@ -342,8 +342,12 @@ static bool __ww_mutex_wound(struct MUTEX *lock,
>  			 * When waking up the task to wound, be sure to clear the
>  			 * blocked_on pointer. Otherwise we can see circular
>  			 * blocked_on relationships that can't resolve.
> +			 *
> +			 * NOTE: We pass NULL here instead of lock, because we
> +			 * are waking the mutex owner, who may be currently
> +			 * blocked on a different mutex.
>  			 */
> -			__clear_task_blocked_on(owner, lock);
> +			__clear_task_blocked_on(owner, NULL);
>  			wake_q_add(wake_q, owner);
>  		}
>  		return true;