linux-kernel - Re: [RFC][PATCH] locking/rtmutex: Make sure we wake anything on the wake_q when we release the lock->wait

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241213124614.GA12338@noisy.programming.kicks-ass.net>
Date: Fri, 13 Dec 2024 13:46:14 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: John Stultz <jstultz@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...hat.com>,
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
	Boqun Feng <boqun.feng@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Bert Karwatzki <spasswolf@....de>, kernel-team@...roid.com
Subject: Re: [RFC][PATCH] locking/rtmutex: Make sure we wake anything on the
 wake_q when we release the lock->wait_lock

On Thu, Dec 12, 2024 at 02:21:33PM -0800, John Stultz wrote:
> Bert reported seeing occasional boot hangs when running with
> PREEPT_RT and bisected it down to commit 894d1b3db41c
> ("locking/mutex: Remove wakeups from under mutex::wait_lock").
> 
> It looks like I missed a few spots where we drop the wait_lock and
> potentially call into schedule without waking up the tasks on the
> wake_q structure. Since the tasks being woken are ww_mutex tasks
> they need to be able to run to release the mutex and unblock the
> task that currently is planning to wake them. Thus we can deadlock.
> 
> So make sure we wake the wake_q tasks when we unlock the wait_lock.
> 
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Waiman Long <longman@...hat.com>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Bert Karwatzki <spasswolf@....de>
> Cc: kernel-team@...roid.com
> Reported-by: Bert Karwatzki <spasswolf@....de>
> Closes: https://lore.kernel.org/lkml/20241211182502.2915-1-spasswolf@web.de
> Fixes: 894d1b3db41c ("locking/mutex: Remove wakeups from under mutex::wait_lock")
> Signed-off-by: John Stultz <jstultz@...gle.com>
> ---

I don't suppose this actually makes things much better -- but I had to
try.


--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1192,6 +1192,17 @@ try_to_take_rt_mutex(struct rt_mutex_bas
 	return 1;
 }
 
+#define WRAP_WAKE(_stmt, _q) \
+do { \
+	struct wake_q_head *_Q = (_q); \
+	guard(preempt)(); \
+	_stmt; \
+	if (_Q && !wake_q_empty(_Q)) { \
+		wake_up_q(_Q); \
+		wake_q_init(_Q); \
+	} \
+} while (0)
+
 /*
  * Task blocks on lock.
  *
@@ -1248,10 +1259,7 @@ static int __sched task_blocks_on_rt_mut
 
 		/* Check whether the waiter should back out immediately */
 		rtm = container_of(lock, struct rt_mutex, rtmutex);
-		preempt_disable();
-		res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, wake_q);
-		wake_up_q(wake_q);
-		preempt_enable();
+		WRAP_WAKE(res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, wake_q), wake_q);
 		if (res) {
 			raw_spin_lock(&task->pi_lock);
 			rt_mutex_dequeue(lock, waiter);
@@ -1295,13 +1303,7 @@ static int __sched task_blocks_on_rt_mut
 	 */
 	get_task_struct(owner);
 
-	preempt_disable();
-	raw_spin_unlock_irq(&lock->wait_lock);
-	/* wake up any tasks on the wake_q before calling rt_mutex_adjust_prio_chain */
-	wake_up_q(wake_q);
-	wake_q_init(wake_q);
-	preempt_enable();
-
+	WRAP_WAKE(raw_spin_unlock_irq(&lock->wait_lock), wake_q);
 
 	res = rt_mutex_adjust_prio_chain(owner, chwalk, lock,
 					 next_lock, waiter, task);
@@ -1645,13 +1647,8 @@ static int __sched rt_mutex_slowlock_blo
 			owner = rt_mutex_owner(lock);
 		else
 			owner = NULL;
-		preempt_disable();
-		raw_spin_unlock_irq(&lock->wait_lock);
-		if (wake_q) {
-			wake_up_q(wake_q);
-			wake_q_init(wake_q);
-		}
-		preempt_enable();
+
+		WRAP_WAKE(raw_spin_unlock_irq(&lock->wait_lock), wake_q);
 
 		if (!owner || !rtmutex_spin_on_owner(lock, waiter, owner))
 			rt_mutex_schedule();
@@ -1802,10 +1799,7 @@ static int __sched rt_mutex_slowlock(str
 	 */
 	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 	ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state, &wake_q);
-	preempt_disable();
-	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
-	wake_up_q(&wake_q);
-	preempt_enable();
+	WRAP_WAKE(raw_spin_unlock_irqrestore(&lock->wait_lock, flags), &wake_q);
 	rt_mutex_post_schedule();
 
 	return ret;
@@ -1863,11 +1857,8 @@ static void __sched rtlock_slowlock_lock
 			owner = rt_mutex_owner(lock);
 		else
 			owner = NULL;
-		preempt_disable();
-		raw_spin_unlock_irq(&lock->wait_lock);
-		wake_up_q(wake_q);
-		wake_q_init(wake_q);
-		preempt_enable();
+
+		WRAP_WAKE(raw_spin_unlock_irq(&lock->wait_lock), wake_q);
 
 		if (!owner || !rtmutex_spin_on_owner(lock, &waiter, owner))
 			schedule_rtlock();
@@ -1896,10 +1887,8 @@ static __always_inline void __sched rtlo
 
 	raw_spin_lock_irqsave(&lock->wait_lock, flags);
 	rtlock_slowlock_locked(lock, &wake_q);
-	preempt_disable();
-	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
-	wake_up_q(&wake_q);
-	preempt_enable();
+
+	WRAP_WAKE(raw_spin_unlock_irqrestore(&lock->wait_lock, flags), &wake_q);
 }
 
 #endif /* RT_MUTEX_BUILD_SPINLOCKS */