[PATCH v5] loop: Fix NULL pointer dereference in lo_rw_aio()

[PATCH v5] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

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, this patch introduces 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, and
also emits BUG: message in order to help filesystem developers identify
the caller of an I/O request that failed to wait for completion so that
filesystem developers can fix such caller to wait for completion.

Note that emitting BUG: message is enabled only if CONFIG_KCOV=y, for
this check is a waste of computation resources for almost all users.

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]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
syzbot has tested this patch (with debug printk() added) using linux-next,
and I confirmed that this patch does not cause new problems for syzbot.

I was expecting that Al Viro can reproduce xfs/259 problem with debug printk(),
but I have to remove this patch (with debug printk() added) from linux-next
because the debug printk() causes performance problem for unmount stress tests
( https://lkml.kernel.org/r/202607151655.9d74999d-lkp@intel.com ).

Anyway, like I have explained in
https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@I-love.SAKURA.ne.jp ,
I think that the xfs/259 problem should be addressed by updating the "umount" user,
for the "target is busy" problem can be reproduced with 7.0 and 7.1 kernels if
delay injection is used.

Therefore, I think that we should proceed to the next step; i.e. identify
who is issuing I/O requests too late and fix such users. But I had to stop
testing this patch using linux-next before syzbot succeeds to find such users.

We can't use "#syz test" because this problem has no reproducers. Since I can
no longer continue floating debug printk() for this problem, sending this patch
to upstream will become the only way to identify who is issuing I/O requests
too late.

drivers/block/loop.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 310de0463beb..c3b607a3ddc4 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -85,8 +85,27 @@ struct loop_cmd {
 	struct bio_vec *bvec;
 	struct cgroup_subsys_state *blkcg_css;
 	struct cgroup_subsys_state *memcg_css;
+#ifdef CONFIG_KCOV
+	unsigned long stack_entries[30];
+	int stack_nr;
+	pid_t pid;
+	char comm[TASK_COMM_LEN];
+#endif
 };
 
+static void loop_check_io_race(struct loop_device *lo, struct loop_cmd *cmd)
+{
+#ifdef CONFIG_KCOV
+	if (unlikely(data_race(READ_ONCE(lo->lo_state)) == Lo_rundown &&
+		     disk_openers(lo->lo_disk) == 0)) {
+		pr_err("BUG: %s/%u is doing I/O request on loop%d in Lo_rundown state.\n",
+		       cmd->comm, cmd->pid, lo->lo_number);
+		printk("Call Trace:\n");
+		stack_trace_print(cmd->stack_entries, cmd->stack_nr, 4);
+	}
+#endif
+}
+
 #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
 #define LOOP_DEFAULT_HW_Q_DEPTH 128
 
@@ -1743,8 +1762,59 @@ static void lo_release(struct gendisk *disk)
 	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
 
-	if (need_clear)
+	if (need_clear) {
+		/*
+		 * Temporarily release disk->open_mutex in order to flush pending I/O
+		 * requests before clearing the backing device.
+		 *
+		 * This is a layering violation. But since bdev->bd_disk->fops->release()
+		 * (which is mapped to lo_release()) is the final function which
+		 * blkdev_put_whole() from bdev_release() calls immediately before
+		 * releasing disk->open_mutex, this changes nothing except opens a new
+		 * race window for allowing disk->fops->open() (which is mapped to
+		 * lo_open()) to be called.
+		 *
+		 * Even if lo_open() is called from blkdev_get_whole() due to this race,
+		 * the Lo_rundown state guarantees that lo_open() will fail with -ENXIO.
+		 * Thus, there will be effectively no change caused by this violation.
+		 */
+		mutex_unlock(&lo->lo_disk->open_mutex);
+		/*
+		 * 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.
+		 *
+		 * Due to synchronize_rcu() + drain_workqueue() sequence above,
+		 * calling blk_mq_unfreeze_queue() immediately after blk_mq_freeze_queue()
+		 * returns has to be safe, for loop_queue_rq() no longer schedules new
+		 * lo_rw_aio() works and lo_rw_aio() no longer submits new AIO requests.
+		 *
+		 * Deferring blk_mq_unfreeze_queue() does not help because we are about
+		 * to clear the backing device and drop the refcount for the backing device.
+		 * There is nothing we can do if blk_mq_freeze_queue() fails to flush.
+		 */
+		blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+		/*
+		 * Perform remaining cleanup, with disk->open_mutex held.
+		 *
+		 * The lo->lo_state should remain Lo_rundown despite we temporarily
+		 * released disk->open_mutex, for I am the only and the last user of
+		 * this loop device because lo_open() cannot succeed.
+		 */
+		mutex_lock(&lo->lo_disk->open_mutex);
+		if (WARN_ON(data_race(READ_ONCE(lo->lo_state)) != Lo_rundown))
+			return;
 		__loop_clr_fd(lo);
+	}
 }
 
 static void lo_free_disk(struct gendisk *disk)
