linux-kernel - Re: [PATCH v3] workqueue: Wrap flush

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]

Message-ID: <5f417d30-34a7-8da1-0ad5-33bd750582c7@I-love.SAKURA.ne.jp>
Date:   Fri, 20 May 2022 18:51:12 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Tejun Heo <tj@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

On 2022/05/20 17:01, Tejun Heo wrote:
>> +/*
>> + * 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.");		\
> 
> It kinda bothers me that this causes a build failure. It'd be better if we
> can trigger #warning instead. I'm not sure whether there'd be a clean way to
> do it tho. Maybe just textual matching would provide similar coverage? How
> did you test this?

This does not cause a build failure, for this wrapping happens only if
flush_workqueue() appears between "#define flush_workqueue(wq)" and
"#undef flush_workqueue". Only flush_scheduled_work() in include/linux/workqueue.h
calls flush_workqueue(system_wq), and flush_scheduled_work() is defined
before the "#define flush_workqueue(wq)" is defined.

And use of #warning directive breaks building with -Werror option.

> 
>>  #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)
> 
> Maybe rename the function to __flush_workqueue() instead of undef'ing the
> macro?

I prefer not adding __ prefix, for flush_workqueue() is meant as a public function.
For easier life of kernel message parsers, I don't feel reason to dare to rename.

But if you still prefer renaming, I will change flush_workqueue() as an inline function
in include/linux/workqueue.h which calls __flush_workqueue() in kernel/workqueue.c.