syzbot is reporting NULL pointer dereference in lo_rw_aio() [1][2].
An analysis by the Gemini AI collaborator [3] considers that this problem
is caused by a timing shift primarily exposed by commit 65565ca5f99b
("block: unify the synchronous bi_end_io callbacks"), along with helper
refactorings like commit 92c3737a2473 ("block: add a bio_submit_or_kill
helper").

But due to difficulty of reproducing this race, discussion about what is
happening and how to fix this problem is stalling. Also, we haven't
identified how many filesystems are subjected to this problem.

Therefore, introduce a grace period for flushing pending I/O requests
(which should be a good thing from the perspective of defensive
programming) so that we won't hit NULL pointer dereference problem.

However, calling drain_workqueue() from __loop_clr_fd() with
disk->open_mutex held causes lockdep warnings. We need to flush pending
I/O requests without disk->open_mutex held. Therefore, defer
__loop_clr_fd() to WQ context, like commit 322c4293ecc5 ("loop: make
autoclear operation asynchronous") did.

The past attempt was reverted by commit bf23747ee053 ("loop: revert "make
autoclear operation asynchronous"") for two reasons:

  (1) Userspace might be expecting that fput() on the backing file is
      processed before lo_release() from close() returns to user mode.
      But a debug patch [4] suggested me that this teardown operation is
      racy regardless of whether disk->open_mutex is temporarily released
      or not, and therefore the xfs/259 breakage should be addressed on
      the xfstests side.

  (2) Lockdep reported circular locking dependency caused by flushing
      system-wide WQs. But we no longer need to worry that dependency
      because all in-tree users no longer flush system-wide WQs.

Therefore, let's retry deferring __loop_clr_fd() to WQ context again.

Link: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28 [1]
Link: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3 [2]
Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@I-love.SAKURA.ne.jp [3]
Link: https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp [4]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Assisted-by: Gemini-Pro
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Sashiko reviewed this patch, and did not find problems
( https://sashiko.dev/#/patchset/5803da44-97c7-440e-a06b-d3cf4afff3c8%40I-love.SAKURA.ne.jp ).
Can we try this approach?


 drivers/block/loop.c | 74 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..e2703f0ee75d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -75,6 +75,7 @@ struct loop_device {
 	struct gendisk		*lo_disk;
 	struct mutex		lo_mutex;
 	bool			idr_visible;
+	struct work_struct	lo_clr_work;
 };
 
 struct loop_cmd {
@@ -1134,13 +1135,35 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	return error;
 }
 
-static void __loop_clr_fd(struct loop_device *lo)
+static void __loop_clr_fd(struct work_struct *work)
 {
+	struct loop_device *lo = container_of(work, struct loop_device, lo_clr_work);
+	struct gendisk *disk = lo->lo_disk;
 	struct queue_limits lim;
 	struct file *filp;
 	gfp_t gfp = lo->old_gfp_mask;
 	int err;
 
+	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
+	/*
+	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
+	 * wait for already started loop_queue_rq() to complete.
+	 */
+	synchronize_rcu();
+	/*
+	 * Now that no more works are scheduled by loop_queue_rq(),
+	 * wait for already scheduled works to complete.
+	 */
+	drain_workqueue(lo->workqueue);
+	/*
+	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
+	 * wait for already started AIO to complete.
+	 */
+	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+
+	/* Step 2: Perform remaining cleanup, with open_mutex held. */
+	mutex_lock(&disk->open_mutex);
+
 	spin_lock_irq(&lo->lo_lock);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
@@ -1151,12 +1174,7 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_sizelimit = 0;
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 
-	/*
-	 * Reset the block size to the default.
-	 *
-	 * No queue freezing needed because this is called from the final
-	 * ->release call only, so there can't be any outstanding I/O.
-	 */
+	/* Reset the block size to the default. */
 	lim = queue_limits_start_update(lo->lo_queue);
 	lim.logical_block_size = SECTOR_SIZE;
 	lim.physical_block_size = SECTOR_SIZE;
@@ -1168,8 +1186,6 @@ static void __loop_clr_fd(struct loop_device *lo)
 	/* let user-space know about this change */
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
-	/* This is safe: open() is still holding a reference. */
-	module_put(THIS_MODULE);
 
 	disk_force_media_change(lo->lo_disk);
 
@@ -1199,12 +1215,24 @@ static void __loop_clr_fd(struct loop_device *lo)
 	WRITE_ONCE(lo->lo_state, Lo_unbound);
 	mutex_unlock(&lo->lo_mutex);
 
+	/* Step 3: Drop refcounts, without open_mutex held. */
+	mutex_unlock(&disk->open_mutex);
+
+	fput(filp);
+
 	/*
-	 * Need not hold lo_mutex to fput backing file. Calling fput holding
-	 * lo_mutex triggers a circular lock dependency possibility warning as
-	 * fput can take open_mutex which is usually taken before lo_mutex.
+	 * Drop all references that would have been dropped as soon as
+	 * returning from lo_release() and releasing disk->open_mutex.
 	 */
-	fput(filp);
+	module_put(disk->fops->owner);
+	put_device(disk_to_dev(disk));
+
+	/*
+	 * This is safe: flush_work() from loop_remove() from loop_exit() waits
+	 * until this function returns; effectively dropping the final module
+	 * references synchronously.
+	 */
+	module_put(THIS_MODULE);
 }
 
 static int loop_clr_fd(struct loop_device *lo)
@@ -1769,8 +1797,20 @@ static void lo_release(struct gendisk *disk)
 	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
 
-	if (need_clear)
-		__loop_clr_fd(lo);
+	/*
+	 * In order to flush pending I/O requests before clearing the backing
+	 * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
+	 * guarantees that lo_open() will fail with -ENXIO.
+	 */
+	if (need_clear) {
+		/*
+		 * Grab all references that will be dropped as soon as
+		 * returning from lo_release() and releasing disk->open_mutex.
+		 */
+		get_device(disk_to_dev(disk));
+		__module_get(disk->fops->owner);
+		queue_work(system_long_wq, &lo->lo_clr_work);
+	}
 }
 
 static void lo_free_disk(struct gendisk *disk)
@@ -2034,6 +2074,7 @@ static int loop_add(int i)
 	lo = kzalloc_obj(*lo);
 	if (!lo)
 		goto out;
+	INIT_WORK(&lo->lo_clr_work, __loop_clr_fd);
 	lo->worker_tree = RB_ROOT;
 	INIT_LIST_HEAD(&lo->idle_worker_list);
 	timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
@@ -2138,6 +2179,9 @@ static int loop_add(int i)
 
 static void loop_remove(struct loop_device *lo)
 {
+	/* Wait for __loop_clr_fd() to complete. */
+	flush_work(&lo->lo_clr_work);
+
 	/* Make this loop device unreachable from pathname. */
 	del_gendisk(lo->lo_disk);
 	blk_mq_free_tag_set(&lo->tag_set);
-- 
2.55.0

On 8/28/26 8:53 AM, Tetsuo Handa wrote:
> +	 * wait for already started loop_queue_rq() to complete.
> +	 */
> +	synchronize_rcu();

Calling synchronize_rcu() to wait for ongoing loop_queue_rq() calls to
complete won't work if anyone would set BLK_MQ_F_BLOCKING for the
request queues created by the loop driver. Please use the block-layer
APIs instead of open-coding these. I'm referring to
blk_mq_quiesce_queue() and blk_mq_wait_quiesce_done().

Freezing the request queue must happen before waiting for ongoing
loop_queue_rq() calls to finish.

Calling synchronize_rcu() does not prevent new I/O to be submitted. What
prevents io_uring to submit more I/O asynchronously, e.g. if a file
descriptor that refers to a loop device instance has been registered in
the fixed-file table?

> +	/*
> +	 * Now that no more works are scheduled by loop_queue_rq(),
> +	 * wait for already scheduled works to complete.
> +	 */
> +	drain_workqueue(lo->workqueue);
> +	/*
> +	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
> +	 * wait for already started AIO to complete.
> +	 */
> +	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));

Freezing the request queue must happen before lo->workqueue is drained.

> +	/* Step 2: Perform remaining cleanup, with open_mutex held. */
> +	mutex_lock(&disk->open_mutex);

After having obtained disk->open_mutex, lease add something like the
following: WARN_ON_ONCE(lo->lo_state == Lo_bound). Even if this
condition can't be triggered today, this may help with detecting bugs in
future loop driver changes.

> @@ -1168,8 +1186,6 @@ static void __loop_clr_fd(struct loop_device *lo)
>   	/* let user-space know about this change */
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   	mapping_set_gfp_mask(filp->f_mapping, gfp);
> -	/* This is safe: open() is still holding a reference. */
> -	module_put(THIS_MODULE);
>   
>   	disk_force_media_change(lo->lo_disk); 

I don't think that it's acceptable to invoke __loop_clr_fd()
asynchronously in its entirety. I think at least the following code
should be executed synchronously from lo_release():

	loop_sysfs_exit(lo);

	mutex_lock(&lo->lo_mutex);
	WRITE_ONCE(lo->lo_state, Lo_unbound);
	mutex_unlock(&lo->lo_mutex);

> @@ -1769,8 +1797,20 @@ static void lo_release(struct gendisk *disk)
>   	need_clear = (lo->lo_state == Lo_rundown);
>   	mutex_unlock(&lo->lo_mutex);
>   
> -	if (need_clear)
> -		__loop_clr_fd(lo);
> +	/*
> +	 * In order to flush pending I/O requests before clearing the backing
> +	 * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
> +	 * guarantees that lo_open() will fail with -ENXIO.
> +	 */
> +	if (need_clear) {
> +		/*
> +		 * Grab all references that will be dropped as soon as
> +		 * returning from lo_release() and releasing disk->open_mutex.
> +		 */
> +		get_device(disk_to_dev(disk));
> +		__module_get(disk->fops->owner);
> +		queue_work(system_long_wq, &lo->lo_clr_work);
> +	}
>   }

