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

[Tetsuo Handa]

Thank you for joining to this thread, Tao.

On 2026/08/31 23:11, Tao Cui wrote:
> From: Tao Cui <cuitao@xxxxxxxxxx>
> 
> Hi Tetsuo, Bart,
> 
>> +	/* Step 1: Flush all outstanding I/O, without open_mutex held. */
>> +	/*
>> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
>> +	 * wait for already started loop_queue_rq() to complete.
>> +	 */
>> +	synchronize_rcu();
> 
> Your reply to Bart says loop_queue_rq() is called with RCU read
> lock, but I don't see one.  The two call sites of ->queue_rq() are
> blk_mq_dispatch_rq_list() and __blk_mq_issue_directly(), and
> neither is wrapped in rcu_read_lock().  Direct issue from
> blk_mq_submit_bio() runs in process context with no RCU read-side
> critical section, so synchronize_rcu() does not wait for a
> loop_queue_rq() that is already running there.  Only the softirq
> dispatch path is an implicit RCU reader.

You can read sashiko's raw logs at https://sashiko.dev/#/log/49552 .

  Okay, let's break this down. My initial thought is to examine how the `blk_mq_run_dispatch_ops`
  macro is implemented. Specifically, how does it handle RCU read locks? It wraps the provided
  `dispatch_ops` in either `rcu_read_lock()` or `srcu_read_lock()`, depending on the `BLK_MQ_F_BLOCKING`
  flag. The loop device, however, is set up *without* this `BLK_MQ_F_BLOCKING` flag. Therefore,
  `loop_queue_rq` executes under the protection of `rcu_read_lock()`.

  Now, the critical question: Does the `synchronize_rcu()` call in `__loop_clr_fd` effectively address
  the potential race conditions? Since `loop_queue_rq()` is indeed RCU-protected, `synchronize_rcu()` will
  correctly wait for all currently executing instances of `loop_queue_rq()` to complete. Furthermore,
  the `lo_state` transition to `Lo_rundown` before calling `__loop_clr_fd` ensures that any *new* calls to
  `loop_queue_rq()` will see this state and return an error. This perfectly fences the execution of
  `loop_queue_rq()`. Moreover, `drain_workqueue()` then makes sure all queued works are completed, and
  these are scheduled by `loop_queue_rq()`. This is great; all scheduled works are completed before
  clearing `lo->lo_backing_file`.

> 
> The window is still closed by the steps below, so this is not a
> correctness bug, but the synchronize_rcu() is not doing what the
> comment claims.  blk_mq_quiesce_queue() +
> blk_mq_wait_quiesce_done(), as Bart suggested, would express the
> intent directly.

synchronize_rcu() + drain_workqueue() can be implied by blk_mq_freeze_queue() + blk_mq_unfreeze_queue().
But why are you talking about blk_mq_quiesce_queue() + blk_mq_wait_quiesce_done() ?

> 
>> +	/*
>> +	 * 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));
> 
> About this step, in your follow-up you wrote:
> 
>>   (1) since we are in lo_release() with disk_openers(disk) == 0, the activity of
>>       incrementing/decrementing q_usage_counter (incremented before loop_queue_rq()
>>       is called, and decremented after loop_queue_rq() returned BLK_STS_IOERR)) will
>>       cease shortly
> 
> The decrement timing here is only true for the error path.  For
> BLK_STS_OK the reference is held until the request is freed, which
> is what makes blk_mq_freeze_queue() wait for requests that already
> passed the state check, including the loop workqueue worker that
> completes them.  That is the property step 1 relies on, and it is
> worth stating in the comment.
> 
> What is still open is Bart's question about io_uring fixed files:
> if submissions can continue after the last close, "cease shortly"
> does not hold, and it is the freeze wait that actually drains them.

If submissions can continue _forever_ despite disk_openers(disk) == 0, what
mechanism was preventing this problem from occurring until Linux 7.0 ?

> 
>> +	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);
>> +	}
> 
> With teardown now asynchronous, between the last close()
> returning and the work item finishing, lo_open() and
> LOOP_CONFIGURE return -ENXIO.  That is the same behavior change
> that led to the revert of the earlier attempt (bf23747ee053,
> xfs/259).  Moving the xfstests side to the tests is one thing, but
> userspace that closes a loop device and immediately reconfigures
> it now needs to handle a transient -ENXIO.  Is that acceptable, or
> should the retry happen in the kernel?

Does whether the teardown being synchronous or asynchronous matter so much?

I don't think we can control when e.g. udev-worker becomes the thread
who actually calls __loop_clr_fd()
( https://lkml.kernel.org/r/9f8b5ab0-efbc-4cf3-a1f8-b43377416946@xxxxxxxxxxxxxxxxxxx ).
Even if an existing user app calls close() immediately followed by open(),
we can't prove that __loop_clr_fd() is synchronously called by that user app
because udev-worker can jump in and udev-worker becomes the thread who actually
calls __loop_clr_fd().

   An user app     udev-worker
   -----------     -----------
                    open()
    close()
    open() // => succeeds due to Lo_bound state
    ioctl(LOOP_CONFIGURE) // => fails with -EBUSY due to Lo_bound state
    close() // <= gives up due to ioctl() failure
                    close() // => becomes  Lo_rundown and __loop_clr_fd() is called

A claim that mentions that the kernel is unable to release forever due to a refcount
leak bug is valid. But a claim that mentions that the kernel cannot prove that
this -ENXIO or -EBUSY problem never happens is invalid. Programs that use the loop
device have to be prepared for transient errors.