Date:   Mon, 16 May 2022 14:00:37 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Tejun Heo <tj@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

Since flush operation synchronously waits for completion, flushing
system-wide WQs (e.g. system_wq) might introduce possibility of deadlock
due to unexpected locking dependency. Tejun Heo commented at [1] that it
makes no sense at all to call flush_workqueue() on the shared WQs as the
caller has no idea what it's gonna end up waiting for.

Although there is flush_scheduled_work() which flushes system_wq WQ with
"Think twice before calling this function! It's very easy to get into
trouble if you don't take great care." warning message, syzbot found a
circular locking dependency caused by flushing system_wq WQ [2].

Therefore, let's change the direction to that developers had better use
their local WQs if flush_scheduled_work()/flush_workqueue(system_*_wq) is
inevitable.

Steps for converting system-wide WQs into local WQs are explained at [3],
and a conversion to stop flushing system-wide WQs is in progress. Now we
want some mechanism for preventing developers who are not aware of this
conversion from again start flushing system-wide WQs.

Since I found that WARN_ON() is complete but awkward approach for teaching
developers about this problem, let's use BUILD_BUG_ON() for incomplete but
handy approach. For completeness, we will also insert WARN_ON() into
flush_workqueue() after all users stopped calling flush_scheduled_work().

Link: https://lore.kernel.org/all/YgnQGZWT%2Fn3VAITX@slm.duckdns.org/ [1]
Link: https://syzkaller.appspot.com/bug?extid=bde0f89deacca7c765b8 [2]
Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp [3]
Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
---
Changes in v3:
  Revert suggested change in v2, for kernel test robot <lkp@...el.com> found

    warning: Function parameter or member 'flush_workqueue' not described in 'void'
    warning: expecting prototype for flush_workqueue(). Prototype was for void() instead

  when built with W=1 option.

Changes in v2:
  Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove
  "#undef flush_workqueue", suggested by Joe Perches <joe@...ches.com>.

This patch is expected to be sent to linux.git immediately before the merge
window for 5.19 closes, for we need to wait until patches for removing
flush_workqueue(system_*_wq) calls reached linux.git during the merge window
for 5.19. Due to this ordering dependency, the kernel build bots complain like
https://lkml.kernel.org/r/202205021448.6SzhD1Cz-lkp@intel.com , but it seems
that simply ignoring these reports is the best choice.

If it is inconvenient for you to send pull request twice from your wq.git tree
(once for immediately after 5.18 is released, the other for immediately before
the merge window for 5.19 closes), I can send pull request for this patch from
my tree. In that case, please give me Acked-by: or Reviewed-by: response regarding
this patch.

 include/linux/workqueue.h | 31 +++++++++++++++++++++++++++++++
 kernel/workqueue.c        |  1 +
 2 files changed, 32 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7fee9b6cfede..5bad503925de 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -663,4 +663,35 @@ int workqueue_offline_cpu(unsigned int cpu);
 void __init workqueue_init_early(void);
 void __init workqueue_init(void);
 
+/*
+ * Detect attempt to flush system-wide workqueues at compile time when possible.
+ * See https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp for
+ * reasons and steps for converting system-wide workqueues into local workqueues.
+ */
+#define flush_workqueue(wq)						\
+({									\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_wq) &&	\
+			 &(wq) == &system_wq,				\
+			 "Please avoid flushing system_wq.");		\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_highpri_wq) && \
+			 &(wq) == &system_highpri_wq,			\
+			 "Please avoid flushing system_highpri_wq.");	\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_long_wq) && \
+			 &(wq) == &system_long_wq,			\
+			 "Please avoid flushing system_long_wq.");	\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_unbound_wq) && \
+			 &(wq) == &system_unbound_wq,			\
+			 "Please avoid flushing system_unbound_wq.");	\
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_wq) &&	\
+			 &(wq) == &system_freezable_wq,			\
+			 "Please avoid flushing system_freezable_wq."); \
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \
+			 &(wq) == &system_power_efficient_wq,		\
+			 "Please avoid flushing system_power_efficient_wq."); \
+	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) &&	\
+			 &(wq) == &system_freezable_power_efficient_wq, \
+			 "Please avoid flushing system_freezable_power_efficient_wq."); \
+	flush_workqueue(wq);						\
+})
+
 #endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0d2514b4ff0d..08255c332e4b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2794,6 +2794,7 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
  * This function sleeps until all work items which were queued on entry
  * have finished execution, but it is not livelocked by new incoming ones.
  */
+#undef flush_workqueue
 void flush_workqueue(struct workqueue_struct *wq)
 {
 	struct wq_flusher this_flusher = {
-- 
2.34.1

Powered by blists - more mailing lists