Please convert the above code to the "early return" style that is used
elsewhere in the kernel.

Why system_long_wq instead of lo->workqueue?

Thanks,

Bart.

On 2026/08/29 1:29, Bart Van Assche wrote:
> On 8/28/26 8:53 AM, Tetsuo Handa wrote:
>> +     * wait for already started loop_queue_rq() to complete.
>> +     */
>> +    synchronize_rcu();
> 
> Calling synchronize_rcu() to wait for ongoing loop_queue_rq() calls to
> complete won't work if anyone would set BLK_MQ_F_BLOCKING for the
> request queues created by the loop driver.

Excuse me, but this ordering is correct (reviewed by Gemini and sashiko).

>                                            Please use the block-layer
> APIs instead of open-coding these. I'm referring to
> blk_mq_quiesce_queue() and blk_mq_wait_quiesce_done().
> 
> Freezing the request queue must happen before waiting for ongoing
> loop_queue_rq() calls to finish.

I and Gemini cannot catch what you want to say here.

> Calling synchronize_rcu() does not prevent new I/O to be submitted. What
> prevents io_uring to submit more I/O asynchronously, e.g. if a file
> descriptor that refers to a loop device instance has been registered in
> the fixed-file table?

Calling synchronize_rcu() makes sure that no more queue_work() calls are
made from loop_queue_work() from loop_queue_rq(). Since loop_queue_rq() is
called with RCU read lock, subsequent loop_queue_rq() calls which are made
after synchronize_rcu() returned shall see lo->lo_state != Lo_bound and
return with BLK_STS_IOERR.

> 
>> +    /*
>> +     * Now that no more works are scheduled by loop_queue_rq(),
>> +     * wait for already scheduled works to complete.
>> +     */
>> +    drain_workqueue(lo->workqueue);
>> +    /*
>> +     * Now that no more AIO requests are scheduled by lo_rw_aio(),
>> +     * wait for already started AIO to complete.
>> +     */
>> +    blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
> 
> Freezing the request queue must happen before lo->workqueue is drained.

Again, I and Gemini cannot catch what you want to say here.

Calling drain_workqueue() after synchronize_rcu() makes sure that no more
loop_handle_cmd() calls are made from loop_process_work() from loop_workfn()
and loop_rootcg_workfn().

Calling blk_mq_freeze_queue() after drain_workqueue() after synchronize_rcu()
does wait for completion of all pending I/O requests which has been scheduled
via loop_queue_rq(), by waiting for q_usage_counter to reach 0. Also, this
synchronize_rcu() => drain_workqueue() => blk_mq_freeze_queue() ordering
guarantees that q_usage_counter won't be incremented again after it once
reached 0, due to the lo->lo_state != Lo_bound check in loop_queue_rq().
This makes it possible to call blk_mq_unfreeze_queue() immediately after
blk_mq_freeze_queue().

> 
>> +    /* Step 2: Perform remaining cleanup, with open_mutex held. */
>> +    mutex_lock(&disk->open_mutex);
> 
> After having obtained disk->open_mutex, lease add something like the
> following: WARN_ON_ONCE(lo->lo_state == Lo_bound). Even if this
> condition can't be triggered today, this may help with detecting bugs in
> future loop driver changes.

Did you mean WARN_ON_ONCE(lo->lo_state != Lo_rundown) ?

> 
>> @@ -1168,8 +1186,6 @@ static void __loop_clr_fd(struct loop_device *lo)
>>       /* let user-space know about this change */
>>       kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>>       mapping_set_gfp_mask(filp->f_mapping, gfp);
>> -    /* This is safe: open() is still holding a reference. */
>> -    module_put(THIS_MODULE);
>>         disk_force_media_change(lo->lo_disk); 
> 
> I don't think that it's acceptable to invoke __loop_clr_fd()
> asynchronously in its entirety. I think at least the following code
> should be executed synchronously from lo_release():
> 
>     loop_sysfs_exit(lo);
> 
>     mutex_lock(&lo->lo_mutex);
>     WRITE_ONCE(lo->lo_state, Lo_unbound);
>     mutex_unlock(&lo->lo_mutex);
> 

