rust-for-linux.vger.kernel.org archive mirror
 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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
  0 siblings, 0 replies; 14+ 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] 14+ 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; 14+ 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] 14+ messages in thread

end of thread, other threads:[~2026-09-12  1:44 UTC | newest]

Thread overview: 14+ 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-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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).