@@ -1851,10 +1921,18 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
 	struct loop_device *lo = rq->q->queuedata;
 
+#ifdef CONFIG_KCOV
+	cmd->stack_nr = stack_trace_save(cmd->stack_entries, ARRAY_SIZE(cmd->stack_entries), 0);
+	cmd->pid = current->pid;
+	get_task_comm(cmd->comm, current);
+#endif
+
 	blk_mq_start_request(rq);
 
-	if (data_race(READ_ONCE(lo->lo_state)) != Lo_bound)
+	if (data_race(READ_ONCE(lo->lo_state)) != Lo_bound) {
+		loop_check_io_race(lo, cmd);
 		return BLK_STS_IOERR;
+	}
 
 	switch (req_op(rq)) {
 	case REQ_OP_FLUSH:
@@ -1897,6 +1975,7 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
 	int ret = 0;
 	struct mem_cgroup *old_memcg = NULL;
 
+	loop_check_io_race(lo, cmd);
 	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY)) {
 		ret = -EIO;
 		goto failed;
-- 
2.52.0

[PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

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, this patch introduces 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.

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]
Fixes: 65565ca5f99b ("block: unify the synchronous bi_end_io callbacks")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v6:
  Drop debug code for emitting BUG: message, which is intended for helping
  filesystem developers identify the caller of an I/O request that failed to
  wait for completion.

 drivers/block/loop.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..feebc9999926 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1769,8 +1769,59 @@ static void lo_release(struct gendisk *disk)
 	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
 
-	if (need_clear)
+	if (need_clear) {
+		/*
+		 * Temporarily release disk->open_mutex in order to flush pending I/O
+		 * requests before clearing the backing device.
+		 *
+		 * This is a layering violation. But since bdev->bd_disk->fops->release()
+		 * (which is mapped to lo_release()) is the final function which
+		 * blkdev_put_whole() from bdev_release() calls immediately before
+		 * releasing disk->open_mutex, this changes nothing except opens a new
+		 * race window for allowing disk->fops->open() (which is mapped to
+		 * lo_open()) to be called.
+		 *
+		 * Even if lo_open() is called from blkdev_get_whole() due to this race,
+		 * the Lo_rundown state guarantees that lo_open() will fail with -ENXIO.
+		 * Thus, there will be effectively no change caused by this violation.
+		 */
+		mutex_unlock(&lo->lo_disk->open_mutex);
+		/*
+		 * 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.
+		 *
+		 * Due to synchronize_rcu() + drain_workqueue() sequence above,
+		 * calling blk_mq_unfreeze_queue() immediately after blk_mq_freeze_queue()
+		 * returns has to be safe, for loop_queue_rq() no longer schedules new
+		 * lo_rw_aio() works and lo_rw_aio() no longer submits new AIO requests.
+		 *
+		 * Deferring blk_mq_unfreeze_queue() does not help because we are about
+		 * to clear the backing device and drop the refcount for the backing device.
+		 * There is nothing we can do if blk_mq_freeze_queue() fails to flush.
+		 */
+		blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));
+		/*
+		 * Perform remaining cleanup, with disk->open_mutex held.
+		 *
+		 * The lo->lo_state should remain Lo_rundown despite we temporarily
+		 * released disk->open_mutex, for I am the only and the last user of
+		 * this loop device because lo_open() cannot succeed.
+		 */
+		mutex_lock(&lo->lo_disk->open_mutex);
+		if (WARN_ON(data_race(READ_ONCE(lo->lo_state)) != Lo_rundown))
+			return;
 		__loop_clr_fd(lo);
