linux-kernel - [PATCH wq/for-5.2-fixes] workqueue: Fix pwq ref leak in rescuer

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>] [day] [month] [year] [list]

Message-ID: <20191004173649.GB3404308@devbig004.ftw2.facebook.com>
Date:   Fri, 4 Oct 2019 10:36:49 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Lai Jiangshan <jiangshanlai@...il.com>
Cc:     linux-kernel@...r.kernel.org, kernel-team@...com,
        NeilBrown <neilb@...e.de>,
        "Williams, Gerald S" <gerald.s.williams@...el.com>
Subject: [PATCH wq/for-5.2-fixes] workqueue: Fix pwq ref leak in
 rescuer_thread()

>From e66b39af00f426b3356b96433d620cb3367ba1ff Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@...nel.org>
Date: Wed, 25 Sep 2019 06:59:15 -0700
Subject: [PATCH 2/2] workqueue: Fix pwq ref leak in rescuer_thread()

008847f66c3 ("workqueue: allow rescuer thread to do more work.") made
the rescuer worker requeue the pwq immediately if there may be more
work items which need rescuing instead of waiting for the next mayday
timer expiration.  Unfortunately, it doesn't check whether the pwq is
already on the mayday list and unconditionally gets the ref and moves
it onto the list.  This doesn't corrupt the list but creates an
additional reference to the pwq.  It got queued twice but will only be
removed once.

This leak later can trigger pwq refcnt warning on workqueue
destruction and prevent freeing of the workqueue.

Signed-off-by: Tejun Heo <tj@...nel.org>
Cc: "Williams, Gerald S" <gerald.s.williams@...el.com>
Cc: NeilBrown <neilb@...e.de>
Cc: stable@...r.kernel.org # v3.19+
---
Applying to wq/for-5.4-fixes.

Thanks.

 kernel/workqueue.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 4a3c30177b94..4dc8270326d7 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2533,8 +2533,14 @@ static int rescuer_thread(void *__rescuer)
 			 */
 			if (need_to_create_worker(pool)) {
 				spin_lock(&wq_mayday_lock);
-				get_pwq(pwq);
-				list_move_tail(&pwq->mayday_node, &wq->maydays);
+				/*
+				 * Queue iff we aren't racing destruction
+				 * and somebody else hasn't queued it already.
+				 */
+				if (wq->rescuer && list_empty(&pwq->mayday_node)) {
+					get_pwq(pwq);
+					list_add_tail(&pwq->mayday_node, &wq->maydays);
+				}
 				spin_unlock(&wq_mayday_lock);
 			}
 		}
@@ -4374,8 +4380,8 @@ void destroy_workqueue(struct workqueue_struct *wq)
 	for_each_pwq(pwq, wq) {
 		spin_lock_irq(&pwq->pool->lock);
 		if (WARN_ON(pwq_busy(pwq))) {
-			pr_warning("%s: %s has the following busy pwq (refcnt=%d)\n",
-				   __func__, wq->name, pwq->refcnt);
+			pr_warning("%s: %s has the following busy pwq\n",
+				   __func__, wq->name);
 			show_pwq(pwq);
 			spin_unlock_irq(&pwq->pool->lock);
 			mutex_unlock(&wq->mutex);
@@ -4670,7 +4676,8 @@ static void show_pwq(struct pool_workqueue *pwq)
 	pr_info("  pwq %d:", pool->id);
 	pr_cont_pool_info(pool);
 
-	pr_cont(" active=%d/%d%s\n", pwq->nr_active, pwq->max_active,
+	pr_cont(" active=%d/%d refcnt=%d%s\n",
+		pwq->nr_active, pwq->max_active, pwq->refcnt,
 		!list_empty(&pwq->mayday_node) ? " MAYDAY" : "");
 
 	hash_for_each(pool->busy_hash, bkt, worker, hentry) {
-- 
2.17.1