Re: [PATCH v7 3/5] blk-mq: Fix races between iterating over requests and freeing requests

[Ming Lei <ming.lei@redhat.com>]

On Wed, Apr 21, 2021 at 08:54:30PM -0700, Bart Van Assche wrote:
> On 4/21/21 8:15 PM, Ming Lei wrote:
> > On Tue, Apr 20, 2021 at 05:02:33PM -0700, Bart Van Assche wrote:
> >> +static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >> +{
> >> +	struct bt_tags_iter_data *iter_data = data;
> >> +	struct blk_mq_tags *tags = iter_data->tags;
> >> +	bool res;
> >> +
> >> +	if (iter_data->flags & BT_TAG_ITER_MAY_SLEEP) {
> >> +		down_read(&tags->iter_rwsem);
> >> +		res = __bt_tags_iter(bitmap, bitnr, data);
> >> +		up_read(&tags->iter_rwsem);
> >> +	} else {
> >> +		rcu_read_lock();
> >> +		res = __bt_tags_iter(bitmap, bitnr, data);
> >> +		rcu_read_unlock();
> >> +	}
> >> +
> >> +	return res;
> >> +}
> > 
> > Holding one rwsem or rcu read lock won't avoid the issue completely
> > because request may be completed remotely in iter_data->fn(), such as
> > nbd_clear_req(), nvme_cancel_request(), complete_all_cmds_iter(),
> > mtip_no_dev_cleanup(), because blk_mq_complete_request() may complete
> > request in softirq, remote IPI, even wq, and the request is still
> > referenced in these contexts after bt_tags_iter() returns.
> 
> The rwsem and RCU read lock are used to serialize iterating over
> requests against blk_mq_sched_free_requests() calls. I don't think it
> matters for this patch from which context requests are freed.

Requests still can be referred in other context after blk_mq_wait_for_tag_iter()
returns, then follows freeing request pool. And use-after-free exists too, doesn't it?

Thanks,
Ming