Re: [PATCH] block: pop cached rq before potentially blocking rq_qos_throttle()

[Dylan Yudaken <dylany@fb.com>]

On Tue, 2022-06-21 at 10:07 -0600, Jens Axboe wrote:
> If rq_qos_throttle() ends up blocking, then we will have invalidated
> and
> flushed our current plug. Since blk_mq_get_cached_request() hasn't
> popped the cached request off the plug list just yet, we end holding
> a
> pointer to a request that is no longer valid. This insta-crashes with
> rq->mq_hctx being NULL in the validity checks just after.
> 
> Pop the request off the cached list before doing rq_qos_throttle() to
> avoid using a potentially stale request.
> 
> Fixes: 0a5aa8d161d1 ("block: fix blk_mq_attempt_bio_merge and
> rq_qos_throttle protection")
> Reported-by: Dylan Yudaken <dylany@fb.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 33145ba52c96..93d9d60980fb 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2765,15 +2765,20 @@ static inline struct request
> *blk_mq_get_cached_request(struct request_queue *q,
>                 return NULL;
>         }
>  
> -       rq_qos_throttle(q, *bio);
> -
>         if (blk_mq_get_hctx_type((*bio)->bi_opf) != rq->mq_hctx-
> >type)
>                 return NULL;
>         if (op_is_flush(rq->cmd_flags) != op_is_flush((*bio)-
> >bi_opf))
>                 return NULL;
>  
> -       rq->cmd_flags = (*bio)->bi_opf;
> +       /*
> +        * If any qos ->throttle() end up blocking, we will have
> flushed the
> +        * plug and hence killed the cached_rq list as well. Pop this
> entry
> +        * before we throttle.
> +        */
>         plug->cached_rq = rq_list_next(rq);
> +       rq_qos_throttle(q, *bio);
> +
> +       rq->cmd_flags = (*bio)->bi_opf;
>         INIT_LIST_HEAD(&rq->queuelist);
>         return rq;
>  }

This fixes the problems I was seeing

Tested-by: Dylan Yudaken <dylany@fb.com>