Re: [PATCH] block: don't dereference request after flush insertion

[Jens Axboe <axboe@kernel.dk>]

On 10/17/21 8:11 PM, Ming Lei wrote:
> On Mon, Oct 18, 2021 at 10:02:32AM +0800, Ming Lei wrote:
>> On Sun, Oct 17, 2021 at 07:50:24PM -0600, Jens Axboe wrote:
>>> On 10/17/21 7:49 PM, Ming Lei wrote:
>>>> On Sat, Oct 16, 2021 at 07:35:39PM -0600, Jens Axboe wrote:
>>>>> We could have a race here, where the request gets freed before we call
>>>>> into blk_mq_run_hw_queue(). If this happens, we cannot rely on the state
>>>>> of the request.
>>>>>
>>>>> Grab the hardware context before inserting the flush.
>>>>>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index 2197cfbf081f..22b30a89bf3a 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -2468,9 +2468,10 @@ void blk_mq_submit_bio(struct bio *bio)
>>>>>  	}
>>>>>  
>>>>>  	if (unlikely(is_flush_fua)) {
>>>>> +		struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>>>>>  		/* Bypass scheduler for flush requests */
>>>>>  		blk_insert_flush(rq);
>>>>> -		blk_mq_run_hw_queue(rq->mq_hctx, true);
>>>>> +		blk_mq_run_hw_queue(hctx, true);
>>>>
>>>> If the request is freed before running queue, the request queue could
>>>> be released and the hctx may be freed.
>>>
>>> No, we still hold a queue enter ref.
>>
>> But that one is released when rq is freed since ac7c5675fa45 ("blk-mq: allow
>> blk_mq_make_request to consume the q_usage_counter reference"), isn't
>> it?
> 
> With commit ac7c5675fa45, any reference to hctx after queuing request could
> lead to UAF in the code path of blk_mq_submit_bio(). Maybe we need to grab
> two ref in queue enter, and release one after the bio is submitted.

I'd rather audit and see if there are any, because extra get+put isn't
exactly free.

-- 
Jens Axboe