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

[Bart Van Assche <bvanassche@acm.org>]

On 8/28/26 8:53 AM, Tetsuo Handa wrote:
> +	 * wait for already started loop_queue_rq() to complete.
> +	 */
> +	synchronize_rcu();

Calling synchronize_rcu() to wait for ongoing loop_queue_rq() calls to
complete won't work if anyone would set BLK_MQ_F_BLOCKING for the
request queues created by the loop driver. Please use the block-layer
APIs instead of open-coding these. I'm referring to
blk_mq_quiesce_queue() and blk_mq_wait_quiesce_done().

Freezing the request queue must happen before waiting for ongoing
loop_queue_rq() calls to finish.

Calling synchronize_rcu() does not prevent new I/O to be submitted. What
prevents io_uring to submit more I/O asynchronously, e.g. if a file
descriptor that refers to a loop device instance has been registered in
the fixed-file table?

> +	/*
> +	 * Now that no more works are scheduled by loop_queue_rq(),
> +	 * wait for already scheduled works to complete.
> +	 */
> +	drain_workqueue(lo->workqueue);
> +	/*
> +	 * Now that no more AIO requests are scheduled by lo_rw_aio(),
> +	 * wait for already started AIO to complete.
> +	 */
> +	blk_mq_unfreeze_queue(lo->lo_queue, blk_mq_freeze_queue(lo->lo_queue));

Freezing the request queue must happen before lo->workqueue is drained.

> +	/* Step 2: Perform remaining cleanup, with open_mutex held. */
> +	mutex_lock(&disk->open_mutex);

After having obtained disk->open_mutex, lease add something like the
following: WARN_ON_ONCE(lo->lo_state == Lo_bound). Even if this
condition can't be triggered today, this may help with detecting bugs in
future loop driver changes.

> @@ -1168,8 +1186,6 @@ static void __loop_clr_fd(struct loop_device *lo)
>   	/* let user-space know about this change */
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   	mapping_set_gfp_mask(filp->f_mapping, gfp);
> -	/* This is safe: open() is still holding a reference. */
> -	module_put(THIS_MODULE);
>   
>   	disk_force_media_change(lo->lo_disk); 

I don't think that it's acceptable to invoke __loop_clr_fd()
asynchronously in its entirety. I think at least the following code
should be executed synchronously from lo_release():

	loop_sysfs_exit(lo);

	mutex_lock(&lo->lo_mutex);
	WRITE_ONCE(lo->lo_state, Lo_unbound);
	mutex_unlock(&lo->lo_mutex);

> @@ -1769,8 +1797,20 @@ static void lo_release(struct gendisk *disk)
>   	need_clear = (lo->lo_state == Lo_rundown);
>   	mutex_unlock(&lo->lo_mutex);
>   
> -	if (need_clear)
> -		__loop_clr_fd(lo);
> +	/*
> +	 * In order to flush pending I/O requests before clearing the backing
> +	 * device, defer __loop_clr_fd() to WQ context. The Lo_rundown state
> +	 * guarantees that lo_open() will fail with -ENXIO.
> +	 */
> +	if (need_clear) {
> +		/*
> +		 * Grab all references that will be dropped as soon as
> +		 * returning from lo_release() and releasing disk->open_mutex.
> +		 */
> +		get_device(disk_to_dev(disk));
> +		__module_get(disk->fops->owner);
> +		queue_work(system_long_wq, &lo->lo_clr_work);
> +	}
>   }

Please convert the above code to the "early return" style that is used
elsewhere in the kernel.

Why system_long_wq instead of lo->workqueue?

Thanks,

Bart.