Re: [PATCH V5 2/4] blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter

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

On Thu, May 06, 2021 at 07:54:40AM +0100, Christoph Hellwig wrote:
> On Wed, May 05, 2021 at 10:58:53PM +0800, Ming Lei wrote:
> > Grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter(), and
> > this way will prevent the request from being re-used when ->fn is
> > running. The approach is same as what we do during handling timeout.
> > 
> > Fix request UAF related with completion race or queue releasing:
> 
> I guess UAF is supposed to mean use-after-free?  Please just spell that
> out.

OK.

> 
> > +	rq = blk_mq_find_and_get_req(tags, bitnr);
> >  	/*
> >  	 * We can hit rq == NULL here, because the tagging functions
> >  	 * test and set the bit before assigning ->rqs[].
> >  	 */
> > +	if (!rq)
> > +		return true;
> 
> Nit: I'd find this much earier to follow if the blk_mq_find_and_get_req
> and thus assignment to rq was after the block comment.

OK.

> 
> >  	struct blk_mq_tags *tags = iter_data->tags;
> >  	bool reserved = iter_data->flags & BT_TAG_ITER_RESERVED;
> >  	struct request *rq;
> > +	bool ret = true;
> > +	bool iter_static_rqs = !!(iter_data->flags & BT_TAG_ITER_STATIC_RQS);
> >  
> >  	if (!reserved)
> >  		bitnr += tags->nr_reserved_tags;
> > @@ -272,16 +288,19 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >  	 * We can hit rq == NULL here, because the tagging functions
> >  	 * test and set the bit before assigning ->rqs[].
> >  	 */
> > +	if (iter_static_rqs)
> 
> I think this local variable just obsfucates what is going on here.

It has to be one local variable, otherwise sparse may complain since
iter_data->flags may be thought as being changed during the period.


Thanks,
Ming