Doing so breaks the whole protection provided by the Lo_rundown state. As soon
as lo->lo_state becomes Lo_unbound, lo_open() will succeed and loop_configure()
will start changing an lo object before WQ context cleans up that lo object.

>> @@ -1769,8 +1797,20 @@ static void lo_release(struct gendisk *disk)
>>       need_clear = (lo->lo_state == Lo_rundown);
>>       mutex_unlock(&lo->lo_mutex);
>>   -    if (need_clear)
>> -        __loop_clr_fd(lo);
>> +    /*
>> +     * In order to flush pending I/O requests before clearing the backing
>> +     * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
>> +     * guarantees that lo_open() will fail with -ENXIO.
>> +     */
>> +    if (need_clear) {
>> +        /*
>> +         * Grab all references that will be dropped as soon as
>> +         * returning from lo_release() and releasing disk->open_mutex.
>> +         */
>> +        get_device(disk_to_dev(disk));
>> +        __module_get(disk->fops->owner);
>> +        queue_work(system_long_wq, &lo->lo_clr_work);
>> +    }
>>   }
> 
> Please convert the above code to the "early return" style that is used
> elsewhere in the kernel.

That is OK. But after you agreed that my patch works as expected.

> 
> Why system_long_wq instead of lo->workqueue?

That is a deadlock. We can't flush a work in lo->workqueue from inside
WQ callback function where that work is associated with.

On 8/28/26 10:17 PM, Tetsuo Handa wrote:
> Calling synchronize_rcu() makes sure that no more queue_work() calls are
> made from loop_queue_work() from loop_queue_rq(). Since loop_queue_rq() is
> called with RCU read lock, subsequent loop_queue_rq() calls which are made
> after synchronize_rcu() returned shall see lo->lo_state != Lo_bound and
> return with BLK_STS_IOERR.

Please add a comment above the synchronize_rcu() call that explains the
purpose of that call.

> That is OK. But after you agreed that my patch works as expected.
I appreciate your work and agree that this patch takes the right
direction. But I would appreciate it if you would take another look at
my review comments.

Thanks,

Bart.

