| 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: <20171008090225.27034-1-boqun.feng@gmail.com>
Date: Sun, 8 Oct 2017 17:02:23 +0800
From: Boqun Feng <boqun.feng@...il.com>
To: linux-kernel@...r.kernel.org
Cc: Josef Bacik <josef@...icpanda.com>,
Boqun Feng <boqun.feng@...il.com>,
Peter Zijlstra <peterz@...radead.org>,
Tejun Heo <tj@...nel.org>,
Lai Jiangshan <jiangshanlai@...il.com>
Subject: [RFC] workqueue: Fix irq inversion deadlock in manage_workers()
Josef reported a HARDIRQ-safe -> HARDIRQ-unsafe lock order detected by
lockdep:
| [ 1270.472259] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
| [ 1270.472783] 4.14.0-rc1-xfstests-12888-g76833e8 #110 Not tainted
| [ 1270.473240] -----------------------------------------------------
| [ 1270.473710] kworker/u5:2/5157 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
| [ 1270.474239] (&(&lock->wait_lock)->rlock){+.+.}, at: [<ffffffff8da253d2>] __mutex_unlock_slowpath+0xa2/0x280
| [ 1270.474994]
| [ 1270.474994] and this task is already holding:
| [ 1270.475440] (&pool->lock/1){-.-.}, at: [<ffffffff8d2992f6>] worker_thread+0x366/0x3c0
| [ 1270.476046] which would create a new lock dependency:
| [ 1270.476436] (&pool->lock/1){-.-.} -> (&(&lock->wait_lock)->rlock){+.+.}
| [ 1270.476949]
| [ 1270.476949] but this new dependency connects a HARDIRQ-irq-safe lock:
| [ 1270.477553] (&pool->lock/1){-.-.}
...
| [ 1270.488900] to a HARDIRQ-irq-unsafe lock:
| [ 1270.489327] (&(&lock->wait_lock)->rlock){+.+.}
...
| [ 1270.494735] Possible interrupt unsafe locking scenario:
| [ 1270.494735]
| [ 1270.495250] CPU0 CPU1
| [ 1270.495600] ---- ----
| [ 1270.495947] lock(&(&lock->wait_lock)->rlock);
| [ 1270.496295] local_irq_disable();
| [ 1270.496753] lock(&pool->lock/1);
| [ 1270.497205] lock(&(&lock->wait_lock)->rlock);
| [ 1270.497744] <Interrupt>
| [ 1270.497948] lock(&pool->lock/1);
, which will cause a irq inversion deadlock if the above lock scenario
happens.
The root cause of this safe -> unsafe lock order is the
mutex_unlock(pool::manager_arb) in manage_workers() with pool::lock
held. An obvious fix is dropping the pool::lock before mutex_unlock()
and re-grabing afterwards, which however will introduce a race condition
between worker_thread() and put_unbound_pool():
put_unbound_pool() will grab both pool::manager_arb and pool::lock to
set all current IDLE workers to DIE, and may wait on the
pool::detach_completion for the last worker to detach from the pool.
And when manage_workers() is called, the caller worker_thread is in
non-ILDE state, so if the worker dropped both pool::{manager_arb, lock}
and got delayed for a while long enough for a put_unbound_pool(), the
put_unbound_pool() would not switch that worker to DIE. As a result, the
worker will not detach from the pool as it's not DIE and the
put_unbound_pool() will not proceed as it's waiting for the last worker
to detach, therefore deadlock.
To overcome this, put the worker back to IDLE state before it drops
pool::lock in manage_workers(), and make the worker check again whether
it's DIE after it re-grabs the pool::lock. In this way, we fix the
potential deadlock reported by lockdep without introducing another.
Reported-by: Josef Bacik <josef@...icpanda.com>
Signed-off-by: Boqun Feng <boqun.feng@...il.com>
Cc: Peter Zijlstra <peterz@...radead.org>
---
kernel/workqueue.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 64d0edf428f8..2ea7b04cc48b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1997,7 +1997,40 @@ static bool manage_workers(struct worker *worker)
maybe_create_worker(pool);
pool->manager = NULL;
+
+ /*
+ * Put the manager back to ->idle_list, this allows us to drop the
+ * pool->lock safely without racing with put_unbound_pool()
+ *
+ * <in "manager worker" thread>
+ * worker_thread():
+ * spin_lock_irq(&pool->lock);
+ * worker_leave_idle();
+ * manage_workers(): // return true
+ * mutex_trylock(&pool->manager_arb);
+ * <without entering idle here>
+ * spin_unlock_irq(&pool->lock);
+ * mutex_unlock(&pool->manager_arb);
+ *
+ * put_unbound_pool():
+ * mutex_lock(&pool->manager_arb);
+ * spin_lock_irq(&pool->lock);
+ * <set ILDE worker to DIE>
+ * <the manager worker is not set to be DIE, because it's not IDLE>
+ * ...
+ * wait_for_completion(&pool->detach_completion);
+ * <no one will complete() because pool->workers is not empty>
+ *
+ * spin_lock_irq(&pool->lock);
+ * <pool->worklist is empty, go to sleep>
+ *
+ * No one is going to wake up the manager worker, even so, it won't
+ * complete(->detach_completion), since it's not a DIE worker.
+ */
+ worker_enter_idle(worker);
+ spin_unlock_irq(&pool->lock);
mutex_unlock(&pool->manager_arb);
+ spin_lock_irq(&pool->lock);
return true;
}
@@ -2202,6 +2235,7 @@ static int worker_thread(void *__worker)
woke_up:
spin_lock_irq(&pool->lock);
+recheck:
/* am I supposed to die? */
if (unlikely(worker->flags & WORKER_DIE)) {
spin_unlock_irq(&pool->lock);
@@ -2216,7 +2250,6 @@ static int worker_thread(void *__worker)
}
worker_leave_idle(worker);
-recheck:
/* no more worker necessary? */
if (!need_more_worker(pool))
goto sleep;
--
2.14.1
Powered by blists - more mailing lists