+	}
 }
 
 static void lo_free_disk(struct gendisk *disk)
-- 
2.52.0

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Markus Elfring]

…
> Therefore, this patch introduces a grace period for …

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n94

Regards,
Markus

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/08/24 0:57, Markus Elfring wrote:
> …
>> Therefore, this patch introduces a grace period for …
> 
> See also once more:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n94

OK. I can do "s/this patch introduces/introduce/".

sashiko did not find problems with my patch
( https://sashiko.dev/#/patchset/8dedfc40-9cae-44ff-9960-e0eb1825e963%40I-love.SAKURA.ne.jp ).

Bart did not like my proposed solution and is making a series, but 
sashiko found problems with Bart's series
( https://sashiko.dev/#/patchset/8330ca0e-b4d9-451a-bc0d-a50178194ea8@I-love.SAKURA.ne.jp and
https://sashiko.dev/#/patchset/2efb1032-34b9-4c31-b6fc-4aa9b1d60895@I-love.SAKURA.ne.jp ).
I still cannot catch why Bart's approach can become safe without synchronize_rcu() and
drain_workqueue().

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/24/26 3:06 PM, Tetsuo Handa wrote:
> but sashiko found problems with Bart's series ( https://sashiko.dev/
> #/patchset/8330ca0e-b4d9-451a-bc0d-a50178194ea8@I-love.SAKURA.ne.jp
> and https://sashiko.dev/#/patchset/2efb1032-34b9-4c31-
> b6fc-4aa9b1d60895@I-love.SAKURA.ne.jp ).

Thanks, I had not yet seen this feedback from Sashiko. My conclusion is
that two of the three Sashiko findings are wrong. I will fix the third
one - the finding about the potential wait-die deadlock.

> I still cannot catch why Bart's approach can become safe without
> synchronize_rcu() and drain_workqueue().

Agreed that drain_workqueue() or flush_workqueue() is necessary. I do
not agree that an explicit synchronize_rcu() call is required. Waiting
until the request queue is frozen includes this call implicitly.
See also __percpu_ref_switch_to_atomic().

Thanks,

Bart.

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/24/26 3:53 PM, Bart Van Assche wrote:
> On 8/24/26 3:06 PM, Tetsuo Handa wrote:
>> I still cannot catch why Bart's approach can become safe without
>> synchronize_rcu() and drain_workqueue().
> 
> Agreed that drain_workqueue() or flush_workqueue() is necessary. I do
> not agree that an explicit synchronize_rcu() call is required. Waiting
> until the request queue is frozen includes this call implicitly.
> See also __percpu_ref_switch_to_atomic().

A correction: it is not safe to call drain_workqueue() nor to freeze the
request queue in __loop_clr_fd(). I'm considering to modify the comment
in that function as follows:

--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1203,8 +1203,13 @@ static void __loop_clr_fd(struct loop_device *lo)
         /*
          * 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.
+        * Asynchronously submitted I/O may still be in progress. 
Freezing the
+        * request queue is not possible without triggering lock inversion.
+        * Freezing a request queue must happen between the
+        * queue_limits_start_update() and queue_limits_commit_update 
calls.  If
+        * the request queue would be frozen between these two calls, a 
deadlock
+        * can be triggered with the loop_clear_limits() call from the 
I/O path
+        * since loop_clear_limits() also calls queue_limits_start_update().
          */

Thanks,

Bart.

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/08/25 7:53, Bart Van Assche wrote:
> On 8/24/26 3:06 PM, Tetsuo Handa wrote:
>> but sashiko found problems with Bart's series ( https://sashiko.dev/
>> #/patchset/8330ca0e-b4d9-451a-bc0d-a50178194ea8@I-love.SAKURA.ne.jp
>> and https://sashiko.dev/#/patchset/2efb1032-34b9-4c31-
>> b6fc-4aa9b1d60895@I-love.SAKURA.ne.jp ).
> 
> Thanks, I had not yet seen this feedback from Sashiko. My conclusion is
> that two of the three Sashiko findings are wrong. I will fix the third
> one - the finding about the potential wait-die deadlock.
> 