syzbot is reporting NULL pointer dereference in lo_rw_aio() [1][2].
An analysis by the Gemini AI collaborator [3] considers that this problem
is caused by a timing shift primarily exposed by commit 65565ca5f99b
("block: unify the synchronous bi_end_io callbacks"), along with helper
refactorings like commit 92c3737a2473 ("block: add a bio_submit_or_kill
helper").

But due to difficulty of reproducing this race, discussion about what is
happening and how to fix this problem is stalling. Also, we haven't
identified how many filesystems are subjected to this problem.

Therefore, introduce a grace period for flushing outstanding I/O
(which should be a good thing from the perspective of defensive
programming) so that we won't hit NULL pointer dereference problem.

However, calling drain_workqueue() from __loop_clr_fd() with
disk->open_mutex held causes lockdep warnings. We need to flush
outstanding I/O without disk->open_mutex held. Therefore, defer
__loop_clr_fd() to WQ context, like commit 322c4293ecc5 ("loop: make
autoclear operation asynchronous") did.

The past attempt was reverted by commit bf23747ee053 ("loop: revert "make
autoclear operation asynchronous"") for two reasons:

  (1) Userspace might be expecting that fput() on the backing file is
      processed before lo_release() from close() returns to user mode.
      But a debug patch [4] suggested me that this teardown operation is
      racy regardless of whether disk->open_mutex is temporarily released
      or not, and therefore the xfs/259 breakage should be addressed on
      the xfstests side.

  (2) Lockdep reported circular locking dependency caused by flushing
      system-wide WQs. But we no longer need to worry that dependency
      because all in-tree users no longer flush system-wide WQs.

Therefore, let's retry deferring __loop_clr_fd() to WQ context.

Link: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28 [1]
Link: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3 [2]
Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@I-love.SAKURA.ne.jp [3]
Link: https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp [4]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Assisted-by: Gemini-Pro
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
What an AI coding assistant based on Gemini told Bart about the v7.1 patch:

  1. Userspace ABI / Teardown Regression (Asynchronous Autoclear)

  The Regression: When a loop device configured with LO_FLAGS_AUTOCLEAR is
  closed (such as during unmounting with umount -d or mount -o loop),
  userspace expects that close() synchronously tears down the device and
  releases the backing file (fput()) before returning to user mode.

  Prior History: This exact asynchronous deferral was merged in commit
  322c4293ecc5 ("loop: make autoclear operation asynchronous") and had to
  be reverted in commit bf23747ee053 ("loop: revert 'make autoclear
  operation asynchronous'") because standard filesystem unmount sequences
  (such as umount ext4_on_xfs; umount /xfs) broke with -EBUSY in xfstests
  (e.g., xfs/259).

  Kernel Policy: Handwaving this in the commit log ("the xfs/259 breakage
  should be addressed on the xfstests side") is not acceptable. Breaking
  synchronous teardown semantics violates the core Linux kernel rule: never
  break userspace. Real-world container engines, test suites, and system
  utilities rely on fput() having completed when lo_release() / close()
  returns.

What Bart says about the v6 patch which does synchronous teardown:

  Releasing and reacquiring disk->open_mutex from __loop_clr_fd() seems
  risky to me. There is plenty of code in block/bdev.c that assumes that
  disk->open_mutex is not released by lo_release().

Then, what direction can we go?

 drivers/block/loop.c | 74 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..516118b3a16c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -75,6 +75,7 @@ struct loop_device {
 	struct gendisk		*lo_disk;
 	struct mutex		lo_mutex;
 	bool			idr_visible;
+	struct work_struct	lo_clr_work;
 };
 
 struct loop_cmd {
@@ -1134,13 +1135,42 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	return error;
 }
 
-static void __loop_clr_fd(struct loop_device *lo)
+static void __loop_clr_fd(struct work_struct *work)
 {
+	struct loop_device *lo = container_of(work, struct loop_device, lo_clr_work);
+	struct gendisk *disk = lo->lo_disk;
 	struct queue_limits lim;
 	struct file *filp;
 	gfp_t gfp = lo->old_gfp_mask;
 	int err;
 
+	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
+	/*
+	 * Since loop_queue_rq() is called with RCU read lock, this synchronize_rcu()
+	 * makes sure that no more queue_work() calls are made from loop_queue_work()
+	 * from loop_queue_rq(). Subsequent loop_queue_rq() calls which are made after
+	 * this synchronize_rcu() returned shall see lo->lo_state != Lo_bound and
+	 * return with BLK_STS_IOERR.
+	 */
+	synchronize_rcu();
+	/*
+	 * This drain_workqueue() makes sure that no more loop_handle_cmd() calls are
+	 * made from loop_process_work() from loop_workfn()/loop_rootcg_workfn().
+	 */
+	drain_workqueue(lo->workqueue);
+	/*
+	 * This blk_mq_freeze_queue() waits for completion of all outstanding I/O
+	 * which has been scheduled via loop_queue_rq(), by waiting for q_usage_counter
+	 * to reach 0. Since the lo->lo_state != Lo_bound check in loop_queue_rq()
+	 * guarantees that no more new I/O requests are made, we can call
+	 * blk_mq_unfreeze_queue() immediately after blk_mq_freeze_queue() returns.
+	 */
+	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+
+	/* Step 2: Perform remaining cleanup, with open_mutex held. */
+	mutex_lock(&disk->open_mutex);
+	WARN_ON_ONCE(lo->lo_state != Lo_rundown);
+
 	spin_lock_irq(&lo->lo_lock);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
@@ -1151,12 +1181,7 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_sizelimit = 0;
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 
-	/*
-	 * Reset the block size to the default.
-	 *
-	 * No queue freezing needed because this is called from the final
-	 * ->release call only, so there can't be any outstanding I/O.
-	 */
+	/* Reset the block size to the default. */
 	lim = queue_limits_start_update(lo->lo_queue);
 	lim.logical_block_size = SECTOR_SIZE;
 	lim.physical_block_size = SECTOR_SIZE;
@@ -1168,8 +1193,6 @@ static void __loop_clr_fd(struct loop_device *lo)
 	/* let user-space know about this change */
 	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
-	/* This is safe: open() is still holding a reference. */
-	module_put(THIS_MODULE);
 
 	disk_force_media_change(lo->lo_disk);
 
@@ -1199,11 +1222,18 @@ static void __loop_clr_fd(struct loop_device *lo)
 	WRITE_ONCE(lo->lo_state, Lo_unbound);
 	mutex_unlock(&lo->lo_mutex);
 
+	/* Step 3: Drop refcounts, without open_mutex held. */
+	mutex_unlock(&disk->open_mutex);
+
+	put_device(disk_to_dev(disk));
+
 	/*
-	 * Need not hold lo_mutex to fput backing file. Calling fput holding
-	 * lo_mutex triggers a circular lock dependency possibility warning as
-	 * fput can take open_mutex which is usually taken before lo_mutex.
+	 * This is safe: flush_work() from loop_remove() from loop_exit() waits
+	 * until this function returns; effectively dropping the final module
+	 * references synchronously.
 	 */
+	module_put(THIS_MODULE);
+
 	fput(filp);
 }
 
@@ -1769,8 +1799,20 @@ static void lo_release(struct gendisk *disk)
 	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
 
-	if (need_clear)
-		__loop_clr_fd(lo);
+	if (!need_clear)
+		return;
+	/*
+	 * In order to flush outstanding I/O before clearing the backing
+	 * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
+	 * guarantees that lo_open() will fail with -ENXIO.
+	 *
+	 * Grab disk reference which will be dropped as soon as
+	 * returning from lo_release() and releasing disk->open_mutex.
+	 * We don't need to grab disk->fops->owner reference because
+	 * we are holding one obtained by loop_configure().
+	 */
+	get_device(disk_to_dev(disk));
+	queue_work(system_long_wq, &lo->lo_clr_work);
 }
 
 static void lo_free_disk(struct gendisk *disk)
@@ -2034,6 +2076,7 @@ static int loop_add(int i)
 	lo = kzalloc_obj(*lo);
 	if (!lo)
 		goto out;
+	INIT_WORK(&lo->lo_clr_work, __loop_clr_fd);
 	lo->worker_tree = RB_ROOT;
 	INIT_LIST_HEAD(&lo->idle_worker_list);
 	timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
@@ -2138,6 +2181,9 @@ static int loop_add(int i)
 
 static void loop_remove(struct loop_device *lo)
 {
+	/* Wait for __loop_clr_fd() to complete. */
+	flush_work(&lo->lo_clr_work);
+
 	/* Make this loop device unreachable from pathname. */
 	del_gendisk(lo->lo_disk);
 	blk_mq_free_tag_set(&lo->tag_set);
-- 
2.55.0

syzbot is reporting NULL pointer dereference in lo_rw_aio() [1][2].
An analysis by the Gemini AI collaborator [3] considers that this problem
is caused by a timing shift primarily exposed by commit 65565ca5f99b
("block: unify the synchronous bi_end_io callbacks"), along with helper
refactorings like commit 92c3737a2473 ("block: add a bio_submit_or_kill
helper").

But due to difficulty of reproducing this race, discussion about what is
happening and how to fix this problem is stalling. Also, we haven't
identified how many filesystems are subjected to this problem.

Therefore, introduce a grace period for flushing outstanding I/O
(which should be a good thing from the perspective of defensive
programming) so that we won't hit NULL pointer dereference problem.

However, calling drain_workqueue() from __loop_clr_fd() with
disk->open_mutex held causes lockdep warnings. We need to flush
outstanding I/O without disk->open_mutex held. But we can't use task work
context, for there is no way to reliably wait for completion of a task work
function inside a loadable module when module unloading code for that
loadable module has started. We need to use a callback function which is
embedded into a built-in module so that it can reliably wait for completion
of synchronous teardown for the loop driver module.

Therefore, add a dedicated callback for the loop module to the block core
layer, and invoke that callback immediately after disk->open_mutex is
released. It is possible that multiple threads invoke that callback
when a teardown work was scheduled because disk->open_mutex was already
released, but concurrently calling flush_work() in order to wait for
completion of an outstanding teardown work will be safe.

Link: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28 [1]
Link: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3 [2]
Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@I-love.SAKURA.ne.jp [3]
Link: https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp [4]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Assisted-by: Gemini-Pro
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
What this patch does is basically the same with the v6 patch. Can this be
a possible alternative for Bart's

  Releasing and reacquiring disk->open_mutex from __loop_clr_fd() seems
  risky to me. There is plenty of code in block/bdev.c that assumes that
  disk->open_mutex is not released by lo_release().

comment?

 block/bdev.c           |  2 ++
 drivers/block/loop.c   | 65 +++++++++++++++++++++++++++++++++---------
 include/linux/blkdev.h |  5 ++++
 3 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index cd8323083740..7ce5acaacf43 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1188,6 +1188,8 @@ void bdev_release(struct file *bdev_file)
 	else
 		blkdev_put_whole(bdev);
 	mutex_unlock(&disk->open_mutex);
+	if (bdev->bd_disk->fops->post_release)
+		bdev->bd_disk->fops->post_release(bdev->bd_disk);
 
 	module_put(disk->fops->owner);
 put_no_open:
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..620298978706 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -75,6 +75,7 @@ struct loop_device {
 	struct gendisk		*lo_disk;
 	struct mutex		lo_mutex;
 	bool			idr_visible;
+	struct work_struct	lo_clr_work;
 };
 
 struct loop_cmd {
@@ -1134,13 +1135,42 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 	return error;
 }
 
-static void __loop_clr_fd(struct loop_device *lo)
+static void __loop_clr_fd(struct work_struct *work)
 {
+	struct loop_device *lo = container_of(work, struct loop_device, lo_clr_work);
+	struct gendisk *disk = lo->lo_disk;
 	struct queue_limits lim;
 	struct file *filp;
 	gfp_t gfp = lo->old_gfp_mask;
 	int err;
 
+	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
+	/*
+	 * Since loop_queue_rq() is called with RCU read lock, this synchronize_rcu()
+	 * makes sure that no more queue_work() calls are made from loop_queue_work()
+	 * from loop_queue_rq(). Subsequent loop_queue_rq() calls which are made after
+	 * this synchronize_rcu() returned shall see lo->lo_state != Lo_bound and
+	 * return with BLK_STS_IOERR.
+	 */
+	synchronize_rcu();
+	/*
+	 * This drain_workqueue() makes sure that no more loop_handle_cmd() calls are
+	 * made from loop_process_work() from loop_workfn()/loop_rootcg_workfn().
+	 */
+	drain_workqueue(lo->workqueue);
+	/*
+	 * This blk_mq_freeze_queue() waits for completion of all outstanding I/O
+	 * which has been scheduled via loop_queue_rq(), by waiting for q_usage_counter
+	 * to reach 0. Since the lo->lo_state != Lo_bound check in loop_queue_rq()
+	 * guarantees that no more new I/O requests are made, we can call
+	 * blk_mq_unfreeze_queue() immediately after blk_mq_freeze_queue() returns.
+	 */
+	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+
+	/* Step 2: Perform remaining cleanup, with open_mutex held. */
+	mutex_lock(&disk->open_mutex);
+	WARN_ON_ONCE(lo->lo_state != Lo_rundown);
+
 	spin_lock_irq(&lo->lo_lock);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
@@ -1151,12 +1181,7 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_sizelimit = 0;
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
 
-	/*
-	 * Reset the block size to the default.
-	 *
-	 * No queue freezing needed because this is called from the final
-	 * ->release call only, so there can't be any outstanding I/O.
-	 */
+	/* Reset the block size to the default. */
 	lim = queue_limits_start_update(lo->lo_queue);
 	lim.logical_block_size = SECTOR_SIZE;
 	lim.physical_block_size = SECTOR_SIZE;
@@ -1199,11 +1224,9 @@ static void __loop_clr_fd(struct loop_device *lo)
 	WRITE_ONCE(lo->lo_state, Lo_unbound);
 	mutex_unlock(&lo->lo_mutex);
 
-	/*
-	 * Need not hold lo_mutex to fput backing file. Calling fput holding
-	 * lo_mutex triggers a circular lock dependency possibility warning as
-	 * fput can take open_mutex which is usually taken before lo_mutex.
-	 */
+	/* Step 3: Drop refcounts, without open_mutex held. */
+	mutex_unlock(&disk->open_mutex);
+
 	fput(filp);
 }
 
@@ -1769,8 +1792,22 @@ static void lo_release(struct gendisk *disk)
 	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
 
+	/*
+	 * In order to flush outstanding I/O (without open_mutex for deadlock
+	 * avoidance) before clearing the backing device, defer __loop_clr_fd()
+	 * to WQ context and let lo_post_release() wait for completion.
+	 * The Lo_rundown state guarantees that lo_open() will fail with -ENXIO.
+	 */
 	if (need_clear)
-		__loop_clr_fd(lo);
+		queue_work(system_long_wq, &lo->lo_clr_work);
+}
+
+static void lo_post_release(struct gendisk *disk)
+{
+	struct loop_device *lo = disk->private_data;
+
+	/* Wait for __loop_clr_fd() to complete. */
+	flush_work(&lo->lo_clr_work);
 }
 
 static void lo_free_disk(struct gendisk *disk)
@@ -1789,6 +1826,7 @@ static const struct block_device_operations lo_fops = {
 	.owner =	THIS_MODULE,
 	.open =         lo_open,
 	.release =	lo_release,
+	.post_release = lo_post_release,
 	.ioctl =	lo_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl =	lo_compat_ioctl,
@@ -2034,6 +2072,7 @@ static int loop_add(int i)
 	lo = kzalloc_obj(*lo);
 	if (!lo)
 		goto out;
+	INIT_WORK(&lo->lo_clr_work, __loop_clr_fd);
 	lo->worker_tree = RB_ROOT;
 	INIT_LIST_HEAD(&lo->idle_worker_list);
 	timer_setup(&lo->timer, loop_free_idle_workers_timer, TIMER_DEFERRABLE);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b..5172d6bdd9e7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1605,6 +1605,11 @@ struct block_device_operations {
 	 * driver.
 	 */
 	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
+	/*
+	 * Special callback for synchronous cleanup without open_mutex.
+	 * Needed by loop devices.
+	 */
+	void (*post_release)(struct gendisk *disk);
 };
 
 #ifdef CONFIG_COMPAT
-- 
2.55.0

Hi Tetsuo,

kernel test robot noticed the following build errors:

[auto build test ERROR on axboe/for-next]
[also build test ERROR on linus/master v7.3-rc1 next-20260904]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tetsuo-Handa/loop-Fix-NULL-pointer-dereference-in-lo_rw_aio/20260904-085046
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux.git for-next
patch link:    https://lore.kernel.org/r/dc2f1e00-5e10-4e02-9415-c6ecb2cbc6b3%40I-love.SAKURA.ne.jp
patch subject: [PATCH v8] loop: Fix NULL pointer dereference in lo_rw_aio()
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20260905/202609052240.ovdWUOyj-lkp@intel.com/config)
compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project 0edd1b088cc36b4faee80358c925a91e16006258)
rustc: rustc 1.96.0 (ac68faa20 2026-05-25)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260905/202609052240.ovdWUOyj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202609052240.ovdWUOyj-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0063]: missing field `post_release` in initializer of `block_device_operations`
   --> rust/kernel/block/mq/gen_disk.rs:128:58
   |
   128 |         const TABLE: bindings::block_device_operations = bindings::block_device_operations {
   |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `post_release`

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Fri, 4 Sep 2026 08:50:46 +0900 Tetsuo Handa wrote:
> syzbot is reporting NULL pointer dereference in lo_rw_aio() [1][2].
> An analysis by the Gemini AI collaborator [3] considers that this problem
> is caused by a timing shift primarily exposed by commit 65565ca5f99b
> ("block: unify the synchronous bi_end_io callbacks"), along with helper
> refactorings like commit 92c3737a2473 ("block: add a bio_submit_or_kill
> helper").
> 
> But due to difficulty of reproducing this race, discussion about what is
> happening and how to fix this problem is stalling. Also, we haven't
> identified how many filesystems are subjected to this problem.
> 
> Therefore, introduce a grace period for flushing outstanding I/O
> (which should be a good thing from the perspective of defensive
> programming) so that we won't hit NULL pointer dereference problem.
> 
> However, calling drain_workqueue() from __loop_clr_fd() with
> disk->open_mutex held causes lockdep warnings. We need to flush
> outstanding I/O without disk->open_mutex held. But we can't use task work
> context, for there is no way to reliably wait for completion of a task work
> function inside a loadable module when module unloading code for that
> loadable module has started. We need to use a callback function which is
> embedded into a built-in module so that it can reliably wait for completion
> of synchronous teardown for the loop driver module.
> 
> Therefore, add a dedicated callback for the loop module to the block core
> layer, and invoke that callback immediately after disk->open_mutex is
> released. It is possible that multiple threads invoke that callback
> when a teardown work was scheduled because disk->open_mutex was already
> released, but concurrently calling flush_work() in order to wait for
> completion of an outstanding teardown work will be safe.
> 
> Link: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28 [1]
> Link: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3 [2]
> Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@I-love.SAKURA.ne.jp [3]
> Link: https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp [4]
> Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
> Assisted-by: Gemini-Pro
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> What this patch does is basically the same with the v6 patch. Can this be
> a possible alternative for Bart's
> 
>   Releasing and reacquiring disk->open_mutex from __loop_clr_fd() seems
>   risky to me. There is plenty of code in block/bdev.c that assumes that
>   disk->open_mutex is not released by lo_release().
> 
> comment?
> 
>  block/bdev.c           |  2 ++
>  drivers/block/loop.c   | 65 +++++++++++++++++++++++++++++++++---------
>  include/linux/blkdev.h |  5 ++++
>  3 files changed, 59 insertions(+), 13 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index cd8323083740..7ce5acaacf43 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -1188,6 +1188,8 @@ void bdev_release(struct file *bdev_file)
>  	else
>  		blkdev_put_whole(bdev);
>  	mutex_unlock(&disk->open_mutex);
> +	if (bdev->bd_disk->fops->post_release)
> +		bdev->bd_disk->fops->post_release(bdev->bd_disk);
>  
A great leap forward, I like this.

> +
> +static void lo_post_release(struct gendisk *disk)
> +{
> +	struct loop_device *lo = disk->private_data;
> +
> +	/* Wait for __loop_clr_fd() to complete. */
> +	flush_work(&lo->lo_clr_work);
>  }

On 2026/08/29 14:17, Tetsuo Handa wrote:
>                                                                 Also, this
> synchronize_rcu() => drain_workqueue() => blk_mq_freeze_queue() ordering
> guarantees that q_usage_counter won't be incremented again after it once
> reached 0, due to the lo->lo_state != Lo_bound check in loop_queue_rq().

Well, this part was inaccurate. Since q_usage_counter is incremented before
loop_queue_rq() is called, it is possible that q_usage_counter itself can be
incremented even after synchronize_rcu() => drain_workqueue() => blk_mq_freeze_queue()
sequence.

But what makes this ordering safe are

  (1) since we are in lo_release() with disk_openers(disk) == 0, the activity of
      incrementing/decrementing q_usage_counter (incremented before loop_queue_rq()
      is called, and decremented after loop_queue_rq() returned BLK_STS_IOERR)) will
      cease shortly

  (2) since there is no pending work in lo->workqueue, no I/O will be made to
      backing file

.

From: Tao Cui <cuitao@kylinos.cn>

Hi Tetsuo, Bart,

> +	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
> +	/*
> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
> +	 * wait for already started loop_queue_rq() to complete.
> +	 */
> +	synchronize_rcu();

Your reply to Bart says loop_queue_rq() is called with RCU read
lock, but I don't see one.  The two call sites of ->queue_rq() are
blk_mq_dispatch_rq_list() and __blk_mq_issue_directly(), and
neither is wrapped in rcu_read_lock().  Direct issue from
blk_mq_submit_bio() runs in process context with no RCU read-side
critical section, so synchronize_rcu() does not wait for a
loop_queue_rq() that is already running there.  Only the softirq
dispatch path is an implicit RCU reader.

The window is still closed by the steps below, so this is not a
correctness bug, but the synchronize_rcu() is not doing what the
comment claims.  blk_mq_quiesce_queue() +
blk_mq_wait_quiesce_done(), as Bart suggested, would express the
intent directly.

> +	/*
> +	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
> +	 * wait for already started AIO to complete.
> +	 */
> +	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));

About this step, in your follow-up you wrote:

>   (1) since we are in lo_release() with disk_openers(disk) == 0, the activity of
>       incrementing/decrementing q_usage_counter (incremented before loop_queue_rq()
>       is called, and decremented after loop_queue_rq() returned BLK_STS_IOERR)) will
>       cease shortly

