Fix a use-after-free triggered by device removal

PATCH In review Bart Van Assche 3 participants 14 messages Sep 3, 2012 – Sep 13, 2012

Subsystems (2)

SCSI Block layer

lore

Share & tools

lore

Bart Van Assche

author

2012-09-03 14:12 UTC #0 lore raw

PATCH 1/1 Fix a use-after-free triggered by device removal

If the put_device() call in scsi_request_fn() drops the sdev refcount to zero then the spin_lock() call after the put_device() call triggers a use-after-free. Avoid that by making sure that blk_cleanup_queue() can only finish after all active scsi_request_fn() calls have returned.

Reported-by Chanho Min <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 block/blk-core.c        |   21 ++++++++++++++++++++-
 block/blk-exec.c        |    2 +-
 block/blk.h             |    2 ++
 drivers/scsi/scsi_lib.c |   10 +---------
 include/linux/blkdev.h  |    5 +++++
 5 files changed, 29 insertions(+), 11 deletions(-)

block/blk-core.c +20 -1

index 4b4dbdf..a668b71 100644
@@ -293,6 +293,24 @@ void blk_sync_queue(struct request_queue *q)
 EXPORT_SYMBOL(blk_sync_queue);
 
 /**
+ * __blk_run_queue_uncond - run a single device queue independent of its state
+ * @q:	The queue to run
+ *
+ * Description:
+ *    Invoke request handling on a queue if there are any pending requests.
+ *    May be used to restart request handling after a request has completed.
+ *    This variant runs the queue whether or not the queue has been
+ *    stopped. Must be called with the queue lock held and interrupts
+ *    disabled. See also @blk_run_queue.
+ */
+void __blk_run_queue_uncond(struct request_queue *q)
+{
+	q->request_fn_active++;
+	q->request_fn(q);
+	q->request_fn_active--;
+}
+
+/**
  * __blk_run_queue - run a single device queue
  * @q:	The queue to run
  *

@@ -305,7 +323,7 @@ void __blk_run_queue(struct request_queue *q)
 	if (unlikely(blk_queue_stopped(q)))
 		return;
 
-	q->request_fn(q);
+	__blk_run_queue_uncond(q);
 }
 EXPORT_SYMBOL(__blk_run_queue);

@@ -388,6 +406,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 			__blk_run_queue(q);
 
 		drain |= q->nr_rqs_elvpriv;
+		drain |= q->request_fn_active;
 
 		/*
 		 * Unfortunately, requests are queued at and tracked from

block/blk-exec.c +1 -1

index 8b6dc5b..f4f6f4d 100644
@@ -72,7 +72,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	__blk_run_queue(q);
 	/* the queue is stopped so it won't be run */
 	if (rq->cmd_type == REQ_TYPE_PM_RESUME)
-		q->request_fn(q);
+		__blk_run_queue_uncond(q);
 	spin_unlock_irq(q->queue_lock);
 }
 EXPORT_SYMBOL_GPL(blk_execute_rq_nowait);

block/blk.h +2 -0

index 2a0ea32..0b11e22 100644
@@ -145,6 +145,8 @@ int blk_try_merge(struct request *rq, struct bio *bio);
 
 void blk_queue_congestion_threshold(struct request_queue *q);
 
+void __blk_run_queue_uncond(struct request_queue *q);
+
 int blk_dev_init(void);

drivers/scsi/scsi_lib.c +1 -9

index ffd7773..9211e80 100644
@@ -1514,10 +1514,6 @@ static void scsi_request_fn(struct request_queue *q)
 	struct scsi_cmnd *cmd;
 	struct request *req;
 
-	if(!get_device(&sdev->sdev_gendev))
-		/* We must be tearing the block queue down already */
-		return;
-
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.

@@ -1626,11 +1622,7 @@ out_delay:
 	if (sdev->device_busy == 0)
 		blk_delay_queue(q, SCSI_QUEUE_DELAY);
 out:
-	/* must be careful here...if we trigger the ->remove() function
-	 * we cannot be holding the q lock */
-	spin_unlock_irq(q->queue_lock);
-	put_device(&sdev->sdev_gendev);
-	spin_lock_irq(q->queue_lock);
+	;
 }
 
 u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)

include/linux/blkdev.h +5 -0

