Re: [PATCH 0/5] blk-mq: fix use-after-free on stale request

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

On Wed, Aug 26, 2020 at 01:03:37PM +0100, John Garry wrote:
> On 21/08/2020 03:49, Ming Lei wrote:
> > Hello Bart,
> > 
> > On Thu, Aug 20, 2020 at 01:30:38PM -0700, Bart Van Assche wrote:
> > > On 8/20/20 11:03 AM, Ming Lei wrote:
> > > > We can't run allocating driver tag and updating tags->rqs[tag] atomically,
> > > > so stale request may be retrieved from tags->rqs[tag]. More seriously, the
> > > > stale request may have been freed via updating nr_requests or switching
> > > > elevator or other use cases.
> > > > 
> > > > It is one long-term issue, and Jianchao previous worked towards using
> > > > static_rqs[] for iterating request, one problem is that it can be hard
> > > > to use when iterating over tagset.
> > > > 
> > > > This patchset takes another different approach for fixing the issue: cache
> > > > freed rqs pages and release them until all tags->rqs[] references on these
> > > > pages are gone.
> > > 
> > > Hi Ming,
> > > 
> > > Is this the only possible solution? Would it e.g. be possible to protect the
> > > code that iterates over all tags with rcu_read_lock() / rcu_read_unlock() and
> > > to free pages that contain request pointers only after an RCU grace period has
> > > expired?
> > 
> > That can't work, tags->rqs[] is host-wide, request pool belongs to scheduler tag
> > and it is owned by request queue actually. When one elevator is switched on this
> > request queue or updating nr_requests, the old request pool of this queue is freed,
> > but IOs are still queued from other request queues in this tagset. Elevator switch
> > or updating nr_requests on one request queue shouldn't or can't other request queues
> > in the same tagset.
> > 
> > Meantime the reference in tags->rqs[] may stay a bit long, and RCU can't cover this
> > case.
> > 
> > Also we can't reset the related tags->rqs[tag] simply somewhere, cause it may
> > race with new driver tag allocation.
> 
> How about iterate all tags->rqs[] for all scheduler tags when exiting the
> scheduler, etc, and clear any scheduler requests references, like this:
> 
> cmpxchg(&hctx->tags->rqs[tag], scheduler_rq, 0);
> 
> So we NULLify any tags->rqs[] entries which contain a scheduler request of
> concern atomically, cleaning up any references.

Looks this approach can work given cmpxchg() will prevent new store on
this address.

> 
> I quickly tried it and it looks to work, but maybe not so elegant.

I think this way is good enough.


thanks, 
Ming