The decrement timing here is only true for the error path.  For
BLK_STS_OK the reference is held until the request is freed, which
is what makes blk_mq_freeze_queue() wait for requests that already
passed the state check, including the loop workqueue worker that
completes them.  That is the property step 1 relies on, and it is
worth stating in the comment.

What is still open is Bart's question about io_uring fixed files:
if submissions can continue after the last close, "cease shortly"
does not hold, and it is the freeze wait that actually drains them.

> +	if (need_clear) {
> +		/*
> +		 * Grab all references that will be dropped as soon as
> +		 * returning from lo_release() and releasing disk->open_mutex.
> +		 */
> +		get_device(disk_to_dev(disk));
> +		__module_get(disk->fops->owner);
> +		queue_work(system_long_wq, &lo->lo_clr_work);
> +	}

With teardown now asynchronous, between the last close()
returning and the work item finishing, lo_open() and
LOOP_CONFIGURE return -ENXIO.  That is the same behavior change
that led to the revert of the earlier attempt (bf23747ee053,
xfs/259).  Moving the xfstests side to the tests is one thing, but
userspace that closes a loop device and immediately reconfigures
it now needs to handle a transient -ENXIO.  Is that acceptable, or
should the retry happen in the kernel?

Thanks,
Tao

On 8/31/26 7:11 AM, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> Hi Tetsuo, Bart,
> 
>> +	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
>> +	/*
>> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
>> +	 * wait for already started loop_queue_rq() to complete.
>> +	 */
>> +	synchronize_rcu();
> 
> Your reply to Bart says loop_queue_rq() is called with RCU read
> lock, but I don't see one.
This is the code that protects .queue_rq() implementations like
loop_queue_rq() with an rcu_read_lock() / rcu_read_unlock() pair:

/* run the code block in @dispatch_ops with rcu/srcu read lock held */
#define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
do {								\
	if ((q)->tag_set->flags & BLK_MQ_F_BLOCKING) {		\
		struct blk_mq_tag_set *__tag_set = (q)->tag_set; \
		int srcu_idx;					\
								\
		might_sleep_if(check_sleep);			\
		srcu_idx = srcu_read_lock(__tag_set->srcu);	\
		(dispatch_ops);					\
		srcu_read_unlock(__tag_set->srcu, srcu_idx);	\
	} else {						\
		rcu_read_lock();				\
		(dispatch_ops);					\
		rcu_read_unlock();				\
	}							\
} while (0)

Bart.

Thank you for joining to this thread, Tao.

On 2026/08/31 23:11, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> Hi Tetsuo, Bart,
> 
>> +	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
>> +	/*
>> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
>> +	 * wait for already started loop_queue_rq() to complete.
>> +	 */
>> +	synchronize_rcu();
> 
> Your reply to Bart says loop_queue_rq() is called with RCU read
> lock, but I don't see one.  The two call sites of ->queue_rq() are
> blk_mq_dispatch_rq_list() and __blk_mq_issue_directly(), and
> neither is wrapped in rcu_read_lock().  Direct issue from
> blk_mq_submit_bio() runs in process context with no RCU read-side
> critical section, so synchronize_rcu() does not wait for a
> loop_queue_rq() that is already running there.  Only the softirq
> dispatch path is an implicit RCU reader.

