Linux block layer
 help / color / mirror / Atom feed
* [PATCH 1/2] block: Add post_release() operation
@ 2026-09-09 10:48 Tetsuo Handa
  2026-09-09 10:49 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Tetsuo Handa
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-09 10:48 UTC (permalink / raw)
  To: Jens Axboe, Bart Van Assche, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

Add post_release() block device operation which provides a hook for
performing synchronous cleanup without disk->open_mutex held, which is
needed by the loop devices.

Real-world container engines, test suites, and system utilities rely on
fput() from __loop_clr_fd() being completed when lo_release() returns.
But changes which went to the v7.1 merge window broke an assumption that
there is no outstanding I/O when __loop_clr_fd() is called, causing NULL
pointer dereference problem in lo_rw_aio().

In order to fix this regression, we want to allow __loop_clr_fd() to flush
outstanding I/O. But calling drain_workqueue() from __loop_clr_fd() with
disk->open_mutex held causes lockdep warnings. We need a mechanism which
can flush outstanding I/O without disk->open_mutex held.

This post_release() operation is intended for performing only idempotent
actions such as flush_work(), for nothing prevents multiple threads from
concurrently calling this operation. That is, the loop device schedules
a work_struct for calling __loop_clr_fd() from lo_release() where
disk->open_mutex is held, and waits for completion of that work_struct
using post_release() operation where disk->open_mutex is not held.

Also, this post_release() operation is called from only bdev_release()
path. This is because loop_configure() is not yet called (there is nothing
to clear) if something went wrong between an initialization lo_open() and
an error-unwinding lo_release() within the bdev_open() path.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 block/bdev.c                     | 2 ++
 include/linux/blkdev.h           | 6 ++++++
 rust/kernel/block/mq/gen_disk.rs | 1 +
 3 files changed, 9 insertions(+)

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/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b..f05dba1b5962 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1605,6 +1605,12 @@ struct block_device_operations {
 	 * driver.
 	 */
 	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
+	/*
+	 * Called after disk->open_mutex is released in the bdev_release() path.
+	 * Used by loop devices that need to perform synchronization without
+	 * holding disk->open_mutex. This operation has to be idempotent.
+	 */
+	void (*post_release)(struct gendisk *disk);
 };
 
 #ifdef CONFIG_COMPAT
diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index fc97dd873974..2ff77ef49781 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -129,6 +129,7 @@ pub fn build<T: Operations>(
             submit_bio: None,
             open: None,
             release: None,
+            post_release: None,
             ioctl: None,
             compat_ioctl: None,
             check_events: None,
-- 
2.55.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-09 10:48 [PATCH 1/2] block: Add post_release() operation Tetsuo Handa
@ 2026-09-09 10:49 ` Tetsuo Handa
  2026-09-09 19:33   ` Bart Van Assche
  2026-09-10 11:53 ` [PATCH 1/2] block: Add post_release() operation Gary Guo
  2026-09-11 19:54 ` Bart Van Assche
  2 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-09 10:49 UTC (permalink / raw)
  To: Jens Axboe, Bart Van Assche, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

syzbot is reporting NULL pointer dereference in lo_rw_aio().
An analysis by the Gemini AI collaborator 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.

Since calling drain_workqueue() from __loop_clr_fd() with disk->open_mutex
held causes lockdep warnings, schedule lo_clr_work from lo_release() which
calls __loop_clr_fd() and wait for completion from lo_post_release().

Link: https://lkml.kernel.org/r/fbb3edda-f108-4e5b-acf2-266f043f8125@I-love.SAKURA.ne.jp
Reported-by: syzbot+cd8a9a308e879a4e2c28@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cd8a9a308e879a4e2c28
Reported-by: syzbot+bc273027d5643e48e5b3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bc273027d5643e48e5b3
Depends-on: "block: Add post_release() operation"
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>
---
 drivers/block/loop.c | 68 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 758c20678bf6..9fe0f7ca4c7e 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 {
@@ -1136,13 +1137,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;
@@ -1153,12 +1183,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;
@@ -1201,11 +1226,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);
 }
 
@@ -1771,8 +1794,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)
@@ -1791,6 +1828,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,
@@ -2036,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);
@@ -2140,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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-09 10:49 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Tetsuo Handa
@ 2026-09-09 19:33   ` Bart Van Assche
  2026-09-10  9:44     ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-09 19:33 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/9/26 3:49 AM, Tetsuo Handa wrote:
> -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);
__loop_clr_fd() is queued from the lo_post_release() callback and hence
may be called concurrently with or after another thread has called
bdev_open(). Hence, lo->lo->state should be checked instead of assuming
that it equals Lo_rundown.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-09 19:33   ` Bart Van Assche
@ 2026-09-10  9:44     ` Tetsuo Handa
  2026-09-11 20:03       ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-10  9:44 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/10 4:33, Bart Van Assche wrote:
> __loop_clr_fd() is queued from the lo_post_release() callback and hence
> may be called concurrently with or after another thread has called
> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
> that it equals Lo_rundown.

No, __loop_clr_fd() is queued from the lo_release() callback.

This patchset passed sashiko's review
( https://sashiko.dev/#/patchset/60bf7af2-b84e-4056-9195-a26ad51ada46%40I-love.SAKURA.ne.jp ).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-09 10:48 [PATCH 1/2] block: Add post_release() operation Tetsuo Handa
  2026-09-09 10:49 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Tetsuo Handa
@ 2026-09-10 11:53 ` Gary Guo
  2026-09-10 12:14   ` Tetsuo Handa
  2026-09-11 19:54 ` Bart Van Assche
  2 siblings, 1 reply; 22+ messages in thread
