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

[Bart Van Assche]

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

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

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

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

Thanks,

Bart.

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

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

+	/*
+	 * Prevent that new asynchronous I/O is submitted while queue limits
+	 * are being modified.
+	 */
+	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
+
+	/* Wait until asynchronous I/O has finished. */
+	memflags = blk_mq_freeze_queue(q);
+	blk_mq_unfreeze_queue(q, memflags);
+
+	/* Wait until I/O dispatching has finished. */
+	blk_mq_quiesce_queue(q);
+	blk_mq_unquiesce_queue(q);
+
+	/* Wait until all I/O-related work has finished. */
+	flush_workqueue(lo->workqueue);
+
 	mutex_lock(&lo->lo_mutex);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
@@ -1168,17 +1187,12 @@ static void __loop_clr_fd(struct loop_device *lo)
 	lo->lo_sizelimit = 0;
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);

-	/*
-	 * Reset the block size to the default.
-	 *
-	 * No queue freezing needed because this is called from the final
-	 * ->release call only, so there can't be any outstanding I/O.
-	 */
-	lim = queue_limits_start_update(lo->lo_queue);
+	/* Reset the block size to the default. */
+	lim = queue_limits_start_update(q);
 	lim.logical_block_size = SECTOR_SIZE;
 	lim.physical_block_size = SECTOR_SIZE;
 	lim.io_min = SECTOR_SIZE;
-	queue_limits_commit_update(lo->lo_queue, &lim);
+	queue_limits_commit_update(q, &lim);

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

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