You can read sashiko's raw logs at https://sashiko.dev/#/log/49552 .

  Okay, let's break this down. My initial thought is to examine how the `blk_mq_run_dispatch_ops`
  macro is implemented. Specifically, how does it handle RCU read locks? It wraps the provided
  `dispatch_ops` in either `rcu_read_lock()` or `srcu_read_lock()`, depending on the `BLK_MQ_F_BLOCKING`
  flag. The loop device, however, is set up *without* this `BLK_MQ_F_BLOCKING` flag. Therefore,
  `loop_queue_rq` executes under the protection of `rcu_read_lock()`.

  Now, the critical question: Does the `synchronize_rcu()` call in `__loop_clr_fd` effectively address
  the potential race conditions? Since `loop_queue_rq()` is indeed RCU-protected, `synchronize_rcu()` will
  correctly wait for all currently executing instances of `loop_queue_rq()` to complete. Furthermore,
  the `lo_state` transition to `Lo_rundown` before calling `__loop_clr_fd` ensures that any *new* calls to
  `loop_queue_rq()` will see this state and return an error. This perfectly fences the execution of
  `loop_queue_rq()`. Moreover, `drain_workqueue()` then makes sure all queued works are completed, and
  these are scheduled by `loop_queue_rq()`. This is great; all scheduled works are completed before
  clearing `lo->lo_backing_file`.

> 
> The window is still closed by the steps below, so this is not a
> correctness bug, but the synchronize_rcu() is not doing what the
> comment claims.  blk_mq_quiesce_queue() +
> blk_mq_wait_quiesce_done(), as Bart suggested, would express the
> intent directly.

synchronize_rcu() + drain_workqueue() can be implied by blk_mq_freeze_queue() + blk_mq_unfreeze_queue().
But why are you talking about blk_mq_quiesce_queue() + blk_mq_wait_quiesce_done() ?

> 
>> +	/*
>> +	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
>> +	 * wait for already started AIO to complete.
>> +	 */
>> +	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
> 
> About this step, in your follow-up you wrote:
> 
>>   (1) since we are in lo_release() with disk_openers(disk) == 0, the activity of
>>       incrementing/decrementing q_usage_counter (incremented before loop_queue_rq()
>>       is called, and decremented after loop_queue_rq() returned BLK_STS_IOERR)) will
>>       cease shortly
> 
> The decrement timing here is only true for the error path.  For
> BLK_STS_OK the reference is held until the request is freed, which
> is what makes blk_mq_freeze_queue() wait for requests that already
> passed the state check, including the loop workqueue worker that
> completes them.  That is the property step 1 relies on, and it is
> worth stating in the comment.
> 
> What is still open is Bart's question about io_uring fixed files:
> if submissions can continue after the last close, "cease shortly"
> does not hold, and it is the freeze wait that actually drains them.

If submissions can continue _forever_ despite disk_openers(disk) == 0, what
mechanism was preventing this problem from occurring until Linux 7.0 ?

> 
>> +	if (need_clear) {
>> +		/*
>> +		 * Grab all references that will be dropped as soon as
>> +		 * returning from lo_release() and releasing disk->open_mutex.
>> +		 */
>> +		get_device(disk_to_dev(disk));
>> +		__module_get(disk->fops->owner);
>> +		queue_work(system_long_wq, &lo->lo_clr_work);
>> +	}
> 
> With teardown now asynchronous, between the last close()
> returning and the work item finishing, lo_open() and
> LOOP_CONFIGURE return -ENXIO.  That is the same behavior change
> that led to the revert of the earlier attempt (bf23747ee053,
> xfs/259).  Moving the xfstests side to the tests is one thing, but
> userspace that closes a loop device and immediately reconfigures
> it now needs to handle a transient -ENXIO.  Is that acceptable, or
> should the retry happen in the kernel?

Does whether the teardown being synchronous or asynchronous matter so much?

I don't think we can control when e.g. udev-worker becomes the thread
who actually calls __loop_clr_fd()
( https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp ).
Even if an existing user app calls close() immediately followed by open(),
we can't prove that __loop_clr_fd() is synchronously called by that user app
because udev-worker can jump in and udev-worker becomes the thread who actually
calls __loop_clr_fd().

   An user app     udev-worker
   -----------     -----------
                    open()
    close()
    open() // => succeeds due to Lo_bound state
    ioctl(LOOP_CONFIGURE) // => fails with -EBUSY due to Lo_bound state
    close() // <= gives up due to ioctl() failure
                    close() // => becomes  Lo_rundown and __loop_clr_fd() is called

A claim that mentions that the kernel is unable to release forever due to a refcount
leak bug is valid. But a claim that mentions that the kernel cannot prove that
this -ENXIO or -EBUSY problem never happens is invalid. Programs that use the loop
device have to be prepared for transient errors.

Hi Bart, Tetsuo,

Thanks for the detailed replies, and to Tetsuo for the sashiko
log.