index 4a2ab7c..132334e 100644
@@ -377,6 +377,11 @@ struct request_queue {
 
 	unsigned int		nr_sorted;
 	unsigned int		in_flight[2];
+	/*
+	 * Number of active request_fn() calls for those request_fn()
+	 * implementations that unlock the queue_lock, e.g. scsi_request_fn().
+	 */
+	unsigned int		request_fn_active;
 
 	unsigned int		rq_timeout;
 	struct timer_list	timeout;

Signature · 1 line

1.7.7

Michael Christie

2012-09-06 16:27 UTC #1 lore raw

Quoted 4 lines · Bart Van Assche

If the put_device() call in scsi_request_fn() drops the sdev refcount to zero then the spin_lock() call after the put_device() call triggers a use-after-free. Avoid that by making sure that blk_cleanup_queue() can only finish after all active scsi_request_fn() calls have returned.

If we have this patch
http://marc.info/?l=linux-scsi&m=134453905402413&w=2
it seems we have all the scsi layer callers of the request_fn/*blk_run_queue holding a reference to the device when they make the call. Right, or are there some other places missing?

What are the other places we can call the request_fn without already holding a reference to the device? Is it the block layer? Is that why we need this patch?

Bart Van Assche

author

2012-09-06 17:58 UTC #2 lore raw

Quoted 6 lines · Michael Christie

Quoted 4 lines · Bart Van Assche
If the put_device() call in scsi_request_fn() drops the sdev refcount to zero then the spin_lock() call after the put_device() call triggers a use-after-free. Avoid that by making sure that blk_cleanup_queue() can only finish after all active scsi_request_fn() calls have returned.

Quoted 8 lines

If we have this patch http://marc.info/?l=linux-scsi&m=134453905402413&w=2
it seems we have all the scsi layer callers of the request_fn/ *blk_run_queue holding a reference to the device when they make the call. Right, or are there some other places missing?
What are the other places we can call the request_fn without already holding a reference to the device? Is it the block layer? Is that why we need this patch?

Hello Mike,

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.

Bart.

Mike Christie

2012-09-06 18:15 UTC #3 lore raw

Quoted 16 lines · Bart Van Assche

Quoted 6 lines · Michael Christie
Quoted 4 lines · Bart Van Assche
If the put_device() call in scsi_request_fn() drops the sdev refcount to zero then the spin_lock() call after the put_device() call triggers a use-after-free. Avoid that by making sure that blk_cleanup_queue() can only finish after all active scsi_request_fn() calls have returned.
Quoted 8 lines
If we have this patch http://marc.info/?l=linux-scsi&m=134453905402413&w=2
it seems we have all the scsi layer callers of the request_fn/ *blk_run_queue holding a reference to the device when they make the call. Right, or are there some other places missing?
What are the other places we can call the request_fn without already holding a reference to the device? Is it the block layer? Is that why we need this patch?

Quoted 7 lines

Hello Mike,
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.

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?

Bart Van Assche

author

2012-09-06 18:52 UTC #4 lore raw

Quoted 29 lines · Mike Christie

Quoted 12 lines · Bart Van Assche
Quoted 11 lines · Michael Christie
Quoted 4 lines · Bart Van Assche
If the put_device() call in scsi_request_fn() drops the sdev refcount to zero then the spin_lock() call after the put_device() call triggers a use-after-free. Avoid that by making sure that blk_cleanup_queue() can only finish after all active scsi_request_fn() calls have returned.
If we have this patch http://marc.info/?l=linux-scsi&m=134453905402413&w=2
it seems we have all the scsi layer callers of the request_fn/ *blk_run_queue holding a reference to the device when they make the call. Right, or are there some other places missing?
Quoted 8 lines
Quoted 3 lines
What are the other places we can call the request_fn without already holding a reference to the device? Is it the block layer? Is that why we need this patch?
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.
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).

Quoted 5 lines

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().

Bart.

Tejun Heo

2012-09-06 23:20 UTC #5 lore raw

Hello, Bart, Mike.

Quoted 4 lines · Bart Van Assche

Quoted 4 lines
Quoted 4 lines
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.

Quoted 13 lines

Quoted 7 lines
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).
Quoted 5 lines
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?

Quoted 4 lines

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.

Signature · 1 line

tejun

Bart Van Assche

author

2012-09-07 06:57 UTC #6 lore raw

Quoted 4 lines · Tejun Heo

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.

Hello Tejun,

I'm not sure it would be a good idea to add a blk_queue_dead() check in any of the __blk_run_queue() variants since blk_drain_queue() can invoke __blk_run_queue() to drain the queue.

Also, as far as I can see the functions that can insert a request into the queue (blk_insert_cloned_request(), queue_unplugged(), blk_execute_rq_nowait()) all check whether the queue is dead before inserting a request. That should be sufficient to prevent that new requests are queued after QUEUE_FLAG_DEAD has been set.

Bart.

Tejun Heo

2012-09-10 23:38 UTC #7 lore raw

Hello,

Quoted 3 lines · Bart Van Assche

I'm not sure it would be a good idea to add a blk_queue_dead() check in any of the __blk_run_queue() variants since blk_drain_queue() can invoke __blk_run_queue() to drain the queue.

Right, we can't cancel requests from block layer which were already seen by the driver.

Quoted 5 lines

Also, as far as I can see the functions that can insert a request into the queue (blk_insert_cloned_request(), queue_unplugged(), blk_execute_rq_nowait()) all check whether the queue is dead before inserting a request. That should be sufficient to prevent that new requests are queued after QUEUE_FLAG_DEAD has been set.

Yes, but does that guarantee that none would call into ->request_fn()? If so, fine; otherwise, we may need to add another state to prevent that.

Thanks.

Signature · 1 line

tejun

Bart Van Assche

author

2012-09-11 06:42 UTC #8 lore raw

Quoted 10 lines · Tejun Heo

Quoted 5 lines · Bart Van Assche
Also, as far as I can see the functions that can insert a request into the queue (blk_insert_cloned_request(), queue_unplugged(), blk_execute_rq_nowait()) all check whether the queue is dead before inserting a request. That should be sufficient to prevent that new requests are queued after QUEUE_FLAG_DEAD has been set.
Yes, but does that guarantee that none would call into ->request_fn()? If so, fine; otherwise, we may need to add another state to prevent that.

Good question. As far as I can see calling request_queue.request_fn() is fine as long as the caller holds a reference on the queue. If e.g. scsi_request_fn() would get invoked after blk_drain_queue() finished it will return immediately because it was invoked with an empty request queue. So we should be fine as long as all blk_run_queue() callers either hold a reference on the request queue itself or on the sdev that owns the request queue. As far as I can see if patch
http://marc.info/?l=linux-scsi&m=134453905402413 gets accepted then all callers in the SCSI core of blk_run_queue() will hold a (direct or indirect) reference on the request_queue before invoking blk_run_queue() or __blk_run_queue().

Bart.

Tejun Heo

2012-09-12 20:53 UTC #9 lore raw

Hello,

Quoted 11 lines · Bart Van Assche

Good question. As far as I can see calling request_queue.request_fn() is fine as long as the caller holds a reference on the queue. If e.g. scsi_request_fn() would get invoked after blk_drain_queue() finished it will return immediately because it was invoked with an empty request queue. So we should be fine as long as all blk_run_queue() callers either hold a reference on the request queue itself or on the sdev that owns the request queue. As far as I can see if patch
http://marc.info/?l=linux-scsi&m=134453905402413 gets accepted then all callers in the SCSI core of blk_run_queue() will hold a (direct or indirect) reference on the request_queue before invoking blk_run_queue() or __blk_run_queue().

It's been quite a while since I really looked through the code and I'm feeling a bit dense but what you describe seems like a two-pronged approach where the drain stalling, when properly done, should be enough.

The problem at hand IIUC is ->request_fn() being invoked when request_queue itself is alive but the underlying driver is gone. We already make sure that a new request is not queued once drain is complete but there's no guarantee about calling into ->request_fn() and this is what you want to fix, right?

I think this is something which the block layer proper should handle correctly and expose sane interface. ie. if the caller has request_queue reference, it should be safe to call __blk_run_queue() no matter what. As long as SCSI follows proper shutdown procedure, it shouldn't need to worry about this.

Am I hopelessly confused somewhere?

Thanks.

Signature · 1 line

tejun

Bart Van Assche

author

2012-09-13 07:46 UTC #10 lore raw

Quoted 5 lines · Tejun Heo

The problem at hand IIUC is ->request_fn() being invoked when request_queue itself is alive but the underlying driver is gone. We already make sure that a new request is not queued once drain is complete but there's no guarantee about calling into ->request_fn() and this is what you want to fix, right?

Actually it's a slightly different issue that I want to address, namely that with the current implementation of the block layer and the SCSI core it's possible that blk_cleanup_queue() finishes after scsi_request_fn() has unlocked the queue lock and before it obtains the queue lock again.

The reason I'm proposing to add a counter in the block layer and not in the SCSI core is because I think it would be useful for other request-based block drivers too to be able to unlock the queue inside their ->request_fn(). That would allow to reduce lock contention on the request_queue lock for low-latency block drivers.

Bart.

Tejun Heo

2012-09-13 16:53 UTC #11 lore raw

Hello, Bart.

Quoted 13 lines · Bart Van Assche

Quoted 5 lines · Tejun Heo
The problem at hand IIUC is ->request_fn() being invoked when request_queue itself is alive but the underlying driver is gone. We already make sure that a new request is not queued once drain is complete but there's no guarantee about calling into ->request_fn() and this is what you want to fix, right?
Actually it's a slightly different issue that I want to address, namely that with the current implementation of the block layer and the SCSI core it's possible that blk_cleanup_queue() finishes after scsi_request_fn() has unlocked the queue lock and before it obtains the queue lock again.

Quoted 5 lines

The reason I'm proposing to add a counter in the block layer and not in the SCSI core is because I think it would be useful for other request-based block drivers too to be able to unlock the queue inside their ->request_fn(). That would allow to reduce lock contention on the request_queue lock for low-latency block drivers.

Oh yeah, I definitely think this is something which needs to be solved from the block layer but I'm hoping this could cover the case Chanho is trying to solve too. They're different but similar problems - you don't want blk_cleanup_queue() to finish while someone is executing inside it and you don't want anyone to enter it after blk_cleanup_queue() is finished, so I really think we should have block layer solution which fixes both problems. That should be possible, right?

Thanks.

Signature · 1 line

tejun

Bart Van Assche

author

2012-09-13 18:27 UTC #12 lore raw

Quoted 8 lines · Tejun Heo

Oh yeah, I definitely think this is something which needs to be solved from the block layer but I'm hoping this could cover the case Chanho is trying to solve too. They're different but similar problems - you don't want blk_cleanup_queue() to finish while someone is executing inside it and you don't want anyone to enter it after blk_cleanup_queue() is finished, so I really think we should have block layer solution which fixes both problems. That should be possible, right?

If I do not receive further feedback I'll start testing the patch below (on top of the patch at the start of this thread):

[PATCH] Avoid that request_fn() gets invoked after draining the queue finished

---
 block/blk-core.c       |    4 ++++
 include/linux/blkdev.h |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)

block/blk-core.c +4 -0

index a668b71..575b7c4 100644
@@ -305,6 +305,9 @@ EXPORT_SYMBOL(blk_sync_queue);
  */
 void __blk_run_queue_uncond(struct request_queue *q)
 {
+	if (unlikely(blk_queue_drained(q)))
+		return;
+
 	q->request_fn_active++;
 	q->request_fn(q);
 	q->request_fn_active--;

@@ -532,6 +535,7 @@ void blk_cleanup_queue(struct request_queue *q)
 	blk_sync_queue(q);
 
 	spin_lock_irq(lock);
+	queue_flag_set(QUEUE_FLAG_DRAINED, q);
 	if (q->queue_lock != &q->__queue_lock)
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);

include/linux/blkdev.h +2 -0

index 132334e..ceebc39 100644
@@ -456,6 +456,7 @@ struct request_queue {
 #define QUEUE_FLAG_ADD_RANDOM  16	/* Contributes to random pool */
 #define QUEUE_FLAG_SECDISCARD  17	/* supports SECDISCARD */
 #define QUEUE_FLAG_SAME_FORCE  18	/* force complete on same CPU */
+#define QUEUE_FLAG_DRAINED     19	/* queue tear-down finished */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\

@@ -526,6 +527,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
+#define blk_queue_drained(q)	test_bit(QUEUE_FLAG_DRAINED, &(q)->queue_flags)
 #define blk_queue_bypass(q)	test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
 #define blk_queue_noxmerges(q)	\

Signature · 1 line

1.7.7

Tejun Heo

2012-09-13 19:25 UTC #13 lore raw

Hello,

Quoted 2 lines · Bart Van Assche

If I do not receive further feedback I'll start testing the patch below (on top of the patch at the start of this thread):

Yeah, it generally seems fine to me. This makes Chanho's patch unnecessary, right? Just some minor points.

include/linux/blkdev.h struct request_queue { · line 459

Quoted 5 lines

include/linux/blkdev.h +1 -0

@@ -456,6 +456,7 @@ struct request_queue {
 #define QUEUE_FLAG_ADD_RANDOM  16	/* Contributes to random pool */
 #define QUEUE_FLAG_SECDISCARD  17	/* supports SECDISCARD */
 #define QUEUE_FLAG_SAME_FORCE  18	/* force complete on same CPU */
+#define QUEUE_FLAG_DRAINED     19	/* queue tear-down finished */

This is rather confusing to me. Maybe we can have QUEUE_FLAG_DYING and QUEUE_FLAG_DEAD? Or at least explain the involve state changes in detail.

Thanks.

Signature · 1 line

tejun