Re: v4.20-rc6: Sporadic use-after-free in bt_iter()

[Jens Axboe <axboe@kernel.dk>]

On 12/20/18 2:31 PM, Bart Van Assche wrote:
> On Thu, 2018-12-20 at 14:26 -0700, Jens Axboe wrote:
>> On 12/20/18 2:23 PM, Bart Van Assche wrote:
>>> On Thu, 2018-12-20 at 14:00 -0700, Jens Axboe wrote:
>>>> On 12/20/18 1:56 PM, Bart Van Assche wrote:
>>>>> @@ -96,6 +97,9 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
>>>>>  {
>>>>>  	struct mq_inflight *mi = priv;
>>>>>  
>>>>> +	if (rq->q != mi->q)
>>>>> +		return;
>>>>
>>>> Aren't you back to square one with this one, if the tags are shared? You
>>>> can't dereference it before you know it matches.
>>>
>>> My patch can only work if the new rq->q = NULL assignment in __blk_mq_free_request()
>>> is executed before the request tag is freed and if freeing a tag does not happen
>>> concurrently with any bt_iter() call. Would you accept that I add a seqlock to avoid
>>> this scenario?
>>
>> Ugh no, let's not go that far. Why not just use my approach that avoids
>> any need to dereference rq, unless we know it belongs to the queue in
>> question? I think that's cheaper than resetting ->queue as well when the
>> rq completes, I'm always wary of adding new stores in the completion
>> path.
> 
> I think there is a race condition in bt_iter() in your approach:
> tags->rqs[bitnr].queue can change after it has been read and that can
> cause a request that is not associated with hctx->queue to be passed
> to iter_data->fn(). Since 'fn' will access '*rq' I think that with
> your patch a use-after-free can occur similar to the one reported at
> the start of this e-mail thread. Your patch may make it harder to
> trigger that issue though.

Yeah, I don't think it's bullet proof either, it just closes the gap.
I'm fine with fiddling with the tag iteration. On top of what I sent, we
could have tag iteration hold the RCU read lock, and then we just need
to ensure that the tags are freed with RCU.

-- 
Jens Axboe