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

[Tetsuo Handa]

On 2026/08/29 1:29, Bart Van Assche wrote:
> 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.

Excuse me, but this ordering is correct (reviewed by Gemini and sashiko).

>                                            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.

I and Gemini cannot catch what you want to say here.

> 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?

Calling synchronize_rcu() makes sure that no more queue_work() calls are
made from loop_queue_work() from loop_queue_rq(). Since loop_queue_rq() is
called with RCU read lock, subsequent loop_queue_rq() calls which are made
after synchronize_rcu() returned shall see lo->lo_state != Lo_bound and
return with BLK_STS_IOERR.

> 
>> +    /*
>> +     * 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.

Again, I and Gemini cannot catch what you want to say here.

Calling drain_workqueue() after synchronize_rcu() makes sure that no more
loop_handle_cmd() calls are made from loop_process_work() from loop_workfn()
and loop_rootcg_workfn().

Calling blk_mq_freeze_queue() after drain_workqueue() after synchronize_rcu()
does wait for completion of all pending I/O requests which has been scheduled
via loop_queue_rq(), by waiting for q_usage_counter to reach 0. Also, this
synchronize_rcu() => drain_workqueue() => blk_mq_freeze_queue() ordering
guarantees that q_usage_counter won't be incremented again after it once
reached 0, due to the lo->lo_state != Lo_bound check in loop_queue_rq().
This makes it possible to call blk_mq_unfreeze_queue() immediately after
blk_mq_freeze_queue().

> 
>> +    /* 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.

Did you mean WARN_ON_ONCE(lo->lo_state != Lo_rundown) ?

> 
>> @@ -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);
> 

Doing so breaks the whole protection provided by the Lo_rundown state. As soon
as lo->lo_state becomes Lo_unbound, lo_open() will succeed and loop_configure()
will start changing an lo object before WQ context cleans up that lo object.

>> @@ -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.

That is OK. But after you agreed that my patch works as expected.

> 
> Why system_long_wq instead of lo->workqueue?

That is a deadlock. We can't flush a work in lo->workqueue from inside
WQ callback function where that work is associated with.