From: Gary Guo @ 2026-09-10 11:53 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Bart Van Assche, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On Wed Sep 9, 2026 at 11:48 AM BST, Tetsuo Handa wrote:
> Add post_release() block device operation which provides a hook for
> performing synchronous cleanup without disk->open_mutex held, which is
> needed by the loop devices.
>
> Real-world container engines, test suites, and system utilities rely on
> fput() from __loop_clr_fd() being completed when lo_release() returns.
> But changes which went to the v7.1 merge window broke an assumption that

What change? You should mention it with a Fixes tag.

The message should also explain how the assumption is broken, not just stating
that it's broken.

> there is no outstanding I/O when __loop_clr_fd() is called, causing NULL
> pointer dereference problem in lo_rw_aio().
>
> In order to fix this regression, we want to allow __loop_clr_fd() to flush
> outstanding I/O. But calling drain_workqueue() from __loop_clr_fd() with
> disk->open_mutex held causes lockdep warnings. We need a mechanism which
> can flush outstanding I/O without disk->open_mutex held.
>
> This post_release() operation is intended for performing only idempotent
> actions such as flush_work(), for nothing prevents multiple threads from
> concurrently calling this operation. That is, the loop device schedules
> a work_struct for calling __loop_clr_fd() from lo_release() where
> disk->open_mutex is held, and waits for completion of that work_struct
> using post_release() operation where disk->open_mutex is not held.
>
> Also, this post_release() operation is called from only bdev_release()
> path. This is because loop_configure() is not yet called (there is nothing
> to clear) if something went wrong between an initialization lo_open() and
> an error-unwinding lo_release() within the bdev_open() path.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  block/bdev.c                     | 2 ++
>  include/linux/blkdev.h           | 6 ++++++
>  rust/kernel/block/mq/gen_disk.rs | 1 +
>  3 files changed, 9 insertions(+)
>
> 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/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4f7905c3412b..f05dba1b5962 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1605,6 +1605,12 @@ struct block_device_operations {
>  	 * driver.
>  	 */
>  	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
> +	/*
> +	 * Called after disk->open_mutex is released in the bdev_release() path.
> +	 * Used by loop devices that need to perform synchronization without
> +	 * holding disk->open_mutex. This operation has to be idempotent.
> +	 */
> +	void (*post_release)(struct gendisk *disk);
>  };
>  
>  #ifdef CONFIG_COMPAT
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index fc97dd873974..2ff77ef49781 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -129,6 +129,7 @@ pub fn build<T: Operations>(
>              submit_bio: None,
>              open: None,
>              release: None,
> +            post_release: None,
>              ioctl: None,
>              compat_ioctl: None,
>              check_events: None,

You can use

    ..pin_init::zeroed()

to have all other fields zeroed. It might be beneficial if we care about knowing
all new callbacks, but it doesn't look this is the new case.

We probably should remove all `None` assignments so it's all covered by the
`..zeroed()`, but that'll need to be its own patch.

Best,
Gary


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-10 11:53 ` [PATCH 1/2] block: Add post_release() operation Gary Guo
@ 2026-09-10 12:14   ` Tetsuo Handa
  0 siblings, 0 replies; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-10 12:14 UTC (permalink / raw)
  To: Gary Guo, Jens Axboe, Bart Van Assche, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/10 20:53, Gary Guo wrote:
>> Real-world container engines, test suites, and system utilities rely on
>> fput() from __loop_clr_fd() being completed when lo_release() returns.
>> But changes which went to the v7.1 merge window broke an assumption that
> 
> What change? You should mention it with a Fixes tag.
> 
> The message should also explain how the assumption is broken, not just stating
> that it's broken.

Unfortunately, nobody knows why it got broken. Please see "[PATCH 2/2] loop:
Perform __loop_clr_fd() after disk->open_mutex is dropped." for background.

I have tried a debug printk() patch at
https://lkml.kernel.org/r/3244d4dd-8254-47c0-9609-b1db53450c7c@I-love.SAKURA.ne.jp
that tries to capture buggy callers, but syzbot could not reproduce this problem
using the linux-next tree. Now that I am forbidden to carry debug printk() patches
in the linux-next tree, we have no means to (try to) debug this problem unless we
send such debug code to the linux tree.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-09 10:48 [PATCH 1/2] block: Add post_release() operation Tetsuo Handa
  2026-09-09 10:49 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Tetsuo Handa
  2026-09-10 11:53 ` [PATCH 1/2] block: Add post_release() operation Gary Guo
@ 2026-09-11 19:54 ` Bart Van Assche
  2026-09-11 22:34   ` Tetsuo Handa
  2 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-11 19:54 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/9/26 3:48 AM, Tetsuo Handa wrote:
>   	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
> +	/*
> +	 * Called after disk->open_mutex is released in the bdev_release() path.
> +	 * Used by loop devices that need to perform synchronization without
> +	 * holding disk->open_mutex. This operation has to be idempotent.
> +	 */
> +	void (*post_release)(struct gendisk *disk);

Instead of referring to bdev_release(), which is not in the public API,
the above comment should mention that .post_release() is called after 
.release(). The above comment should also mention that .post_release()
implementations may sleep.

Additionally, why is .post_release() added at the end instead of just
under .release()?

Otherwise this patch looks good to me.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-10  9:44     ` Tetsuo Handa
@ 2026-09-11 20:03       ` Bart Van Assche
  2026-09-11 22:18         ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-11 20:03 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/10/26 2:44 AM, Tetsuo Handa wrote:
> On 2026/09/10 4:33, Bart Van Assche wrote:
>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>> may be called concurrently with or after another thread has called
>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>> that it equals Lo_rundown.
> 
> No, __loop_clr_fd() is queued from the lo_release() callback.

Yes, it is *queued* from the lo_release() callback function but there is
no guarantee that __loop_clr_fd() has started before bdev_open() is
called again.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-11 20:03       ` Bart Van Assche
@ 2026-09-11 22:18         ` Tetsuo Handa
  2026-09-12  1:02           ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-11 22:18 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/12 5:03, Bart Van Assche wrote:
> On 9/10/26 2:44 AM, Tetsuo Handa wrote:
>> On 2026/09/10 4:33, Bart Van Assche wrote:
>>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>>> may be called concurrently with or after another thread has called
>>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>>> that it equals Lo_rundown.
>>
>> No, __loop_clr_fd() is queued from the lo_release() callback.
> 
> Yes, it is *queued* from the lo_release() callback function but there is
> no guarantee that __loop_clr_fd() has started before bdev_open() is
> called again.
> 

Since lo->lo_state was set to Lo_rundown by lo_release(), lo_open() will return -ENXIO.
What can go wrong if bdev_open() is called again before __loop_clr_fd() starts?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-11 19:54 ` Bart Van Assche
@ 2026-09-11 22:34   ` Tetsuo Handa
  2026-09-12  0:31     ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-11 22:34 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/12 4:54, Bart Van Assche wrote:
> On 9/9/26 3:48 AM, Tetsuo Handa wrote:
>>       int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
>> +    /*
>> +     * Called after disk->open_mutex is released in the bdev_release() path.
>> +     * Used by loop devices that need to perform synchronization without
>> +     * holding disk->open_mutex. This operation has to be idempotent.
>> +     */
>> +    void (*post_release)(struct gendisk *disk);
> 
> Instead of referring to bdev_release(), which is not in the public API,
> the above comment should mention that .post_release() is called after .release().

Such change is not accurate. The .release() is also called if something went wrong after
the .open() succeeded. The reason I emphasis bdev_release() is that the .post_release()
is not called if something went wrong after the .open() succeeded.

> The above comment should also mention that .post_release()
> implementations may sleep.

Such change is fine.

> 
> Additionally, why is .post_release() added at the end instead of just
> under .release()?

Because I consider the .post_release() is categorized to

	/*
	 * Special callback for probing GPT entry at a given sector.
	 * Needed by Android devices, used by GPT scanner and MMC blk
	 * driver.
	 */
	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);

hook.

> 
> Otherwise this patch looks good to me.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-11 22:34   ` Tetsuo Handa
@ 2026-09-12  0:31     ` Bart Van Assche
  2026-09-12  1:43       ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-12  0:31 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/11/26 3:34 PM, Tetsuo Handa wrote:
> On 2026/09/12 4:54, Bart Van Assche wrote:
>> On 9/9/26 3:48 AM, Tetsuo Handa wrote:
>>>        int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
>>> +    /*
>>> +     * Called after disk->open_mutex is released in the bdev_release() path.
>>> +     * Used by loop devices that need to perform synchronization without
>>> +     * holding disk->open_mutex. This operation has to be idempotent.
>>> +     */
>>> +    void (*post_release)(struct gendisk *disk);
>>
>> Instead of referring to bdev_release(), which is not in the public API,
>> the above comment should mention that .post_release() is called after .release().
> 
> Such change is not accurate. The .release() is also called if something went wrong after
> the .open() succeeded. The reason I emphasis bdev_release() is that the .post_release()
> is not called if something went wrong after the .open() succeeded.

Please integrate the above text in the comment above .post_release().

>> The above comment should also mention that .post_release()
>> implementations may sleep.
> 
> Such change is fine.
> 
>>
>> Additionally, why is .post_release() added at the end instead of just
>> under .release()?
> 
> Because I consider the .post_release() is categorized to
> 
> 	/*
> 	 * Special callback for probing GPT entry at a given sector.
> 	 * Needed by Android devices, used by GPT scanner and MMC blk
> 	 * driver.
> 	 */
> 	int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
> 
> hook.

Are you claiming that .post_release() is Android-specific? I don't think
that's correct.

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-11 22:18         ` Tetsuo Handa
@ 2026-09-12  1:02           ` Bart Van Assche
  2026-09-12  1:22             ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-12  1:02 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/11/26 3:18 PM, Tetsuo Handa wrote:
> On 2026/09/12 5:03, Bart Van Assche wrote:
>> On 9/10/26 2:44 AM, Tetsuo Handa wrote:
>>> On 2026/09/10 4:33, Bart Van Assche wrote:
>>>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>>>> may be called concurrently with or after another thread has called
>>>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>>>> that it equals Lo_rundown.
>>>
>>> No, __loop_clr_fd() is queued from the lo_release() callback.
>>
>> Yes, it is *queued* from the lo_release() callback function but there is
>> no guarantee that __loop_clr_fd() has started before bdev_open() is
>> called again.
> 
> Since lo->lo_state was set to Lo_rundown by lo_release(), lo_open() will return -ENXIO.
> What can go wrong if bdev_open() is called again before __loop_clr_fd() starts?

This breaks LO_FLAGS_AUTOCLEAR, isn't it?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-12  1:02           ` Bart Van Assche
@ 2026-09-12  1:22             ` Tetsuo Handa
  2026-09-15 17:36               ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-12  1:22 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/12 10:02, Bart Van Assche wrote:
> On 9/11/26 3:18 PM, Tetsuo Handa wrote:
>> On 2026/09/12 5:03, Bart Van Assche wrote:
>>> On 9/10/26 2:44 AM, Tetsuo Handa wrote:
>>>> On 2026/09/10 4:33, Bart Van Assche wrote:
>>>>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>>>>> may be called concurrently with or after another thread has called
>>>>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>>>>> that it equals Lo_rundown.
>>>>
>>>> No, __loop_clr_fd() is queued from the lo_release() callback.
>>>
>>> Yes, it is *queued* from the lo_release() callback function but there is
>>> no guarantee that __loop_clr_fd() has started before bdev_open() is
>>> called again.
>>
>> Since lo->lo_state was set to Lo_rundown by lo_release(), lo_open() will return -ENXIO.
>> What can go wrong if bdev_open() is called again before __loop_clr_fd() starts?
> 
> This breaks LO_FLAGS_AUTOCLEAR, isn't it?

Why do you think so?

  App1                   App2                   system_long_wq
  Calls lo_release().
    Schedules __loop_clr_fd().
                         Calls lo_open() but fails with -ENXIO.
                                                Starts __loop_clr_fd().
  Calls lo_post_release().
    Starts waiting for completion of __loop_clr_fd().
                                                Finishes __loop_clr_fd().
    Finishes waiting for completion of __loop_clr_fd().

  Calls lo_open() again and succeeds.
                         Calls lo_open() again and succeeds.

App1's open() after close() is succeeding.
App2's open() being temporarily failing with -ENXIO should be acceptable.
If App2 wants to avoid repeatedly failing with -ENXIO, App2 should use
ioctl(LOOP_CTL_GET_FREE) before open().

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] block: Add post_release() operation
  2026-09-12  0:31     ` Bart Van Assche
@ 2026-09-12  1:43       ` Tetsuo Handa
  0 siblings, 0 replies; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-12  1:43 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/12 9:31, Bart Van Assche wrote:
> On 9/11/26 3:34 PM, Tetsuo Handa wrote:
>> On 2026/09/12 4:54, Bart Van Assche wrote:
>>> On 9/9/26 3:48 AM, Tetsuo Handa wrote:
>>>>        int (*alternative_gpt_sector)(struct gendisk *disk, sector_t *sector);
>>>> +    /*
>>>> +     * Called after disk->open_mutex is released in the bdev_release() path.
>>>> +     * Used by loop devices that need to perform synchronization without
>>>> +     * holding disk->open_mutex. This operation has to be idempotent.
>>>> +     */
>>>> +    void (*post_release)(struct gendisk *disk);
>>>
>>> Instead of referring to bdev_release(), which is not in the public API,
>>> the above comment should mention that .post_release() is called after .release().
>>
>> Such change is not accurate. The .release() is also called if something went wrong after
>> the .open() succeeded. The reason I emphasis bdev_release() is that the .post_release()
>> is not called if something went wrong after the .open() succeeded.
> 
> Please integrate the above text in the comment above .post_release().
> 
Are you OK with below description?

--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1577,6 +1577,14 @@ struct block_device_operations {
 			unsigned int flags);
 	int (*open)(struct gendisk *disk, blk_mode_t mode);
 	void (*release)(struct gendisk *disk);
+	/*
+	 * This operation is called after returned from release() and
+	 * disk->open_mutex was released. But this operation is not called
+	 * after an initialization open() has succeeded but something went
+	 * wrong and an error-unwinding release() was called.
+	 * This operation might sleep and has to be idempotent.
+	 */
+	void (*post_release)(struct gendisk *disk);
 	int (*ioctl)(struct block_device *bdev, blk_mode_t mode,
 			unsigned cmd, unsigned long arg);
 	int (*compat_ioctl)(struct block_device *bdev, blk_mode_t mode,


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-12  1:22             ` Tetsuo Handa
@ 2026-09-15 17:36               ` Bart Van Assche
  2026-09-15 22:38                 ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-15 17:36 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 9/11/26 6:22 PM, Tetsuo Handa wrote:
> On 2026/09/12 10:02, Bart Van Assche wrote:
>> On 9/11/26 3:18 PM, Tetsuo Handa wrote:
>>> On 2026/09/12 5:03, Bart Van Assche wrote:
>>>> On 9/10/26 2:44 AM, Tetsuo Handa wrote:
>>>>> On 2026/09/10 4:33, Bart Van Assche wrote:
>>>>>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>>>>>> may be called concurrently with or after another thread has called
>>>>>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>>>>>> that it equals Lo_rundown.
>>>>>
>>>>> No, __loop_clr_fd() is queued from the lo_release() callback.
>>>>
>>>> Yes, it is *queued* from the lo_release() callback function but there is
>>>> no guarantee that __loop_clr_fd() has started before bdev_open() is
>>>> called again.
>>>
>>> Since lo->lo_state was set to Lo_rundown by lo_release(), lo_open() will return -ENXIO.
>>> What can go wrong if bdev_open() is called again before __loop_clr_fd() starts?
>>
>> This breaks LO_FLAGS_AUTOCLEAR, isn't it?
> 
> Why do you think so?
> 
>    App1                   App2                   system_long_wq
>    Calls lo_release().
>      Schedules __loop_clr_fd().
>                           Calls lo_open() but fails with -ENXIO.
>                                                  Starts __loop_clr_fd().
>    Calls lo_post_release().
>      Starts waiting for completion of __loop_clr_fd().
>                                                  Finishes __loop_clr_fd().
>      Finishes waiting for completion of __loop_clr_fd().
> 
>    Calls lo_open() again and succeeds.
>                           Calls lo_open() again and succeeds.
> 
> App1's open() after close() is succeeding.
> App2's open() being temporarily failing with -ENXIO should be acceptable.
> If App2 wants to avoid repeatedly failing with -ENXIO, App2 should use
> ioctl(LOOP_CTL_GET_FREE) before open().

Userspace applications and tests (such as mount/umount, losetup, 
systemd, and xfstests) expect that:
1. Teardown of a loop device with LO_FLAGS_AUTOCLEAR completes
    synchronously during the final close() (in lo_release()), releasing
    the backing file references (fput()).
2. Calling open() immediately after close() succeeds (allowing the loop
    device to be immediately reallocated, reopened, or configured in
    Lo_unbound state).

There is evidence of this in the history of drivers/block/loop.c:
* The Asynchronous Autoclear Regression & Revert (Commits 322c4293ecc5
   and bf23747ee053) In December 2021, commit 322c4293ecc5
   ("loop: make autoclear operation asynchronous") attempted to break a
   circular lock dependency by offloading autoclear (__loop_clr_fd())
   from lo_release() to a workqueue (system_long_wq).
* In February 2022, commit bf23747ee053 ("loop: revert 'make autoclear
   operation asynchronous'") had to revert that change after xfstests
   broke. The commit message explicitly states:

   "The kernel test robot is reporting that xfstest which does

     umount ext2 on xfs
     umount xfs

   sequence started failing, for commit 322c4293ecc58110 ("loop: make
   autoclear operation asynchronous") removed a guarantee that fput() of
   backing file is processed before lo_release() from close() returns to
   user mode."

   When autoclear was asynchronous, close() returned before the device
   was unbound and before the backing file was released, breaking
   immediate reuse and subsequent filesystem unmounts.

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-15 17:36               ` Bart Van Assche
@ 2026-09-15 22:38                 ` Tetsuo Handa
  2026-09-17 22:18                   ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-15 22:38 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Christoph Hellwig
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux

On 2026/09/16 2:36, Bart Van Assche wrote:
> On 9/11/26 6:22 PM, Tetsuo Handa wrote:
>> On 2026/09/12 10:02, Bart Van Assche wrote:
>>> On 9/11/26 3:18 PM, Tetsuo Handa wrote:
>>>> On 2026/09/12 5:03, Bart Van Assche wrote:
>>>>> On 9/10/26 2:44 AM, Tetsuo Handa wrote:
>>>>>> On 2026/09/10 4:33, Bart Van Assche wrote:
>>>>>>> __loop_clr_fd() is queued from the lo_post_release() callback and hence
>>>>>>> may be called concurrently with or after another thread has called
>>>>>>> bdev_open(). Hence, lo->lo->state should be checked instead of assuming
>>>>>>> that it equals Lo_rundown.
>>>>>>
>>>>>> No, __loop_clr_fd() is queued from the lo_release() callback.
>>>>>
>>>>> Yes, it is *queued* from the lo_release() callback function but there is
>>>>> no guarantee that __loop_clr_fd() has started before bdev_open() is
>>>>> called again.
>>>>
>>>> Since lo->lo_state was set to Lo_rundown by lo_release(), lo_open() will return -ENXIO.
>>>> What can go wrong if bdev_open() is called again before __loop_clr_fd() starts?
>>>
>>> This breaks LO_FLAGS_AUTOCLEAR, isn't it?
>>
>> Why do you think so?
>>
>>    App1                   App2                   system_long_wq
>>    Calls lo_release().
>>      Schedules __loop_clr_fd().
>>                           Calls lo_open() but fails with -ENXIO.
>>                                                  Starts __loop_clr_fd().
>>    Calls lo_post_release().
>>      Starts waiting for completion of __loop_clr_fd().
>>                                                  Finishes __loop_clr_fd().
>>      Finishes waiting for completion of __loop_clr_fd().
>>
>>    Calls lo_open() again and succeeds.
>>                           Calls lo_open() again and succeeds.
>>
>> App1's open() after close() is succeeding.
>> App2's open() being temporarily failing with -ENXIO should be acceptable.
>> If App2 wants to avoid repeatedly failing with -ENXIO, App2 should use
>> ioctl(LOOP_CTL_GET_FREE) before open().
> 
> Userspace applications and tests (such as mount/umount, losetup, systemd, and xfstests) expect that:
> 1. Teardown of a loop device with LO_FLAGS_AUTOCLEAR completes
>    synchronously during the final close() (in lo_release()), releasing
>    the backing file references (fput()).

App1 is achieving it via the combination of lo_release() and lo_post_release().

> 2. Calling open() immediately after close() succeeds (allowing the loop
>    device to be immediately reallocated, reopened, or configured in
>    Lo_unbound state).

App1 is achieving it via the combination of lo_release() and lo_post_release().

So, what is wrong?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped
  2026-09-16 19:51 [PATCH 0/2] loop: Fix teardown Bart Van Assche
@ 2026-09-16 19:51 ` Bart Van Assche
  2026-09-16 22:47   ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-16 19:51 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Christoph Hellwig, Tetsuo Handa, Nilay Shroff,
	Bart Van Assche

In order to prevent NULL pointer dereferences in lo_rw_aio() when tearing
down a loop device, outstanding I/O must be flushed before clearing the
backing file and device state. However, calling blk_mq_wait_quiesce_done(),
drain_workqueue(), or blk_mq_freeze_queue() with disk->open_mutex held
causes lockdep warnings and potential deadlocks.

Use the .post_release() block device operation to execute __loop_clr_fd()
synchronously after disk->open_mutex has been released by the block layer.
Inside __loop_clr_fd(), outstanding I/O is flushed and the request queue
is frozen before acquiring disk->open_mutex to perform the remaining
device teardown and partition rescans.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/loop.c | 51 ++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 758c20678bf6..d3e686bebcd3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1138,11 +1138,33 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
 static void __loop_clr_fd(struct loop_device *lo)
 {
+	struct gendisk *disk = lo->lo_disk;
 	struct queue_limits lim;
 	struct file *filp;
 	gfp_t gfp = lo->old_gfp_mask;
+	unsigned int memflags;
 	int err;
 
+	WARN_ON_ONCE(READ_ONCE(lo->lo_state) != Lo_rundown);
+
+	/*
+	 * Wait for ongoing loop_queue_rq() calls. Subsequent loop_queue_rq()
+	 * calls which are made after this call returned will see lo->lo_state
+	 * != Lo_bound and return with BLK_STS_IOERR.
+	 */
+	blk_mq_wait_quiesce_done(&lo->tag_set);
+
+	/* loop_queue_rq() queues work on lo->workqueue, hence drain it. */
+	drain_workqueue(lo->workqueue);
+
+	lim = queue_limits_start_update(lo->lo_queue);
+
+	/*
+	 * Freeze the request queue while updating parameters used while
+	 * processing requests.
+	 */
+	memflags = blk_mq_freeze_queue(lo->lo_queue);
+
 	spin_lock_irq(&lo->lo_lock);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
@@ -1153,18 +1175,17 @@ 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.logical_block_size = SECTOR_SIZE;
 	lim.physical_block_size = SECTOR_SIZE;
 	lim.io_min = SECTOR_SIZE;
 	queue_limits_commit_update(lo->lo_queue, &lim);
 
+	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
+
+	/* Serialize against concurrent bdev_open() calls. */
+	mutex_lock(&disk->open_mutex);
+
 	invalidate_disk(lo->lo_disk);
 	loop_sysfs_exit(lo);
 	/* let user-space know about this change */
@@ -1178,9 +1199,6 @@ static void __loop_clr_fd(struct loop_device *lo)
 	/*
 	 * Remove all partitions, including partitions added manually with
 	 * BLKPG, which may exist even if LO_FLAGS_PARTSCAN is not set.
-	 *
-	 * open_mutex has been held already in release path, so don't acquire
-	 * it here.
 	 */
 	err = bdev_disk_changed(lo->lo_disk, false);
 	if (err)
@@ -1197,6 +1215,8 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_flags = 0;
 	if (!part_shift)
 		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
+	mutex_unlock(&disk->open_mutex);
+
 	mutex_lock(&lo->lo_mutex);
 	WRITE_ONCE(lo->lo_state, Lo_unbound);
 	mutex_unlock(&lo->lo_mutex);
@@ -1754,7 +1774,6 @@ static int lo_open(struct gendisk *disk, blk_mode_t mode)
 static void lo_release(struct gendisk *disk)
 {
 	struct loop_device *lo = disk->private_data;
-	bool need_clear = false;
 
 	if (disk_openers(disk) > 0)
 		return;
@@ -1767,11 +1786,14 @@ static void lo_release(struct gendisk *disk)
 	mutex_lock(&lo->lo_mutex);
 	if (lo->lo_state == Lo_bound && (lo->lo_flags & LO_FLAGS_AUTOCLEAR))
 		WRITE_ONCE(lo->lo_state, Lo_rundown);
-
-	need_clear = (lo->lo_state == Lo_rundown);
 	mutex_unlock(&lo->lo_mutex);
+}
+
+static void lo_post_release(struct gendisk *disk)
+{
+	struct loop_device *lo = disk->private_data;
 
-	if (need_clear)
+	if (READ_ONCE(lo->lo_state) == Lo_rundown)
 		__loop_clr_fd(lo);
 }
 
@@ -1791,6 +1813,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,

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped
  2026-09-16 19:51 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Bart Van Assche
@ 2026-09-16 22:47   ` Tetsuo Handa
  2026-09-16 23:05     ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-16 22:47 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Linus Torvalds
  Cc: linux-block, Christoph Hellwig, Nilay Shroff

On 2026/09/17 4:51, Bart Van Assche wrote:
> In order to prevent NULL pointer dereferences in lo_rw_aio() when tearing
> down a loop device, outstanding I/O must be flushed before clearing the
> backing file and device state. However, calling blk_mq_wait_quiesce_done(),
> drain_workqueue(), or blk_mq_freeze_queue() with disk->open_mutex held
> causes lockdep warnings and potential deadlocks.
> 
> Use the .post_release() block device operation to execute __loop_clr_fd()
> synchronously after disk->open_mutex has been released by the block layer.
> Inside __loop_clr_fd(), outstanding I/O is flushed and the request queue
> is frozen before acquiring disk->open_mutex to perform the remaining
> device teardown and partition rescans.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Nacked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Please stop stealing my series and stop proposing broken series.
Instead of spreading lies claiming that my patch series hasn't been working,
please answer my questions.

Your proposed patch introduces a critical concurrency regression that results in
kernel crash (NULL-filp dereference).

Since .post_release is called entirely outside of disk->open_mutex, multiple
non-final close() threads can concurrently invoke lo_post_release() and race
inside __loop_clr_fd() without any serialization. Specifically, Thread 1 can
nullify lo->lo_backing_file under lo->lo_lock. A concurrent Thread 2 will then
fetch lo_backing_file as NULL, yet immediately attempt to dereference it inside
mapping_set_gfp_mask(filp->f_mapping, ...). This triggers an instantaneous NULL
pointer dereference (Oops) and panics the kernel.

The "post_release() operation is intended for performing only idempotent actions
such as flush_work()" in my series is a requirement. Note that concurrently calling
flush_work() from lo_post_release() is safe.

> ---
>  drivers/block/loop.c | 51 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 758c20678bf6..d3e686bebcd3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1138,11 +1138,33 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>  
>  static void __loop_clr_fd(struct loop_device *lo)
>  {
> +	struct gendisk *disk = lo->lo_disk;
>  	struct queue_limits lim;
>  	struct file *filp;
>  	gfp_t gfp = lo->old_gfp_mask;
> +	unsigned int memflags;
>  	int err;
>  
> +	WARN_ON_ONCE(READ_ONCE(lo->lo_state) != Lo_rundown);
> +
> +	/*
> +	 * Wait for ongoing loop_queue_rq() calls. Subsequent loop_queue_rq()
> +	 * calls which are made after this call returned will see lo->lo_state
> +	 * != Lo_bound and return with BLK_STS_IOERR.
> +	 */
> +	blk_mq_wait_quiesce_done(&lo->tag_set);
> +
> +	/* loop_queue_rq() queues work on lo->workqueue, hence drain it. */
> +	drain_workqueue(lo->workqueue);
> +
> +	lim = queue_limits_start_update(lo->lo_queue);
> +
> +	/*
> +	 * Freeze the request queue while updating parameters used while
> +	 * processing requests.
> +	 */
> +	memflags = blk_mq_freeze_queue(lo->lo_queue);
> +
>  	spin_lock_irq(&lo->lo_lock);
>  	filp = lo->lo_backing_file;
>  	lo->lo_backing_file = NULL;
> @@ -1153,18 +1175,17 @@ 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.logical_block_size = SECTOR_SIZE;
>  	lim.physical_block_size = SECTOR_SIZE;
>  	lim.io_min = SECTOR_SIZE;
>  	queue_limits_commit_update(lo->lo_queue, &lim);
>  
> +	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
> +
> +	/* Serialize against concurrent bdev_open() calls. */
> +	mutex_lock(&disk->open_mutex);
> +
>  	invalidate_disk(lo->lo_disk);
>  	loop_sysfs_exit(lo);
>  	/* let user-space know about this change */
> @@ -1178,9 +1199,6 @@ static void __loop_clr_fd(struct loop_device *lo)
>  	/*
>  	 * Remove all partitions, including partitions added manually with
>  	 * BLKPG, which may exist even if LO_FLAGS_PARTSCAN is not set.
> -	 *
> -	 * open_mutex has been held already in release path, so don't acquire
> -	 * it here.
>  	 */
>  	err = bdev_disk_changed(lo->lo_disk, false);
>  	if (err)
> @@ -1197,6 +1215,8 @@ static void __loop_clr_fd(struct loop_device *lo)
>  	lo->lo_flags = 0;
>  	if (!part_shift)
>  		set_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
> +	mutex_unlock(&disk->open_mutex);
> +
>  	mutex_lock(&lo->lo_mutex);
>  	WRITE_ONCE(lo->lo_state, Lo_unbound);
>  	mutex_unlock(&lo->lo_mutex);
> @@ -1754,7 +1774,6 @@ static int lo_open(struct gendisk *disk, blk_mode_t mode)
>  static void lo_release(struct gendisk *disk)
>  {
>  	struct loop_device *lo = disk->private_data;
> -	bool need_clear = false;
>  
>  	if (disk_openers(disk) > 0)
>  		return;
> @@ -1767,11 +1786,14 @@ static void lo_release(struct gendisk *disk)
>  	mutex_lock(&lo->lo_mutex);
>  	if (lo->lo_state == Lo_bound && (lo->lo_flags & LO_FLAGS_AUTOCLEAR))
>  		WRITE_ONCE(lo->lo_state, Lo_rundown);
> -
> -	need_clear = (lo->lo_state == Lo_rundown);
>  	mutex_unlock(&lo->lo_mutex);
> +}
> +
> +static void lo_post_release(struct gendisk *disk)
> +{
> +	struct loop_device *lo = disk->private_data;
>  
> -	if (need_clear)
> +	if (READ_ONCE(lo->lo_state) == Lo_rundown)
>  		__loop_clr_fd(lo);
>  }
>  
> @@ -1791,6 +1813,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,


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped
  2026-09-16 22:47   ` Tetsuo Handa
@ 2026-09-16 23:05     ` Bart Van Assche
  2026-09-17  0:16       ` Tetsuo Handa
  0 siblings, 1 reply; 22+ messages in thread
From: Bart Van Assche @ 2026-09-16 23:05 UTC (permalink / raw)
  To: Tetsuo Handa, Jens Axboe, Linus Torvalds
  Cc: linux-block, Christoph Hellwig, Nilay Shroff

On 9/16/26 3:47 PM, Tetsuo Handa wrote:
> Please stop stealing my series

No, I'm not stealing your work. I credited you in the first patch with a
Suggested-by.

> and stop proposing broken series.

I think we disagree. My view is that your series is broken.
Additionally, I have explained multiple times in detail why I think
your series is broken but you chose to ignore my feedback.

> Since .post_release is called entirely outside of disk->open_mutex, multiple
> non-final close() threads can concurrently invoke lo_post_release() and race
> inside __loop_clr_fd() without any serialization.

This is wrong. The .release() and .post_release() callbacks are only
called once. The following code from fs/file_table.c illustrates this:

void fput(struct file *file)
{
	if (unlikely(file_ref_put(&file->f_ref)))
		__fput_deferred(file);
}

/* the real guts of fput() - releasing the last reference to file */
static void __fput(struct file *file)
{
	[ ... ]
	if (file->f_op->release)
		file->f_op->release(inode, file);
	[ ... ]
}

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped
  2026-09-16 23:05     ` Bart Van Assche
@ 2026-09-17  0:16       ` Tetsuo Handa
  0 siblings, 0 replies; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-17  0:16 UTC (permalink / raw)
  To: Bart Van Assche, Jens Axboe, Linus Torvalds
  Cc: linux-block, Christoph Hellwig, Nilay Shroff

On 2026/09/17 8:05, Bart Van Assche wrote:
>> Since .post_release is called entirely outside of disk->open_mutex, multiple
>> non-final close() threads can concurrently invoke lo_post_release() and race
>> inside __loop_clr_fd() without any serialization.
> 
> This is wrong. The .release() and .post_release() callbacks are only
> called once. The following code from fs/file_table.c illustrates this:
> 
> void fput(struct file *file)
> {
>     if (unlikely(file_ref_put(&file->f_ref)))
>         __fput_deferred(file);
> }
> 
> /* the real guts of fput() - releasing the last reference to file */
> static void __fput(struct file *file)
> {
>     [ ... ]
>     if (file->f_op->release)
>         file->f_op->release(inode, file);
>     [ ... ]
> }

You are misunderstanding bdev_release() concurrency for the same "struct gendisk"
even if __fput() for the same "struct file" does not run concurrently.

Two processes can obtain file descriptor of the same loop device using open() syscall.
Two processes can release that file descriptor using close() syscall.

While __fput() is serialized within each thread because it runs under the task work context,
lo_post_release() (for the same "struct loop_device" which can be derived from "struct gendisk")
is not serialized between two processes.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-15 22:38                 ` Tetsuo Handa
@ 2026-09-17 22:18                   ` Tetsuo Handa
  2026-09-18  0:59                     ` Bart Van Assche
  0 siblings, 1 reply; 22+ messages in thread
From: Tetsuo Handa @ 2026-09-17 22:18 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux, Jens Axboe,
	Christoph Hellwig, Nilay Shroff

On 2026/09/16 7:38, Tetsuo Handa wrote:
>> Userspace applications and tests (such as mount/umount, losetup, systemd, and xfstests) expect that:
>> 1. Teardown of a loop device with LO_FLAGS_AUTOCLEAR completes
>>    synchronously during the final close() (in lo_release()), releasing
>>    the backing file references (fput()).
> 
> App1 is achieving it via the combination of lo_release() and lo_post_release().
> 
>> 2. Calling open() immediately after close() succeeds (allowing the loop
>>    device to be immediately reallocated, reopened, or configured in
>>    Lo_unbound state).
> 
> App1 is achieving it via the combination of lo_release() and lo_post_release().
> 
> So, what is wrong?

Since you are mimicking my patch, you agreed that there is no problem with my patch, don't you?
Then, why not respond to my patch with Acked-by or Reviewed-by line?


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped.
  2026-09-17 22:18                   ` Tetsuo Handa
@ 2026-09-18  0:59                     ` Bart Van Assche
  0 siblings, 0 replies; 22+ messages in thread
From: Bart Van Assche @ 2026-09-18  0:59 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Al Viro, Andrew Morton, Brian Foster, Damien Le Moal,
	Hillf Danton, Markus Elfring, Ming Lei, Qu Wenruo, Tao Cui,
	kernel test robot, linux-block, rust-for-linux, Jens Axboe,
	Christoph Hellwig, Nilay Shroff

On 9/17/26 3:18 PM, Tetsuo Handa wrote:
> Since you are mimicking my patch, you agreed that there is no problem with my patch, don't you?
> Then, why not respond to my patch with Acked-by or Reviewed-by line?

v2 of your patch has been integrated in this series with you as author:

https://lore.kernel.org/linux-block/cover.1789679858.git.bvanassche@acm.org/

Bart.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2026-09-18  0:59 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-09 10:48 [PATCH 1/2] block: Add post_release() operation Tetsuo Handa
2026-09-09 10:49 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Tetsuo Handa
2026-09-09 19:33   ` Bart Van Assche
2026-09-10  9:44     ` Tetsuo Handa
2026-09-11 20:03       ` Bart Van Assche
2026-09-11 22:18         ` Tetsuo Handa
2026-09-12  1:02           ` Bart Van Assche
2026-09-12  1:22             ` Tetsuo Handa
2026-09-15 17:36               ` Bart Van Assche
2026-09-15 22:38                 ` Tetsuo Handa
2026-09-17 22:18                   ` Tetsuo Handa
2026-09-18  0:59                     ` Bart Van Assche
2026-09-10 11:53 ` [PATCH 1/2] block: Add post_release() operation Gary Guo
2026-09-10 12:14   ` Tetsuo Handa
2026-09-11 19:54 ` Bart Van Assche
2026-09-11 22:34   ` Tetsuo Handa
2026-09-12  0:31     ` Bart Van Assche
2026-09-12  1:43       ` Tetsuo Handa
  -- strict thread matches above, loose matches on Subject: below --
2026-09-16 19:51 [PATCH 0/2] loop: Fix teardown Bart Van Assche
2026-09-16 19:51 ` [PATCH 2/2] loop: Perform __loop_clr_fd() after disk->open_mutex is dropped Bart Van Assche
2026-09-16 22:47   ` Tetsuo Handa
2026-09-16 23:05     ` Bart Van Assche
2026-09-17  0:16       ` Tetsuo Handa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox