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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Sep 2022 11:06:19 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
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>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with
 flush_work().

On Mon, Sep 26, 2022 at 07:28:37PM +0900, Tetsuo Handa wrote:
> On 2022/09/26 2:27, Andrew Morton wrote:
> > On Sat, 24 Sep 2022 14:11:25 +0900 Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp> wrote:
> > 
> >> Like commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a
> >> macro") says, flush_scheduled_work() is dangerous and will be forbidden.
> >> We are on the way for removing all flush_scheduled_work() callers from
> >> the kernel, and this patch is for removing flush_scheduled_work() call
> >>  from tsi721 driver.
> >>
> >> Since "struct tsi721_device" is per a device struct, I assume that
> >> tsi721_remove() needs to wait for only two works associated with that
> >> device. Therefore, wait for only these works using flush_work().
> >>
> >> --- a/drivers/rapidio/devices/tsi721.c
> >> +++ b/drivers/rapidio/devices/tsi721.c
> >> @@ -2941,7 +2941,8 @@ static void tsi721_remove(struct pci_dev *pdev)
> >>  
> >>  	tsi721_disable_ints(priv);
> >>  	tsi721_free_irq(priv);
> >> -	flush_scheduled_work();
> >> +	flush_work(&priv->idb_work);
> >> +	flush_work(&priv->pw_work);
> >>  	rio_unregister_mport(&priv->mport);
> > 
> > Why not use cancel_work[_sync](), as the flush_scheduled_work() comment
> > recommends?
> > 
> 
> 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.

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.

Alan Stern