Re: [PATCH 2/2] block: only return started requests from blk_mq_tag_to_rq()

[Ming Lei <ming.lei@redhat.com>]

On Fri, Jun 19, 2020 at 12:38:01PM -0600, Jens Axboe wrote:
> On 6/19/20 8:09 AM, Hannes Reinecke wrote:
> > On 6/19/20 4:01 PM, Hannes Reinecke wrote:
> >> blk_mq_tag_to_rq() is used from within the driver to map a tag
> >> to a request. As such it should only return requests which are
> >> already started (ie passed to the driver); otherwise the driver
> >> might trip over requests which it has never seen and random
> >> crashes will occur.
> >>
> >> Signed-off-by: Hannes Reinecke <hare@suse.de>
> >> ---
> >>   block/blk-mq.c | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >> index 4f57d27bfa73..f02d18113f9e 100644
> >> --- a/block/blk-mq.c
> >> +++ b/block/blk-mq.c
> >> @@ -815,9 +815,13 @@ EXPORT_SYMBOL(blk_mq_delay_kick_requeue_list);
> >>   
> >>   struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
> >>   {
> >> +	struct request *rq;
> >> +
> >>   	if (tag < tags->nr_tags) {
> >>   		prefetch(tags->rqs[tag]);
> >> -		return tags->rqs[tag];
> >> +		rq = tags->rqs[tag];
> >> +		if (blk_mq_request_started(rq))
> >> +			return rq;
> >>   	}
> >>   
> >>   	return NULL;
> >>
> > This becomes particularly obnoxious for SCSI drivers using 
> > scsi_host_find_tag() for cleaning up stale commands (ie drivers like 
> > qla4xxx, fnic, and snic).
> > All other drivers use it from the completion routine, so one can expect 
> > a valid (and started) tag here. So for those it shouldn't matter.
> > 
> > But still, if there are objections I could look at fixing it within the 
> > SCSI stack; although that would most likely mean I'll have to implement 
> > the above patch as an additional function.
> 
> The helper does exactly what it should, return a request associated
> with a tag. Either add the logic to the caller, or provide a new
> helper that does what you need. I'd be inclined to just add it to
> the caller that needs it.

I agree, even though the idea is good for most of driver usage since driver
should only care started request.

It may cause kernel oops in some corner case, such as blk_mq_poll_hybrid(),
blk_poll() may see one not-started request, one example is nvme_execute_rq_polled().

Thanks,
Ming