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

[Bart Van Assche]

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

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

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

Releasing and reacquiring disk->open_mutex from __loop_clr_fd() seems
risky to me. There is plenty of code in block/bdev.c that assumes that
disk->open_mutex is not released by lo_release().

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

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

Thanks,

Bart.

loop: Serialize I/O and queue limits updates

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

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

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

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

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

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