linux-kernel - Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with 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] [day] [month] [year] [list]

Date:   Mon, 10 Oct 2022 19:16:07 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Tejun Heo <tj@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Matt Porter <mporter@...nel.crashing.org>,
        Alexandre Bounine <alex.bou9@...il.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Arnd Bergmann <arnd@...db.de>,
        Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with
 flush_work().

On 2022/09/27 7:13, Tetsuo Handa wrote:
> On 2022/09/27 0:06, Alan Stern wrote:
>>> Alan Stern suggested to use cancel_work_sync() in
>>> commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()")
>>> and Tejun Heo suggested to use flush_work() in
>>> https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org .
>>>
>>> Is there some reason to prefer one over the other?
>>> I think that user-visible results between flush_work() and cancel_work_sync()
>>> are the same because both wait until work completes.
>>
>> No, you haven't got it quite right.  flush_work() waits until the work 
>> completes, but cancel_work_sync() first tries to cancel the work item.  
>> It then waits until the work item is either cancelled or completed.
> 
> I know there is a difference if the cancellation was successful.
> But unless cancel_work_sync() is called immediately after schedule_work(),
> that work likely (e.g. 99%+) already started running or already completed.
> 
>>
>> If the cancellation is successful (i.e., it happens before the work item 
>> starts to run) then the call will return at that time and the work item 
>> will never run -- hence it will never complete.
> 
> A difficult to judge thing is whether the owner/maintainer of that code wants
> that work completed or cancelled.
> Unlike e.g. https://lkml.kernel.org/r/Yy3byxFrfAfQL9xK@intel.com ,
> tsi721_remove() does not say whether pending works should run.
> 

It seems that Tejun is too busy to respond.

Although it is a bit worrisome that tsi721_db_dpc() unconditionally re-enables
IDB interrupts when tsi721_remove() wants to disable all device interrupts (I
don't know behavior/specification of tsi721 hardware), I think it is OK to go
with flush_work() (as with other patches which removed flush_scheduled_work()
usage) because flush_work() matches the behavior of flush_scheduled_work().

It is maintainer's responsibility to fix if re-enabling IDB interrupts is not safe,
using a trick explained in e.g. commit a91b750fd6629354 ("net: rds: don't hold sock
lock when cancelling work from rds_tcp_reset_callbacks()").