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

From: Ming Lei <ming.lei@redhat.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Daniel Wagner <dwagner@suse.de>,
	Khazhismel Kumykov <khazhy@google.com>,
	Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	Hannes Reinecke <hare@suse.de>,
	Johannes Thumshirn <johannes.thumshirn@wdc.com>,
	John Garry <john.garry@huawei.com>,
	linux-scsi@vger.kernel.org
Subject: Re: [PATCH v7 3/5] blk-mq: Fix races between iterating over requests and freeing requests
Date: Tue, 27 Apr 2021 08:11:59 +0800	[thread overview]
Message-ID: <YIdWz8C5eklPvEiV@T590> (raw)
In-Reply-To: <d1d3c068-4446-145b-34c6-12fa1f30d4da@acm.org>

On Mon, Apr 26, 2021 at 09:29:54AM -0700, Bart Van Assche wrote:
> On 4/24/21 5:09 PM, Ming Lei wrote:
> > Terminating all pending commands can't avoid the issue wrt. request UAF,
> > so far blk_mq_tagset_wait_completed_request() is used for making sure
> > that all pending requests are really aborted.
> > 
> > However, blk_mq_wait_for_tag_iter() still may return before
> > blk_mq_wait_for_tag_iter() is done because blk_mq_wait_for_tag_iter()
> > supposes all request reference is just done inside bt_tags_iter(),
> > especially .iter_rwsem and read rcu lock is added in bt_tags_iter().
> 
> Hi Ming,
> 
> I think that we agree that completing a request from inside a tag
> iteration callback function may cause the request completion to happen
> after tag iteration has finished. This can happen because
> blk_mq_complete_request() may redirect completion processing to another
> CPU via an IPI.
> 
> But can this mechanism trigger a use-after-free by itself? If request
> completion is redirected to another CPU, the request is still considered
> pending and request queue freezing won't complete. Request queue
> freezing will only succeed after __blk_mq_free_request() has been called
> because it is __blk_mq_free_request() that calls blk_queue_exit().
> 
> In other words, do we really need the new
> blk_mq_complete_request_locally() function?
> 
> Did I perhaps miss something?

Please see the example in the following link:

https://lore.kernel.org/linux-block/20210421000235.2028-4-bvanassche@acm.org/T/#m4d7bc9aa01108f03d5b4b7ee102eb26eb0c778aa

In short:

1) One async completion from interrupt context is pending, and one request
isn't really freed yet because its driver tag isn't released.

2) Meantime iterating code still can visit this request, and ->fn() schedules
a new remote completion, and returns.

3) The scheduled async completion in 1) is really done now,
but the scheduled async completion in 2) isn't started or done yet

4) queue becomes frozen because of one pending elevator switching, so
sched request pool is freed since there isn't any pending iterator ->fn()

5) request UAF is caused when running the scheduled async completion in 2)
now.

Thanks,
Ming