linux-kernel - [PATCH] workqueue: ensure all flush_work() completed when being destoryed

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]

Date:   Tue,  2 Jun 2020 13:49:14 +0000
From:   Lai Jiangshan <laijs@...ux.alibaba.com>
To:     linux-kernel@...r.kernel.org
Cc:     Lai Jiangshan <laijs@...ux.alibaba.com>, Tejun Heo <tj@...nel.org>,
        Lai Jiangshan <jiangshanlai@...il.com>
Subject: [PATCH] workqueue: ensure all flush_work() completed when being destoryed

In old days, worker threads are not shared among different
workqueues and destroy_workqueue() used kthread_stop() to destroy
all workers before going to destroy workqueue structures.
And kthread_stop() can ensure the scheduled (worker->scheduled)
work items and the linked work items queued by flush_work()
to be completed.

For a workqueue to be completed/unused for wq users means that all
queued works have completed and all flush_work() have return,
and the workqueue is legitimate to passed to destroy_workqueue().

But
e22bee782b3b("workqueue: implement concurrency managed dynamic worker pool")
made worker pools and workers shared among different
workqueues and kthread_stop() is not used to sync the completion
of the work items. destroy_workqueue() uses drain_workqueue()
to drain user work items, but internal work items queued by
flush_work() is not drained due to they don't have colors.

So problems may occur when wq_barrier_func() does complete(&barr->done)
and the wokenup wq-user code does destroy_workqueue(). destroy_workqueue()
can be scheduled eariler than the proccess_one_work() to do
the put_pwq(), so that the sanity check in destroy_workqueue()
can see the no yet put pwq->refcnt and blame false positively.

The problem can be easily fixed by removing the WORK_NO_COLOR
and making the internal work item queued by flush_work() inherit
the color of the work items to be flushed. It would definitely
revert the design and the benefits of the WORK_NO_COLOR.

The patch simply adds an atomic counter for in-flight flush_work()
and a completion for destroy_workqueue() waiting for them.

Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
---
Changed from V1:
	Change from flush_no_color based mechanism to atomic+completion
	based as TJ suggested.

 kernel/workqueue.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1921c982f920..71272beb8e01 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -253,6 +253,9 @@ struct workqueue_struct {
 	int			nr_drainers;	/* WQ: drain in progress */
 	int			saved_max_active; /* WQ: saved pwq max_active */

+	atomic_t		nr_flush_work;	/* flush work in progress */
+	struct completion	flush_work_done; /* sync flush_work() */
+
 	struct workqueue_attrs	*unbound_attrs;	/* PW: only for unbound wqs */
 	struct pool_workqueue	*dfl_pwq;	/* PW: only for unbound wqs */

@@ -1154,6 +1157,12 @@ static void pwq_activate_first_delayed(struct pool_workqueue *pwq)
 	pwq_activate_delayed_work(work);
 }

+static void dec_nr_in_flight_flush_work(struct workqueue_struct *wq)
+{
+	if (atomic_dec_and_test(&wq->nr_flush_work))
+		complete(&wq->flush_work_done);
+}
+
 /**
  * pwq_dec_nr_in_flight - decrement pwq's nr_in_flight
  * @pwq: pwq of interest
@@ -1168,8 +1177,10 @@ static void pwq_activate_first_delayed(struct pool_workqueue *pwq)
 static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, int color)
 {
 	/* uncolored work items don't participate in flushing or nr_active */
-	if (color == WORK_NO_COLOR)
+	if (color == WORK_NO_COLOR) {
+		dec_nr_in_flight_flush_work(pwq->wq);
 		goto out_put;
+	}

 	pwq->nr_in_flight[color]--;

@@ -2682,6 +2693,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	}

 	debug_work_activate(&barr->work);
+	atomic_inc(&pwq->wq->nr_flush_work);
 	insert_work(pwq, &barr->work, head,
 		    work_color_to_flags(WORK_NO_COLOR) | linked);
 }
@@ -4278,6 +4290,9 @@ struct workqueue_struct *alloc_workqueue(const char *fmt,
 	wq_init_lockdep(wq);
 	INIT_LIST_HEAD(&wq->list);

+	atomic_set(&wq->nr_flush_work, 1);
+	init_completion(&wq->flush_work_done);
+
 	if (alloc_and_link_pwqs(wq) < 0)
 		goto err_unreg_lockdep;

@@ -4354,6 +4369,10 @@ void destroy_workqueue(struct workqueue_struct *wq)
 	/* drain it before proceeding with destruction */
 	drain_workqueue(wq);

+	/* flush all uncompleted internal work items queued by flush_work() */
+	dec_nr_in_flight_flush_work(wq);
+	wait_for_completion(&wq->flush_work_done);
+
 	/* kill rescuer, if sanity checks fail, leave it w/o rescuer */
 	if (wq->rescuer) {
 		struct worker *rescuer = wq->rescuer;
-- 
2.20.1