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: <1a1634ac-db0e-a44c-b286-a3aba55ad695@I-love.SAKURA.ne.jp>
Date:   Fri, 20 May 2022 20:43:41 +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 20:11, Tejun Heo wrote:
>>> 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.
> 
> What I mean is that if there's a file which didn't get tested or another
> pull request which raced and that thing flushes one of the system_wq's,
> it'll trigger a build error instead of a warning, which is a bit of an
> overkill.

All flush_workqueue(system_*_wq) users are gone in linux-next.git, and this patch
is for preventing new flush_workqueue(system_*_wq) users from coming in.

Therefore, triggering a build error (by sending this patch to linux.git right
before 5.19-rc1 in order to make sure that developers will not use
flush_workqueue(system_*_wq) again) is what this patch is for.

We will also remove flush_scheduled_work() after
all flush_scheduled_work() users are gone.

> 
>> And use of #warning directive breaks building with -Werror option.
> 
> If the user wants to fail build on warnings, sure. That's different from
> kernel failing to build in a way which may require non-trivial changes to
> fix.

How can #warning directive be utilized inside #define or inline function, for
we can't do like

  #define flush_workqueue(wq)						\
  #if wq == "system_wq"                                                \
  #warning Please avoid flushing system_wq.                            \
  #endif                                                               \
  __flush_workqueue(wq)

or

  static inline void flush_workqueue(struct workqueue_struct *wq)
  {
  #if wq == "system_wq"
  #warning Please avoid flushing system_wq.
  #endif
  	__flush_workqueue(wq);
  }

. We can use BUiLD_BUG_ON() but I don't think we can use #warning directive.

> 
>>> 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.
> 
> You mean the WARN_ON messages? Given how they never trigger, I doubt there's
> much to break. Maybe some kprobe users? But they can survive.

WARN_ON() by passing system-wide workqueues should not happen.
But backtrace of a warning message while inside __flush_workqueue() will be
still possible.