Re: [PATCH] Fix a use-after-free triggered by device removal

[Tejun Heo <tj@kernel.org>]

Hello, Bart, Mike.

On Thu, Sep 06, 2012 at 08:52:09PM +0200, Bart Van Assche wrote:
> >> The purpose of this patch is indeed to make *blk_run_queue() calls from
> >> the block layer safe. There are several direct or indirect
> >> *blk_run_queue() calls in the block layer where a reference on the queue
> >> is held but not on the sdev, e.g. in the md, dm and bsg drivers.

Yeah, while writing blk_drain_queue(), I was thinking only about
requests.  I didn't consider control reaching into driver.

> > Is there a race still? If some blk code is calling blk_run_queue
> > (waiting on the queue lock) but no IO is queued,
> > blk_drain_queue/blk_cleanup_queue could complete since the drain
> > counters are zero. Then blk_run_queue could grab the queue lock and call
> > the request_fn on a freed scsi_device (sdev pointed to by q->queuedata
> > would be freed so scsi_reuqest_fn would be freed).
> > 
> > Do we need a check in __blk_run_queue for QUEUE_FLAG_DEAD (if dead then
> > fail IO?), or do we need a check in scsi_request_fn for this. A dead
> > queue check or maybe null q->queuedata in
> > scsi_device_dev_release_usercontext (do this with the queue lock held),
> > then check for null q->queuedata in scsi_request_fn?
> 
> Yet another scenario is that scsi_remove_host() gets invoked and
> finishes after scsi_request_fn() has unlocked the queue and before it
> locks the queue again. That's a scenario that can't be handled by adding
> more checks at the start of __blk_run_queue() or scsi_request_fn().

I think Mike is wondering whether your patch in isolation is enough or
we also need to have DEAD check there too.  The proposed patch can't
handle the case where q->request_fn() is invoked after drain is
complete.  I'm not really sure whether that can happen tho.

Other than that, yeah, I think it's a real problem and we definitely
want to remove get/put_device() from scsi_request_fn().  Refcnting
oneself is often wrong (as shown here too) and generally just sad.

Thanks.

-- 
tejun