| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d737c0f0-c0e0-4df5-8246-b484db8d061b@linux.intel.com>
Date: Tue, 5 Aug 2025 11:27:47 +0200
From: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To: John Stultz <jstultz@...gle.com>, LKML <linux-kernel@...r.kernel.org>
Cc: syzbot+602c4720aed62576cd79@...kaller.appspotmail.com,
K Prateek Nayak <kprateek.nayak@....com>, Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>, Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Valentin Schneider <valentin.schneider@....com>,
Suleiman Souhlal <suleiman@...gle.com>, airlied@...il.com,
mripard@...nel.org, simona@...ll.ch, tzimmermann@...e.de,
dri-devel@...ts.freedesktop.org, kernel-team@...roid.com
Subject: Re: [PATCH v3] locking: Fix __clear_task_blocked_on() warning from
__ww_mutex_wound() path
Acked-by: Maarten Lankhorst <maarten.lankhorst@...ux.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@...kaller.appspotmail.com
> Reported-by: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Closes: https://lore.kernel.org/lkml/68894443.a00a0220.26d0e1.0015.GAE@google.com/
> Signed-off-by: John Stultz <jstultz@...gle.com>
> Reviewed-by: K Prateek Nayak <kprateek.nayak@....com>
> Tested-by: K Prateek Nayak <kprateek.nayak@....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@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Juri Lelli <juri.lelli@...hat.com>
> Cc: Vincent Guittot <vincent.guittot@...aro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@....com>
> Cc: Valentin Schneider <valentin.schneider@....com>
> Cc: K Prateek Nayak <kprateek.nayak@....com>
> Cc: Suleiman Souhlal <suleiman@...gle.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Cc: airlied@...il.com
> Cc: mripard@...nel.org
> Cc: simona@...ll.ch
> Cc: tzimmermann@...e.de
> Cc: dri-devel@...ts.freedesktop.org
> Cc: kernel-team@...roid.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;
Powered by blists - more mailing lists