[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

.

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

[Tao Cui]

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

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

[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`.

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

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

[Tao Cui]

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

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

[Bart Van Assche]

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.

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

[Bart Van Assche]

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.

[PATCH v7.1] 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 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

[PATCH v8] 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 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