在 2026/8/31 23:49, Tetsuo Handa 写道:
> Thank you for joining to this thread, Tao.
> 
> On 2026/08/31 23:11, Tao Cui wrote:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> Hi Tetsuo, Bart,
>>
>>> +	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
>>> +	/*
>>> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
>>> +	 * wait for already started loop_queue_rq() to complete.
>>> +	 */
>>> +	synchronize_rcu();
>>
>> Your reply to Bart says loop_queue_rq() is called with RCU read
>> lock, but I don't see one.  The two call sites of ->queue_rq() are
>> blk_mq_dispatch_rq_list() and __blk_mq_issue_directly(), and
>> neither is wrapped in rcu_read_lock().  Direct issue from
>> blk_mq_submit_bio() runs in process context with no RCU read-side
>> critical section, so synchronize_rcu() does not wait for a
>> loop_queue_rq() that is already running there.  Only the softirq
>> dispatch path is an implicit RCU reader.
> 
> You can read sashiko's raw logs at https://sashiko.dev/#/log/49552 .
> 
>   Okay, let's break this down. My initial thought is to examine how the `blk_mq_run_dispatch_ops`
>   macro is implemented. Specifically, how does it handle RCU read locks? It wraps the provided
>   `dispatch_ops` in either `rcu_read_lock()` or `srcu_read_lock()`, depending on the `BLK_MQ_F_BLOCKING`
>   flag. The loop device, however, is set up *without* this `BLK_MQ_F_BLOCKING` flag. Therefore,
>   `loop_queue_rq` executes under the protection of `rcu_read_lock()`.
> 
>   Now, the critical question: Does the `synchronize_rcu()` call in `__loop_clr_fd` effectively address
>   the potential race conditions? Since `loop_queue_rq()` is indeed RCU-protected, `synchronize_rcu()` will
>   correctly wait for all currently executing instances of `loop_queue_rq()` to complete. Furthermore,
>   the `lo_state` transition to `Lo_rundown` before calling `__loop_clr_fd` ensures that any *new* calls to
>   `loop_queue_rq()` will see this state and return an error. This perfectly fences the execution of
>   `loop_queue_rq()`. Moreover, `drain_workqueue()` then makes sure all queued works are completed, and
>   these are scheduled by `loop_queue_rq()`. This is great; all scheduled works are completed before
>   clearing `lo->lo_backing_file`.
> 

Bart, on 8/31 you wrote:
> This is the code that protects .queue_rq() implementations like
> loop_queue_rq() with an rcu_read_lock() / rcu_read_unlock() pair:
>
> /* run the code block in @dispatch_ops with rcu/srcu read lock held */
> #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
> ...
> 	} else {						\
> 		rcu_read_lock();				\
> 		(dispatch_ops);					\
> 		rcu_read_unlock();				\
> 	}							\
> } while (0)

You are both right and I was wrong.  I stopped at
__blk_mq_issue_directly() and blk_mq_dispatch_rq_list() and missed
that every caller goes through this macro.  So synchronize_rcu()
does wait for a running loop_queue_rq() with the current loop
setup.  I withdraw that comment.

>>
>> The window is still closed by the steps below, so this is not a
>> correctness bug, but the synchronize_rcu() is not doing what the
>> comment claims.  blk_mq_quiesce_queue() +
>> blk_mq_wait_quiesce_done(), as Bart suggested, would express the
>> intent directly.
> 
> synchronize_rcu() + drain_workqueue() can be implied by blk_mq_freeze_queue() + blk_mq_unfreeze_queue().
> But why are you talking about blk_mq_quiesce_queue() + blk_mq_wait_quiesce_done() ?
> 

That was Bart's suggestion from his earlier review, and I passed
it on while assuming the synchronize_rcu() was unfounded.  With
the RCU pairing confirmed, there is nothing left to replace and
the quiesce suggestion is moot.

>>
>>> +	/*
>>> +	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
>>> +	 * wait for already started AIO to complete.
>>> +	 */
>>> +	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
>>
>> About this step, in your follow-up you wrote:
>>
>>>   (1) since we are in lo_release() with disk_openers(disk) == 0, the activity of
>>>       incrementing/decrementing q_usage_counter (incremented before loop_queue_rq()
>>>       is called, and decremented after loop_queue_rq() returned BLK_STS_IOERR)) will
>>>       cease shortly
>>
>> The decrement timing here is only true for the error path.  For
>> BLK_STS_OK the reference is held until the request is freed, which
>> is what makes blk_mq_freeze_queue() wait for requests that already
>> passed the state check, including the loop workqueue worker that
>> completes them.  That is the property step 1 relies on, and it is
>> worth stating in the comment.
>>
>> What is still open is Bart's question about io_uring fixed files:
>> if submissions can continue after the last close, "cease shortly"
>> does not hold, and it is the freeze wait that actually drains them.
> 

The decrement part I would still reword: for BLK_STS_OK the
reference is held until the request is freed, and that is the
property blk_mq_freeze_queue() relies on.  But you are right that
the activity ceases, and for a simpler reason than the freeze:

> If submissions can continue _forever_ despite disk_openers(disk) == 0, what
> mechanism was preventing this problem from occurring until Linux 7.0 ?
> 

An io_uring fixed file holds a reference to the struct file, so
bd_openers never reaches zero and lo_release() does not start
while such a submission path exists.  Any submission needs an
open block device file (or a mount holding it), so "openers == 0
but submissions continue" needs no additional mechanism.  I'll
drop the io_uring part.

>>
>>> +	if (need_clear) {
>>> +		/*
>>> +		 * Grab all references that will be dropped as soon as
>>> +		 * returning from lo_release() and releasing disk->open_mutex.
>>> +		 */
>>> +		get_device(disk_to_dev(disk));
>>> +		__module_get(disk->fops->owner);
>>> +		queue_work(system_long_wq, &lo->lo_clr_work);
>>> +	}
>>
>> With teardown now asynchronous, between the last close()
>> returning and the work item finishing, lo_open() and
>> LOOP_CONFIGURE return -ENXIO.  That is the same behavior change
>> that led to the revert of the earlier attempt (bf23747ee053,
>> xfs/259).  Moving the xfstests side to the tests is one thing, but
>> userspace that closes a loop device and immediately reconfigures
>> it now needs to handle a transient -ENXIO.  Is that acceptable, or
>> should the retry happen in the kernel?
> 
> Does whether the teardown being synchronous or asynchronous matter so much?
> 
> I don't think we can control when e.g. udev-worker becomes the thread
> who actually calls __loop_clr_fd()
> ( https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp ).
> Even if an existing user app calls close() immediately followed by open(),
> we can't prove that __loop_clr_fd() is synchronously called by that user app
> because udev-worker can jump in and udev-worker becomes the thread who actually
> calls __loop_clr_fd().
> 
>    An user app     udev-worker
>    -----------     -----------
>                     open()
>     close()
>     open() // => succeeds due to Lo_bound state
>     ioctl(LOOP_CONFIGURE) // => fails with -EBUSY due to Lo_bound state
>     close() // <= gives up due to ioctl() failure
>                     close() // => becomes  Lo_rundown and __loop_clr_fd() is called
> 
> A claim that mentions that the kernel is unable to release forever due to a refcount
> leak bug is valid. But a claim that mentions that the kernel cannot prove that
> this -ENXIO or -EBUSY problem never happens is invalid. Programs that use the loop
> device have to be prepared for transient errors.

Your udev-worker sequence shows that synchronous completion was
never guaranteed and -EBUSY or -ENXIO transients were already
possible.  I accept that.  The remaining difference is only that
the window now exists after every close instead of requiring an
interleaving close from another opener, and if programs must
tolerate transient errors anyway, that is not a blocker.

Tao