linux-kernel - Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

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:   Thu, 5 May 2022 22:42:19 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Tejun Heo <tj@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Joe Perches <joe@...ches.com>,
        Andy Whitcroft <apw@...onical.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Subject: Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

On 2022/04/25 9:33, Tetsuo Handa wrote:
> On 2022/04/25 8:45, Joe Perches wrote:
>> And it's probably more readable using separate lines and it looks as
>> if the 3rd test is unnecessary as it could be combined with the 2nd.
>>
>> 		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
>> 		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {
> 
> We don't need to worry about possibility like
> 
> 	flush_workqueue(system_module1_wq);
> 
> ? Then, we can simplify like you suggested.

I initially thought that also doing static checks by scripts/checkpatch.pl
is better than only doing runtime WARN_ON(). But not all patches are checked
by scripts/checkpatch.pl . Thus, as an attempt to check without exemptions,
I now think that doing static checks via BUILD_BUG_ON() is better than
scripts/checkpatch.pl . I sent below patch to linux-next.git , and so far
it seems working (I mean, no build failure reports caused by compilers).

Subject: workqueue: Wrap flush_workqueue() using a macro

A conversion to stop flushing kernel-global workqueues is in progress.
Wrap flush_workqueue() and inject BUILD_BUG_ON() checks, in order to
prevent users who are not aware of this conversion from again start
flushing kernel-global workqueues.

Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
---
 include/linux/workqueue.h | 21 +++++++++++++++++++++
 kernel/workqueue.c        |  1 +
 2 files changed, 22 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7b13fae377e2e..9f78414d507c8 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -654,4 +654,25 @@ 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.
+ * Checks for system_wq and system_{long,unbound,highpri}_wq will be added after
+ * all in-tree users stopped flushing these workqueues.
+ */
+#define flush_workqueue(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 abcc9a2ac3197..5c612532f3e93 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2813,6 +2813,7 @@ static void warn_flush_attempt(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 = {
-- 

Tejun, can we use this approach? If yes, when to apply?

If we can include this patch into 5.18, can be applied as-is.
If we can include this patch into 5.19, can be applied with checks for
system_{long,unbound,highpri}_wq added because all flush_workqueue() users
on system_*_wq are gone in next-20220505.

Now that all flush_workqueue() users on system_*_wq are gone in next-20220505,
next step is to remove all flush_scheduled_work() users. Therefore, instead of
using /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/ in scripts/checkpatch.pl ,
I think we can ask BUILD_BUG_ON() for blocking new system_*_wq users, and ask
scripts/checkpatch.pl for blocking new system_wq users.