Virtualization - Sep 2014 - blk-mq crash under KVM in multiqueue block code (with virtio-blk and ext4)

On 2014-09-17 07:52, Ming Lei wrote:
> On Wed, 17 Sep 2014 14:00:34 +0200
> David Hildenbrand <dahi at linux.vnet.ibm.com> wrote:
>
>>>>>> Does anyone have an idea?
>>>>>> The request itself is completely filled with cc
>>>>>
>>>>> That is very weird, the 'rq' is got from
hctx->tags,  and rq should be
>>>>> valid, and rq->q shouldn't have been changed even
though it was
>>>>> double free or double allocation.
>>>>>
>>>>>> I am currently asking myself if blk_mq_map_request
should protect against softirq here but I cant say for sure,as I have never
looked into that code before.
>>>>>
>>>>> No, it needn't the protection.
>>>>>
>>>>> Thanks,
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
kvm" in
>>>> the body of a message to majordomo at vger.kernel.org
>>>> More majordomo info at 
http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>> Digging through the code, I think I found a possible cause:
>>
>> tags->rqs[..] is not initialized with zeroes (via alloc_pages_node
in
>> blk-mq.c:blk_mq_init_rq_map()).
>
> Yes, it may cause problem when the request is allocated at the 1st time,
> and timeout handler may comes just after the allocation and before its
> initialization, then oops triggered because of garbage data in the request.
>
> --
>  From ffd0824b7b686074c2d5d70bc4e6bba3ba56a30c Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei at canonical.com>
> Date: Wed, 17 Sep 2014 21:00:34 +0800
> Subject: [PATCH] blk-mq: initialize request before the 1st allocation
>
> Otherwise the request can be accessed from timeout handler
> just after its 1st allocation from tag pool and before
> initialization in blk_mq_rq_ctx_init(), so cause oops since
> the request is filled up with garbage data.
>
> Signed-off-by: Ming Lei <ming.lei at canonical.com>
> ---
>   block/blk-mq.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 4aac826..d24673f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -514,6 +514,10 @@ struct request *blk_mq_tag_to_rq(struct blk_mq_tags
*tags, unsigned int tag)
>   {
>   	struct request *rq = tags->rqs[tag];
>
> +	/* uninitialized request */
> +	if (!rq->q || rq->tag == -1)
> +		return rq;
> +
>   	if (!is_flush_request(rq, tag))
>   		return rq;
>
> @@ -1401,6 +1405,12 @@ static struct blk_mq_tags *blk_mq_init_rq_map(struct
blk_mq_tag_set *set,
>   		left -= to_do * rq_size;
>   		for (j = 0; j < to_do; j++) {
>   			tags->rqs[i] = p;
> +
> +			/* Avoiding early access from timeout handler */
> +			tags->rqs[i]->tag = -1;
> +			tags->rqs[i]->q = NULL;
> +			tags->rqs[i]->cmd_flags = 0;
> +
>   			if (set->ops->init_request) {
>   				if (set->ops->init_request(set->driver_data,
>   						tags->rqs[i], hctx_idx, i,
Another way would be to ensure that the timeout handler doesn't touch 
hw_ctx or tag_sets that aren't fully initialized yet. But I think this 
is safer/cleaner.

-- 
Jens Axboe

On Wed, Sep 17, 2014 at 10:22 PM, Jens Axboe <axboe at kernel.dk>
wrote:
>
> Another way would be to ensure that the timeout handler doesn't touch
hw_ctx
> or tag_sets that aren't fully initialized yet. But I think this is
> safer/cleaner.
That may not be easy or enough to check if hw_ctx/tag_sets are
fully initialized if you mean all requests have been used one time.

On Wed, Sep 17, 2014 at 10:11 PM, David Hildenbrand
> I was playing with a simple patch that just sets cmd_flags and action_flags
to
What is action_flags?
> 0. That should already be sufficient to hinder blk_mq_tag_to_rq and the
calling
> method to do the wrong thing.
Yes, clearing rq->cmd_flags should be enough.

And looks better to move rq initialization to __blk_mq_free_request()
too, otherwise timeout still may see old cmd_flags and rq->q before
rq's new initialization.


Thanks,
-- 
Ming Lei

> On Wed, Sep 17, 2014 at 10:22 PM, Jens Axboe <axboe at kernel.dk>
wrote:
> >
> > Another way would be to ensure that the timeout handler doesn't
touch hw_ctx
> > or tag_sets that aren't fully initialized yet. But I think this is
> > safer/cleaner.
> 
> That may not be easy or enough to check if hw_ctx/tag_sets are
> fully initialized if you mean all requests have been used one time.
> 
> On Wed, Sep 17, 2014 at 10:11 PM, David Hildenbrand
> > I was playing with a simple patch that just sets cmd_flags and
action_flags to
> 
> What is action_flags?
atomic_flags, sorry :)

Otherwise e.g. REQ_ATOM_STARTED could already be set due to the randomness. I
am not sure if this is really necessary, or if it is completely shielded by the
tag-handling code, but seemed to be clean for me to do it (and I remember it
not being set within blk_mq_rq_ctx_init).
> 
> > 0. That should already be sufficient to hinder blk_mq_tag_to_rq and
the calling
> > method to do the wrong thing.
> 
> Yes, clearing rq->cmd_flags should be enough.
> 
> And looks better to move rq initialization to __blk_mq_free_request()
> too, otherwise timeout still may see old cmd_flags and rq->q before
> rq's new initialization.
Yes, __blk_mq_free_request() should also reset at least rq->cmd_flags, and I
think we can remove the initialization from  __blk_mq_alloc_request().

David
> 
> 
> Thanks,