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

[Tao Cui]

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.

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.

> +	/*
> +	 * 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 (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?

Thanks,
Tao