As of commit fe4c990e6a30 ("loop: Add __guarded_by() annotations") in block-loop branch,

  modprobe loop
  losetup /dev/loop0 testfile.img; losetup /dev/loop1 /dev/loop0; losetup -D
  sleep 1
  losetup /dev/loop1 testfile.img; losetup /dev/loop0 /dev/loop1; losetup -D

causes lockdep warning. This is a false positive, but we need to avoid it anyway.

>> I still cannot catch why Bart's approach can become safe without
>> synchronize_rcu() and drain_workqueue().
> 
> Agreed that drain_workqueue() or flush_workqueue() is necessary. I do
> not agree that an explicit synchronize_rcu() call is required. Waiting
> until the request queue is frozen includes this call implicitly.
> See also __percpu_ref_switch_to_atomic().

OK. You recognized that we need drain_workqueue() or flush_workqueue(), and

On 2026/08/25 8:24, Bart Van Assche wrote:
> On 8/24/26 3:53 PM, Bart Van Assche wrote:
>> On 8/24/26 3:06 PM, Tetsuo Handa wrote:
>>> I still cannot catch why Bart's approach can become safe without
>>> synchronize_rcu() and drain_workqueue().
>>
>> Agreed that drain_workqueue() or flush_workqueue() is necessary. I do
>> not agree that an explicit synchronize_rcu() call is required. Waiting
>> until the request queue is frozen includes this call implicitly.
>> See also __percpu_ref_switch_to_atomic().
> 
> A correction: it is not safe to call drain_workqueue() nor to freeze the
> request queue in __loop_clr_fd(). I'm considering to modify the comment
> in that function as follows:

you also recognized that we can't call drain_workqueue() or flush_workqueue().

Therefore, I chose to temporarily drop lo->lo_disk->open_mutex in order to
make it possible to safely perform drain_workqueue().

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/25/26 8:13 AM, Tetsuo Handa wrote:
> As of commit fe4c990e6a30 ("loop: Add __guarded_by() annotations") in block-loop branch,
> 
>    modprobe loop
>    losetup /dev/loop0 testfile.img; losetup /dev/loop1 /dev/loop0; losetup -D
>    sleep 1
>    losetup /dev/loop1 testfile.img; losetup /dev/loop0 /dev/loop1; losetup -D
> 
> causes lockdep warning. This is a false positive, but we need to avoid it anyway.

Thanks for having shared the above reproducer. I have dropped the 
patches that protect all lo_backing_file dereferences with lo_mutex. It
is too tricky to get this right and at the same time to keep lockdep
happy.

> you also recognized that we can't call drain_workqueue() or flush_workqueue().
> 
> Therefore, I chose to temporarily drop lo->lo_disk->open_mutex in order to
> make it possible to safely perform drain_workqueue().

There is another possibility: set QUEUE_FLAG_DYING in __loop_clr_fd() 
before modifying queue limits and clear it again after modifying queue
limits has finished. Feedback on the patch below is welcome.

Thanks,

