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

[Bart Van Assche <bvanassche@acm.org>]

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.

Bart.