Re: [PATCH] loop: add sync_blockdev() in __loop_clr_fd() to prevent UAF

[Ming Lei <tom.leiming@gmail.com>]

On Fri, May 22, 2026 at 03:54:54PM +0900, Tetsuo Handa wrote:
> On 2026/05/22 12:45, Ming Lei wrote:
> > On Fri, May 22, 2026 at 12:28:34PM +0900, Tetsuo Handa wrote:
> >> On 2026/05/22 11:54, Ming Lei wrote:
> >>> Fix this by adding sync_blockdev() in __loop_clr_fd() to flush all
> >>> pending writeback I/O before clearing lo->lo_backing_file. Since the
> >>> loop disk is already closed at this point, no new I/O can be submitted
> >>> — only writeback remains.
> >>
> >> Why can you assume that synchronize_rcu() + drain_workqueue(lo->workqueue)
> >> is unnecessary? Since we don't know exact commit which is causing this
> >> problem, we don't know what has changed.
> > 
> > When sync_blockdev() returns, there can't be any inflight IO:
> > 
> > - no one can open this loop disk
> > - no dirty page cache any more
> > 
> > So why do you want to add rcu/drain_workqueue?
> 
> The fact that __loop_clr_fd() is called before lo_rw_aio() completes suggests that
> some works queued before __loop_clr_fd() is called have not been completed.
> 
> I don't know what sync_blockdev() guarantees, but even if sync_blockdev() can guarantee
> both "loop_queue_rq() is no longer running" and "all pending AIO requests issued by
> lo_rw_aio() have been completed", I think that sync_blockdev() cannot determine whether
> lo->lo_device has pending work or not.

Please do take a look at the commit log of this one and commit 1fe0b1acb14d

> 
> Therefore, I guess that at least drain_workqueue() is needed before sync_blockdev().
> If sync_blockdev() cannot guarantee "loop_queue_rq() is no longer running",
> I guess that both synchronize_rcu() + drain_workqueue() are needed before sync_blockdev().

Not at all.

What matters is that resources cleared in __loop_clr_fd() are not used any
more, and the resource is only accessed when handling io command from both
submission and completion path.

Again:

When the loop disk is closed, no foreground IO is possible, meantime
no one can open it because of ->open_mutex. So the only remained IO are
writeback, sync_blockdev() writes out all dirty pages and waits until
all are done, then there can't be any new IO, and it is safe to clear
these resources.

work function may still be run, but command list includes nothing, so
resources to be cleared won't be touched.

> 
> (And I don't know whether sync_blockdev() is appropriate because we don't know what change
> made this problem visible. We didn't hit this problem until Linux 7.0.)
> 
> > 
> > Fixes: 1fe0b1acb14d ("loop: only freeze the queue in __loop_clr_fd when needed")
> 
> The Fixes: for these reports should be the commit which made this problem visible.
> Or, this patch will be needlessly backported to stable kernels.

Will add it in V2.


Thanks,
Ming