Bart.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index df72350cad15..2fb9dc8ea47e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1153,11 +1153,30 @@ static int loop_configure(struct loop_device 
*lo, blk_mode_t mode,

  static void __loop_clr_fd(struct loop_device *lo)
  {
+	struct request_queue *q = lo->lo_queue;
  	struct queue_limits lim;
+	unsigned int memflags;
  	struct file *filp;
  	gfp_t gfp = lo->old_gfp_mask;
  	int err;

+	/*
+	 * Prevent that new asynchronous I/O is submitted while queue limits
+	 * are being modified.
+	 */
+	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
+
+	/* Wait until asynchronous I/O has finished. */
+	memflags = blk_mq_freeze_queue(q);
+	blk_mq_unfreeze_queue(q, memflags);
+
+	/* Wait until I/O dispatching has finished. */
+	blk_mq_quiesce_queue(q);
+	blk_mq_unquiesce_queue(q);
+
+	/* Wait until all I/O-related work has finished. */
+	flush_workqueue(lo->workqueue);
+
  	mutex_lock(&lo->lo_mutex);
  	filp = lo->lo_backing_file;
  	lo->lo_backing_file = NULL;
@@ -1168,17 +1187,12 @@ 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.
-	 */
-	lim = queue_limits_start_update(lo->lo_queue);
+	/* Reset the block size to the default. */
+	lim = queue_limits_start_update(q);
  	lim.logical_block_size = SECTOR_SIZE;
  	lim.physical_block_size = SECTOR_SIZE;
  	lim.io_min = SECTOR_SIZE;
-	queue_limits_commit_update(lo->lo_queue, &lim);
+	queue_limits_commit_update(q, &lim);

  	invalidate_disk(lo->lo_disk);
  	loop_sysfs_exit(lo);
@@ -1216,6 +1230,9 @@ static void __loop_clr_fd(struct loop_device *lo)
  	WRITE_ONCE(lo->lo_state, Lo_unbound);
  	mutex_unlock(&lo->lo_mutex);

+	/* Reallow I/O. */
+	blk_queue_flag_clear(QUEUE_FLAG_DYING, q);
+
  	fput(filp);
  }

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/08/26 7:18, Bart Van Assche wrote:
>> you also recognized that we can't call drain_workqueue() or flush_workqueue().
>>
>> Therefore, I chose to temporarily drop lo->lo_disk->open_mutex in order to
>> make it possible to safely perform drain_workqueue().
> 
> There is another possibility: set QUEUE_FLAG_DYING in __loop_clr_fd() before modifying queue limits and clear it again after modifying queue
> limits has finished. Feedback on the patch below is welcome.
> 

See https://lkml.kernel.org/r/26717cb6-81b0-4d5d-a5db-669283f9bb9d@I-love.SAKURA.ne.jp .

> +    /* Wait until all I/O-related work has finished. */
> +    flush_workqueue(lo->workqueue);

I don't know about QUEUE_FLAG_DYING manipulation, but we know we can't call
flush_workqueue() while holding open_mutex.

Please post as a standalone patch (subject starting with "[PATCH" than "Re: [PATCH")
that can be applied on current linux.git tree, so that sashiko can review your patch.

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/25/26 8:13 AM, Tetsuo Handa wrote:
> On 2026/08/25 7:53, Bart Van Assche wrote:
>> A correction: it is not safe to call drain_workqueue() nor to freeze the
>> request queue in __loop_clr_fd(). I'm considering to modify the comment
>> in that function as follows:
> 
> you also recognized that we can't call drain_workqueue() or flush_workqueue().
> 
> Therefore, I chose to temporarily drop lo->lo_disk->open_mutex in order to
> make it possible to safely perform drain_workqueue().

My sentence was incomplete: I should have written that it is not safe to
call drain_workqueue() nor flush_workqueue() from __loop_clr_fd() while
the request queue is frozen. It is not clear to me how draining or
flushing the workqueue from inside __loop_clr_fd() could cause trouble
if this happens with the queue unfrozen since disk->open_mutex is not
acquired by the memory reclaim code?

Thanks,

Bart.

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/08/26 8:16, Bart Van Assche wrote:
> On 8/25/26 8:13 AM, Tetsuo Handa wrote:
>> On 2026/08/25 7:53, Bart Van Assche wrote:
>>> A correction: it is not safe to call drain_workqueue() nor to freeze the
>>> request queue in __loop_clr_fd(). I'm considering to modify the comment
>>> in that function as follows:
>>
>> you also recognized that we can't call drain_workqueue() or flush_workqueue().
>>
>> Therefore, I chose to temporarily drop lo->lo_disk->open_mutex in order to
>> make it possible to safely perform drain_workqueue().
> 
> My sentence was incomplete: I should have written that it is not safe to
> call drain_workqueue() nor flush_workqueue() from __loop_clr_fd() while
> the request queue is frozen. It is not clear to me how draining or
> flushing the workqueue from inside __loop_clr_fd() could cause trouble
> if this happens with the queue unfrozen since disk->open_mutex is not
> acquired by the memory reclaim code?

I don't know how "since disk->open_mutex is not acquired by the memory reclaim
code" is relevant...

Current situation is a result of what we had considered 4 years ago; we don't need
to destroy workqueue (note that destroy_workqueue() implies drain_workqueue()) from
__loop_clr_fd() ( https://lkml.kernel.org/r/20220330052917.2566582-16-hch@lst.de ).

Since there is a

  Chain exists of:
    (wq_completion)loop0 --> system_transition_mutex/1 --> &disk->open_mutex
  
   Possible unsafe locking scenario:
  
         CPU0                    CPU1
         ----                    ----
    lock(&disk->open_mutex);
                                 lock(system_transition_mutex/1);
                                 lock(&disk->open_mutex);
    lock((wq_completion)loop0);

dependency, but my proposal to forbid binding loop device to pseudo files
( https://lkml.kernel.org/r/148efba2-a0b6-47d7-ac76-b19d2f4b696c@I-love.SAKURA.ne.jp )
was rejected by Christoph, we are stuck in a

  Draining workqueue with open_mutex held causes creating a complex lock dependency
  chain involving the global system_transition_mutex. (Maybe there are other paths
  that create similar dependency chain if we drain workqueue with open_mutex held.)

versus

  Not draining workqueue causes NULL pointer dereference in lo_rw_aio().

collision. Therefore,

  Draining workqueue *without open_mutex held* can avoid creating a complex lock
  dependency chain involving the global system_transition_mutex and can also avoid
  NULL pointer dereference in lo_rw_aio().

is my solution.

Can you agree with my solution?

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/26/26 3:37 AM, Tetsuo Handa wrote:
> Current situation is a result of what we had considered 4 years ago; we don't need
> to destroy workqueue (note that destroy_workqueue() implies drain_workqueue()) from
> __loop_clr_fd() ( https://lkml.kernel.org/r/20220330052917.2566582-16-hch@lst.de ).
> 
> Since there is a
> 
>    Chain exists of:
>      (wq_completion)loop0 --> system_transition_mutex/1 --> &disk->open_mutex
>    
>     Possible unsafe locking scenario:
>    
>           CPU0                    CPU1
>           ----                    ----
>      lock(&disk->open_mutex);
>                                   lock(system_transition_mutex/1);
>                                   lock(&disk->open_mutex);
>      lock((wq_completion)loop0);

This ABBA locking scenario can only be triggered if a loop device is
bound to a sysfs attribute with read or write methods that lock
system_transition_mutex, e.g. /sys/kernel/power, isn't it?

> dependency, but my proposal to forbid binding loop device to pseudo files
> ( https://lkml.kernel.org/r/148efba2-a0b6-47d7-ac76-b19d2f4b696c@I-love.SAKURA.ne.jp )
 > was rejected by Christoph, we are stuck in a
 >
>    Draining workqueue with open_mutex held causes creating a complex lock dependency
>    chain involving the global system_transition_mutex. (Maybe there are other paths
>    that create similar dependency chain if we drain workqueue with open_mutex held.)
> 
> versus
> 
>    Not draining workqueue causes NULL pointer dereference in lo_rw_aio().
> 
> collision. Therefore,
> 
>    Draining workqueue *without open_mutex held* can avoid creating a complex lock
>    dependency chain involving the global system_transition_mutex and can also avoid
>    NULL pointer dereference in lo_rw_aio().
> 
> is my solution.

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().

I think there is another solution: instead of draining the workqueue
from inside __loop_clr_fd(), postpone it until the next time the loop
device is bound. See also the patch below.

Regarding your earlier request for a Sashiko review: I will look into
configuring Sashiko such that I can run "sashiko review ${commit_id}"
locally. The only part I'm missing right now is a Sashiko API key.

Thanks,

Bart.

loop: Serialize I/O and queue limits updates

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 119758b45e47..1406932fae93 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1086,6 +1086,12 @@ static int loop_configure(struct loop_device *lo, 
blk_mode_t mode,
  			error = -ENOMEM;
  			goto out_unlock;
  		}
+	} else {
+		/*
+		 * Wait until all work related to a previously bound file has
+		 * finished.
+		 */
+		flush_workqueue(lo->workqueue);
  	}

  	/* suppress uevents while reconfiguring the device */
@@ -1154,11 +1160,27 @@ static int loop_configure(struct loop_device 
*lo, blk_mode_t mode,
  static void __loop_clr_fd(struct gendisk *disk, struct loop_device *lo)
  	__must_hold(&disk->open_mutex)
  {
+	struct request_queue *q = lo->lo_queue;
  	struct queue_limits lim;
+	unsigned int memflags;
  	struct file *filp;
  	gfp_t gfp = lo->old_gfp_mask;
  	int err;

+	/*
+	 * Prevent that new asynchronous I/O is submitted while queue limits
+	 * are being modified.
+	 */
+	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
+
+	/* Wait until asynchronous I/O has finished. */
+	memflags = blk_mq_freeze_queue(q);
+	blk_mq_unfreeze_queue(q, memflags);
+
+	/* Wait until I/O dispatching has finished. */
+	blk_mq_quiesce_queue(q);
+	blk_mq_unquiesce_queue(q);
+
  	mutex_lock(&lo->lo_mutex);
  	filp = lo->lo_backing_file;
  	lo->lo_backing_file = NULL;
@@ -1169,17 +1191,12 @@ static void __loop_clr_fd(struct gendisk *disk, 
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.
-	 */
-	lim = queue_limits_start_update(lo->lo_queue);
+	/* Reset the block size to the default. */
+	lim = queue_limits_start_update(q);
  	lim.logical_block_size = SECTOR_SIZE;
  	lim.physical_block_size = SECTOR_SIZE;
  	lim.io_min = SECTOR_SIZE;
-	queue_limits_commit_update(lo->lo_queue, &lim);
+	queue_limits_commit_update(q, &lim);

  	invalidate_disk(disk);
  	loop_sysfs_exit(lo);
@@ -1217,6 +1234,9 @@ static void __loop_clr_fd(struct gendisk *disk, 
struct loop_device *lo)
  	WRITE_ONCE(lo->lo_state, Lo_unbound);
  	mutex_unlock(&lo->lo_mutex);

+	/* Reallow I/O. */
+	blk_queue_flag_clear(QUEUE_FLAG_DYING, q);
+
  	fput(filp);
  }

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/08/27 2:44, Bart Van Assche wrote:
> 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().

sashiko's review did not find problems
( https://sashiko.dev/#/patchset/8dedfc40-9cae-44ff-9960-e0eb1825e963%40I-love.SAKURA.ne.jp ).
This callback is the last code which is performed immediately before releasing disk->open_mutex.

> 
> I think there is another solution: instead of draining the workqueue
> from inside __loop_clr_fd(), postpone it until the next time the loop
> device is bound. See also the patch below.

That is too late to avoid NULL pointer dereference problem.
We need to flush before setting lo->lo_backing_file to NULL.

> 
> Regarding your earlier request for a Sashiko review: I will look into
> configuring Sashiko such that I can run "sashiko review ${commit_id}"
> locally. The only part I'm missing right now is a Sashiko API key.
While Sashiko considers a static blk_mq_freeze_queue() sufficient to avoid
the NULL pointer dereference, relying on freezing the queue inside __loop_clr_fd()
while holding `disk->open_mutex` is a dangerous decision.

It is true that blk_mq_freeze_queue() alone is equivalent to
synchronize_rcu() + drain_workqueue() + blk_mq_freeze_queue().

Although omitting drain_workqueue() causes lockdep to stop complaining, not
using drain_workqueue() does not avoid the runtime deadlock situation shown below.
As long as there is a possibility of an in-flight I/O holding or waiting on a
lock that subsequently tries to acquire `disk->open_mutex`, the deadlock
risk remains. Lockdep will remain silent here simply because it cannot inspect
the wake-up conditions inside wait_event() during a queue freeze.

Here is the exact execution timeline that leads to the silent deadlock:

Thread 1:                         Thread 2:                         Thread 3:
========================================================================================
                                                                    Holds a global lock 
                                                                    (e.g., system_transition_mutex).

                                  Block core increments 
                                  `q_usage_counter`.

Block core holds `disk->open_mutex`.

lo_release() tries to wait for 
`q_usage_counter` to reach 0 
via blk_mq_freeze_queue() 
  => BLOCKED by Thread 2.
                                  loop_handle_cmd() tries 
                                  to hold the global lock 
                                    => BLOCKED by Thread 3.
                                                                    Tries to hold 
                                                                    `disk->open_mutex` 
                                                                      => BLOCKED by Thread 1.
========================================================================================

Since Thread 1 will never release `disk->open_mutex`, Thread 3 can never release
the global lock, and Thread 2 can never reach blk_mq_end_request() to unblock Thread 1.

I've tried offloading __loop_clr_fd() entirely to task_work context in v3 patch, but
it did not work due to module lifecycle restrictions
( https://sashiko.dev/#/patchset/fda8abc8-6aa2-463b-bf72-865f6b838034@I-love.SAKURA.ne.jp ).

Therefore, I consider that temporarily releasing `disk->open_mutex` within the process
context to safely perform flushing/draining before clearing the backing file pointer
is the most robust architectural solution to break this deadlock chain.

Re: [PATCH v6] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

On 8/27/26 8:30 AM, Tetsuo Handa wrote:
> While Sashiko considers a static blk_mq_freeze_queue() sufficient to avoid
> the NULL pointer dereference, relying on freezing the queue inside __loop_clr_fd()
> while holding `disk->open_mutex` is a dangerous decision.
> 
> It is true that blk_mq_freeze_queue() alone is equivalent to
> synchronize_rcu() + drain_workqueue() + blk_mq_freeze_queue().
> 
> Although omitting drain_workqueue() causes lockdep to stop complaining, not
> using drain_workqueue() does not avoid the runtime deadlock situation shown below.
> As long as there is a possibility of an in-flight I/O holding or waiting on a
> lock that subsequently tries to acquire `disk->open_mutex`, the deadlock
> risk remains. Lockdep will remain silent here simply because it cannot inspect
> the wake-up conditions inside wait_event() during a queue freeze.
> 
> Here is the exact execution timeline that leads to the silent deadlock:
> 
> Thread 1:                         Thread 2:                         Thread 3:
> ========================================================================================
>                                                                      Holds a global lock
>                                                                      (e.g., system_transition_mutex).
> 
>                                    Block core increments
>                                    `q_usage_counter`.
> 
> Block core holds `disk->open_mutex`.
> 
> lo_release() tries to wait for
> `q_usage_counter` to reach 0
> via blk_mq_freeze_queue()
>    => BLOCKED by Thread 2.
>                                    loop_handle_cmd() tries
>                                    to hold the global lock
>                                      => BLOCKED by Thread 3.
>                                                                      Tries to hold
>                                                                      `disk->open_mutex`
>                                                                        => BLOCKED by Thread 1.
> ========================================================================================
> 
> Since Thread 1 will never release `disk->open_mutex`, Thread 3 can never release
> the global lock, and Thread 2 can never reach blk_mq_end_request() to unblock Thread 1.
> 
> I've tried offloading __loop_clr_fd() entirely to task_work context in v3 patch, but
> it did not work due to module lifecycle restrictions
> ( https://sashiko.dev/#/patchset/fda8abc8-6aa2-463b-bf72-865f6b838034@I-love.SAKURA.ne.jp ).
> 
> Therefore, I consider that temporarily releasing `disk->open_mutex` within the process
> context to safely perform flushing/draining before clearing the backing file pointer
> is the most robust architectural solution to break this deadlock chain.

I'm going to drop patch "loop: Serialize I/O and queue limits updates"
from my patch series and leave it to someone else to solve this issue
since none of the proposed fixes that have been discussed so far make me
enthusiast.

Thanks,

Bart.

[PATCH v7] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

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

Re: [PATCH v7] loop: Fix NULL pointer dereference in lo_rw_aio()

[Bart Van Assche]

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.

Re: [PATCH v7] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

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.

Re: [PATCH v7] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

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

.