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

[Tao Cui <cui.tao@linux.dev>]

Hi Bart, Tetsuo,

Thanks for the detailed replies, and to Tetsuo for the sashiko
log.

在 2026/8/31 23:49, Tetsuo Handa 写道:
> Thank you for joining to this thread, Tao.
> 
> On 2026/08/31 23:11, Tao Cui wrote:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> 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`.
> 

Bart, on 8/31 you wrote:
> This is the code that protects .queue_rq() implementations like
> loop_queue_rq() with an rcu_read_lock() / rcu_read_unlock() pair:
>
> /* run the code block in @dispatch_ops with rcu/srcu read lock held */
> #define __blk_mq_run_dispatch_ops(q, check_sleep, dispatch_ops)	\
> ...
> 	} else {						\
> 		rcu_read_lock();				\
> 		(dispatch_ops);					\
> 		rcu_read_unlock();				\
> 	}							\
> } while (0)

You are both right and I was wrong.  I stopped at
__blk_mq_issue_directly() and blk_mq_dispatch_rq_list() and missed
that every caller goes through this macro.  So synchronize_rcu()
does wait for a running loop_queue_rq() with the current loop
setup.  I withdraw that comment.

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

That was Bart's suggestion from his earlier review, and I passed
it on while assuming the synchronize_rcu() was unfounded.  With
the RCU pairing confirmed, there is nothing left to replace and
the quiesce suggestion is moot.

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

The decrement part I would still reword: for BLK_STS_OK the
reference is held until the request is freed, and that is the
property blk_mq_freeze_queue() relies on.  But you are right that
the activity ceases, and for a simpler reason than the freeze:

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

An io_uring fixed file holds a reference to the struct file, so
bd_openers never reaches zero and lo_release() does not start
while such a submission path exists.  Any submission needs an
open block device file (or a mount holding it), so "openers == 0
but submissions continue" needs no additional mechanism.  I'll
drop the io_uring part.

>>
>>> +	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@I-love.SAKURA.ne.jp ).
> 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.

Your udev-worker sequence shows that synchronous completion was
never guaranteed and -EBUSY or -ENXIO transients were already
possible.  I accept that.  The remaining difference is only that
the window now exists after every close instead of requiring an
interleaving close from another opener, and if programs must
tolerate transient errors anyway, that is not a blocker.

Tao