Re: [PATCH v2] loop: Fix NULL pointer dereference by synchronizing lo_release and loop_queue_rq

[Ming Lei <tom.leiming@gmail.com>]

On Tue, May 19, 2026 at 06:27:11PM +0900, Tetsuo Handa wrote:
> On 2026/05/19 9:40, Andrew Morton wrote:
> > AI review asked a couple of questions:
> > 	https://sashiko.dev/#/patchset/9b2032d6-3f36-4d2b-8128-985c08a4fa37@I-love.SAKURA.ne.jp
> 
> To: gemini/gemini-3.1-pro-preview
> 
> Thank you for your valuable feedback. Your point about asynchronous I/O completing after drain_workqueue()
> and potentially causing a UAF at file_inode() from kiocb_end_write() from lo_rw_aio_do_completion() is correct.
> The drain_workqueue() alone does not wait for in-flight AIOs that have already returned -EIOCBQUEUED. However,
> I'm not convinced that use of blk_mq_freeze_queue() inside __loop_clr_fd() where disk->open_mutex was already
> held by bdev_release() is absolutely deadlock-free.
> 
> 1. VFS and Block Layer Lock Contention:
>    __loop_clr_fd() is exclusively invoked from the lo_release() path during the final close of the device.
>    At this stage, the block layer is holding disk->open_mutex. If we call blk_mq_freeze_queue() here, it will
>    synchronously wait for all in-flight AIOs to complete. However, the completion paths of those in-flight AIOs
>    (or subsequent metadata processing in the underlying filesystem) may attempt to acquire resources or execute
>    code paths that depend on the very same device state or open/close status. This creates a circular dependency,
>    leading to an unrecoverable hang.
> 
> 2. Memory Reclaim Deadlock:
>    blk_mq_freeze_queue() blocks until the queue's usage counter drops to zero. If an in-flight AIO requires memory
>    allocation for metadata updates upon completion, and the system is under heavy memory pressure, it can trigger
>    direct memory reclaim. If the reclaim path attempts to sync other buffers or interact with the frozen loop
>    device/queue, a circular deadlock occurs.
> 
> Therefore, I would like to choose SRCU-based synchronization instead of blk_mq_freeze_queue().
> 
> * Locking: We call srcu_read_lock(&loop_io_srcu) only for asynchronous paths (cmd->use_aio) immediately
>   before submitting the I/O to the underlying filesystem in lo_rw_aio().
> 
> * Unlocking: The reader lock is released via srcu_read_unlock() at the very end of the AIO completion handler
>   (lo_rw_aio_do_completion()).
> 
> * Synchronization: We place synchronize_srcu(&loop_io_srcu) immediately after drain_workqueue() in __loop_clr_fd().
> 
> I think that this guarantees that __loop_clr_fd() safely blocks until all pending AIO callbacks are 100% completed,
> fully eliminating the UAF risk and ensuring the safety of the subsequent mapping_set_gfp_mask() and fput(), while
> remaining entirely deadlock-free.
> 
> What do you think about this approach?
> 
>  drivers/block/loop.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 0000913f7efc..7c3961f3cbc9 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -80,6 +80,7 @@ struct loop_cmd {
>  	struct list_head list_entry;
>  	bool use_aio; /* use AIO interface to handle I/O */
>  	atomic_t ref; /* only for aio */
> +	int srcu_idx;
>  	long ret;
>  	struct kiocb iocb;
>  	struct bio_vec *bvec;
> @@ -93,6 +94,7 @@ struct loop_cmd {
>  static DEFINE_IDR(loop_index_idr);
>  static DEFINE_MUTEX(loop_ctl_mutex);
>  static DEFINE_MUTEX(loop_validate_mutex);
> +DEFINE_SRCU(loop_io_srcu);
>  
>  /**
>   * loop_global_lock_killable() - take locks for safe loop_validate_file() test
> @@ -327,6 +329,8 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
>  		kiocb_end_write(&cmd->iocb);
>  	if (likely(!blk_should_fake_timeout(rq->q)))
>  		blk_mq_complete_request(rq);
> +	if (cmd->use_aio)
> +		srcu_read_unlock(&loop_io_srcu, cmd->srcu_idx);
>  }
>  
>  static void lo_rw_aio_complete(struct kiocb *iocb, long ret)
> @@ -392,6 +396,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>  	if (cmd->use_aio) {
>  		cmd->iocb.ki_complete = lo_rw_aio_complete;
>  		cmd->iocb.ki_flags = IOCB_DIRECT;
> +		cmd->srcu_idx = srcu_read_lock(&loop_io_srcu);
>  	} else {
>  		cmd->iocb.ki_complete = NULL;
>  		cmd->iocb.ki_flags = 0;
> @@ -1118,6 +1123,22 @@ static void __loop_clr_fd(struct loop_device *lo)
>  	struct file *filp;
>  	gfp_t gfp = lo->old_gfp_mask;
>  
> +	/*
> +	 * Now that loop_queue_rq() sees lo->lo_state != Lo_bound,
> +	 * wait for already started loop_queue_rq() to complete.
> +	 */
> +	synchronize_rcu();
> +	/*
> +	 * 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.
> +	 */
> +	synchronize_srcu(&loop_io_srcu);

The IO after close(loop) should be from writeback. rcu/sruc isn't necessary,
please see the patch posted in another thread:

https://lore.kernel.org/linux-block/agxJdUf1b0JSDAux@fedora/

in which the check on lo->lo_state is moved to loop_handle_cmd(), meantime
drain_workqueue() is added for draining in-flight workers.

Thanks,
Ming