SCSI device removal fixes

From: Muthukumar Ratty <[email protected]>

If the queue is dead blk_execute_rq_nowait() doesn't invoke the done() callback function. That will result in blk_execute_rq() being stuck in wait_for_completion(). Avoid this by initializing rq->end_io to the done() callback before we check the queue state. Also, make sure the queue lock is held around the invocation of the done() callback. Found this through source code review.

Signed-off-by Muthukumar Ratty <[email protected]>

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

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

Quoted 9 lines · Bart Van Assche

From: Muthukumar Ratty <[email protected]>

If the queue is dead blk_execute_rq_nowait() doesn't invoke the done() callback function. That will result in blk_execute_rq() being stuck in wait_for_completion(). Avoid this by initializing rq->end_io to the done() callback before we check the queue state. Also, make sure the queue lock is held around the invocation of the done() callback. Found this through source code review.

Quoted 5 lines

Signed-off-by Muthukumar Ratty <[email protected]>

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

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks a lot!

--
tejun

drivers/scsi/hosts.c +0 -0

 	}
 
 	scsi_destroy_command_freelist(shost);

drivers/scsi/scsi_lib.c +2 -7

index 6dfb978..c26ef49 100644
@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 
-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);
 	shost = sdev->host;
 	if (scsi_target(sdev)->single_lun)
 		scsi_single_lun_run(sdev);

@@ -1370,16 +1367,18 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
  * may be changed after request stacking drivers call the function,
  * regardless of taking lock or not.
  *
- * When scsi can't dispatch I/Os anymore and needs to kill I/Os
- * (e.g. !sdev), scsi needs to return 'not busy'.
- * Otherwise, request stacking drivers may hold requests forever.
+ * When scsi can't dispatch I/Os anymore and needs to kill I/Os scsi

Since scsi_prep_fn() may be invoked concurrently with __scsi_remove_device(), keep the queuedata pointer in __scsi_remove_device(). This patch fixes a kernel oops that can be triggered by USB device removal. See also
http://www.spinics.net/lists/linux-scsi/msg56254.html.

Reported-by Jun'ichi Nomura <[email protected]>

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

Reviewed-by Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Stefan Richter <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/hosts.c      |    8 +++++++-
 drivers/scsi/scsi_lib.c   |   35 ++++++++---------------------------
 drivers/scsi/scsi_priv.h  |    1 -
 drivers/scsi/scsi_sysfs.c |    5 +----
 4 files changed, 16 insertions(+), 33 deletions(-)

drivers/scsi/hosts.c +0 -0

index a3a056a..6b9d89a 100644

drivers/scsi/hosts.c +1 -1

 		kfree(q->queuedata);
 		q->queuedata = NULL;
-		scsi_free_queue(q);
+		blk_cleanup_queue(q);
 	}
 
 	scsi_destroy_command_freelist(shost);

drivers/scsi/scsi_lib.c +0 -0

index 6dfb978..c26ef49 100644

drivers/scsi/scsi_lib.c +6 -4

 	shost = sdev->host;
 	if (scsi_target(sdev)->single_lun)
 		scsi_single_lun_run(sdev);

@@ -1370,16 +1367,18 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
  * may be changed after request stacking drivers call the function,
  * regardless of taking lock or not.
  *
- * When scsi can't dispatch I/Os anymore and needs to kill I/Os
- * (e.g. !sdev), scsi needs to return 'not busy'.
- * Otherwise, request stacking drivers may hold requests forever.
+ * When scsi can't dispatch I/Os anymore and needs to kill I/Os scsi
+ * needs to return 'not busy'. Otherwise, request stacking drivers
+ * may hold requests forever.
  */
 static int scsi_lld_busy(struct request_queue *q)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 
-	if (!sdev)
+	BUG_ON(!sdev);
+
+	if (blk_queue_dead(q))
 		return 0;
 
 	shost = sdev->host;

Quoted 4 lines

drivers/scsi/scsi_lib.c +0 -0

@@ -1490,11 +1489,7 @@ static void scsi_request_fn(struct request_queue *q)
 	struct scsi_cmnd *cmd;
 	struct request *req;
 

drivers/scsi/scsi_lib.c static void scsi_request_fn(struct request_queue *q) · line 1497

Quoted 5 lines

drivers/scsi/scsi_lib.c +0 -5

-	if (!sdev) {
-		while ((req = blk_peek_request(q)) != NULL)
-			scsi_kill_request(req, q);
-		return;
-	}

That means that this hunk of code has to stay, but needs to be gated on blk_queue_dead(q); there's still a race where this can occur.

Hey, James.

Quoted 4 lines · James Bottomley

Quoted 4 lines

drivers/scsi/scsi_lib.c +0 -0

@@ -1490,11 +1489,7 @@ static void scsi_request_fn(struct request_queue *q)
 	struct scsi_cmnd *cmd;
 	struct request *req;
 

Quoted 8 lines

drivers/scsi/scsi_lib.c static void scsi_request_fn(struct request_queue *q) · line 1497

Quoted 5 lines

drivers/scsi/scsi_lib.c +0 -5

-	if (!sdev) {
-		while ((req = blk_peek_request(q)) != NULL)
-			scsi_kill_request(req, q);
-		return;
-	}

That means that this hunk of code has to stay, but needs to be gated on blk_queue_dead(q); there's still a race where this can occur.

Wouldn't the scsi_device_online() check down below be enough? Block layer drain is gonna loop until all requests are done, so the looping is handled from block layer.

Quoted 16 lines · Tejun Heo

Hey, James.

Quoted 4 lines · James Bottomley

Quoted 4 lines

(quoted diff) +0 -0

@@ -1490,11 +1489,7 @@ static void scsi_request_fn(struct request_queue *q)
 	struct scsi_cmnd *cmd;
 	struct request *req;
 

Quoted 8 lines

Review static void scsi_request_fn(struct request_queue *q) · line 1497

Quoted 5 lines

(quoted diff) +0 -5

-	if (!sdev) {
-		while ((req = blk_peek_request(q)) != NULL)
-			scsi_kill_request(req, q);
-		return;
-	}

That means that this hunk of code has to stay, but needs to be gated on blk_queue_dead(q); there's still a race where this can occur.

Quoted 3 lines

Wouldn't the scsi_device_online() check down below be enough? Block layer drain is gonna loop until all requests are done, so the looping is handled from block layer.

It might be ... in theory the teardown is supposed to happen in SDEV_CANCEL and be done by SDEV_DEL. However, I'm not sure that's entirely true now. blk_queue_dead() is safer since we know we just killed the queue. Another reason for doing it like this is that the kill on queue dead isn't noisy ... the one on !online is ... and logs were getting stuffed with messages about killing requests to dead queues.

Quoted 5 lines · Bart Van Assche

Since scsi_prep_fn() may be invoked concurrently with __scsi_remove_device(), keep the queuedata pointer in __scsi_remove_device(). This patch fixes a kernel oops that can be triggered by USB device removal. See also
http://www.spinics.net/lists/linux-scsi/msg56254.html.

This patch causes a subtle change of semantics: you're substituting our signal for dead queue as !sdev with a check of blk_queue_dead().

Quoted 25 lines

Reported-by Jun'ichi Nomura <[email protected]>

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

Reviewed-by Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Stefan Richter <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/hosts.c      |    8 +++++++-
 drivers/scsi/scsi_lib.c   |   35 ++++++++---------------------------
 drivers/scsi/scsi_priv.h  |    1 -
 drivers/scsi/scsi_sysfs.c |    5 +----
 4 files changed, 16 insertions(+), 33 deletions(-)

drivers/scsi/hosts.c +4 -0

index a3a056a..6b9d89a 100644
@@ -299,9 +299,15 @@ static void scsi_host_dev_release(struct device *dev)
 		destroy_workqueue(shost->work_q);
 	q = shost->uspace_req_q;
 	if (q) {
+		/*
+		 * Note: freeing queuedata before invoking blk_cleanup_queue()
+		 * is safe here because no request function is associated with
+		 * uspace_req_q. See also the __scsi_alloc_queue() call in

drivers/scsi/hosts.c static void scsi_host_dev_release(struct device *dev) · line 307

Quoted 2 lines

drivers/scsi/hosts.c +2 -0

+		 * drivers/scsi/scsi_tgt_lib.c.
+		 */

This comment doesn't really make a whole lot of sense. What I think it's saying is that it's OK for the commands executed by the drain in blk_cleanup_queue to have a NULL queuedata and by the time we reach this, there can be no concurrent racing calls to queuecommand? Is this true, and why?

Quoted 35 lines · James Bottomley

Quoted 5 lines · Bart Van Assche

Since scsi_prep_fn() may be invoked concurrently with __scsi_remove_device(), keep the queuedata pointer in __scsi_remove_device(). This patch fixes a kernel oops that can be triggered by USB device removal. See also
http://www.spinics.net/lists/linux-scsi/msg56254.html.

This patch causes a subtle change of semantics: you're substituting our signal for dead queue as !sdev with a check of blk_queue_dead().

Quoted 25 lines

Reported-by Jun'ichi Nomura <[email protected]>

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

Reviewed-by Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Stefan Richter <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/hosts.c      |    8 +++++++-
 drivers/scsi/scsi_lib.c   |   35 ++++++++---------------------------
 drivers/scsi/scsi_priv.h  |    1 -
 drivers/scsi/scsi_sysfs.c |    5 +----
 4 files changed, 16 insertions(+), 33 deletions(-)

drivers/scsi/hosts.c +4 -0

index a3a056a..6b9d89a 100644
@@ -299,9 +299,15 @@ static void scsi_host_dev_release(struct device *dev)
 		destroy_workqueue(shost->work_q);
 	q = shost->uspace_req_q;
 	if (q) {
+		/*
+		 * Note: freeing queuedata before invoking blk_cleanup_queue()
+		 * is safe here because no request function is associated with
+		 * uspace_req_q. See also the __scsi_alloc_queue() call in

Quoted 8 lines

drivers/scsi/hosts.c static void scsi_host_dev_release(struct device *dev) · line 307

Quoted 2 lines

drivers/scsi/hosts.c +2 -0

+		 * drivers/scsi/scsi_tgt_lib.c.
+		 */

This comment doesn't really make a whole lot of sense. What I think it's saying is that it's OK for the commands executed by the drain in blk_cleanup_queue to have a NULL queuedata and by the time we reach this, there can be no concurrent racing calls to queuecommand? Is this true, and why?

Only the scsi_tgt_lib.c code uses the uses the uspace_req_q. That code does not use it in a traditional way. It passes __scsi_alloc_queue NULL for the request_fn function argument, so this request queue does not have a function that pulls requests off a queue like is done for the initiator path.

That target code mostly uses the queue struct so that it can use the block/scsi functions that need a request queue to build scatterlists, cmds, bios, etc. When that code was made originally we were going to use the queue in a more tradition way or break out the queue limits into another struct and pass them around. I think we have the latter now, but the target code has not been converted.

But so that is why we cannot hit a race like you are thinking about in the initiator path.

It is also a weird use of the queue so it is also why it does not make sense :)

Quoted 13 lines · Mike Christie

Only the scsi_tgt_lib.c code uses the uses the uspace_req_q. That code does not use it in a traditional way. It passes __scsi_alloc_queue NULL for the request_fn function argument, so this request queue does not have a function that pulls requests off a queue like is done for the initiator path.

That target code mostly uses the queue struct so that it can use the block/scsi functions that need a request queue to build scatterlists, cmds, bios, etc. When that code was made originally we were going to use the queue in a more tradition way or break out the queue limits into another struct and pass them around. I think we have the latter now, but the target code has not been converted.

Quoted 5 lines

But so that is why we cannot hit a race like you are thinking about in the initiator path.

It is also a weird use of the queue so it is also why it does not make sense :)

OK, could we encapsulate all that in the comment so I don't have to ask again in a year's time ... ?

Quoted 8 lines · Bart Van Assche

drivers/scsi/scsi_lib.c +0 -0

index 6dfb978..c26ef49 100644
@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 409

Quoted 5 lines

drivers/scsi/scsi_lib.c +1 -4

-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);

Needs to be a blk_queue_dead() check as well.

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 410

Quoted 1 line

drivers/scsi/scsi_lib.c +1 -0

+	BUG_ON(!sdev);

I'm with Tejun, these BUG_ON's are now pretty pointless.

Quoted 9 lines · James Bottomley

Quoted 8 lines · Bart Van Assche

drivers/scsi/scsi_lib.c +0 -0

index 6dfb978..c26ef49 100644
@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 

Quoted 7 lines

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 409

Quoted 5 lines

drivers/scsi/scsi_lib.c +1 -4

-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);

Needs to be a blk_queue_dead() check as well.

Callers of scsi_run_queue() don't hold the queue lock. Does it make sense to test whether the queue is dead without the queue lock being held ?

Quoted 19 lines · Bart Van Assche

Quoted 9 lines · James Bottomley

Quoted 8 lines · Bart Van Assche

drivers/scsi/scsi_lib.c +0 -0

index 6dfb978..c26ef49 100644
@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 

Quoted 7 lines

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 409

Quoted 5 lines

drivers/scsi/scsi_lib.c +1 -4

-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);

Needs to be a blk_queue_dead() check as well.

Quoted 2 lines

Callers of scsi_run_queue() don't hold the queue lock. Does it make sense to test whether the queue is dead without the queue lock being held ?

I'm not entirely sure I understand the question, but I think you think that locking is required to read the queue state? That's emphatically not correct. In fact, you don't even need locking to write the state provied the state variable is a natcural processor witdth (i.e. u32 on 32 bit or u64 or u32 on 64 bit) and you don't do dependent state variable updates or other atomic sequences.

Quoted 8 lines

Quoted 8 lines

drivers/scsi/scsi_lib.c +0 -0

Quoted 7 lines

Quoted 7 lines

index 6dfb978..c26ef49 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 409

Quoted 5 lines

drivers/scsi/scsi_lib.c +1 -4

-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);

Needs to be a blk_queue_dead() check as well.

Callers of scsi_run_queue() don't hold the queue lock. Does it make sense to test whether the queue is dead without the queue lock being held ?

I think there is still another bug in this path when it is called from the requeue path. If scsi_requeue_command requeues a command and that gets executed by some other thread before scsi_requeue_command calls scsi_run_queue then we could end up accessing freed memory.

It looks possible some other thread is doing blk_cleanup_queue->blk_drain_queue and that calls blk_run_queue and that kills/fails the IO that scsi_requeue_command had queued. Then blk_cleanup_queue could complete and we could end up doing the last put on the device and freeing the queue and sdev before scsi_requeue_command can call scsi_run_queue. scsi_run_queue could then be accessing freed memory.

I think we need a get/put:

scsi_requeue_command....

get_device(&sdev->sdev_gendev);

spin_lock_irqsave(q->queue_lock, flags);
scsi_unprep_request(req);
blk_requeue_request(q, req);
spin_unlock_irqrestore(q->queue_lock, flags);

scsi_run_queue(q);

put_device(&sdev->sdev_gendev);

This will prevent some other path from freeing the queue/sdev from under us.

Quoted 16 lines · Bart Van Assche

Quoted 8 lines

Quoted 8 lines

drivers/scsi/scsi_lib.c +0 -0

Quoted 7 lines

Quoted 7 lines

index 6dfb978..c26ef49 100644

drivers/scsi/scsi_lib.c +0 -0

@@ -406,10 +406,7 @@ static void scsi_run_queue(struct request_queue *q)
 	LIST_HEAD(starved_list);
 	unsigned long flags;
 

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

drivers/scsi/scsi_lib.c static void scsi_run_queue(struct request_queue *q) · line 409

Quoted 5 lines

drivers/scsi/scsi_lib.c +1 -4

-	/* if the device is dead, sdev will be NULL, so no queue to run */
-	if (!sdev)
-		return;
-
+	BUG_ON(!sdev);

Needs to be a blk_queue_dead() check as well.

Quoted 3 lines

Callers of scsi_run_queue() don't hold the queue lock. Does it make sense to test whether the queue is dead without the queue lock being held ?

I think there is still another bug in this path when it is called from the requeue path. If scsi_requeue_command requeues a command and that gets executed by some other thread before scsi_requeue_command calls scsi_run_queue then we could end up accessing freed memory.

It looks possible some other thread is doing blk_cleanup_queue->blk_drain_queue and that calls blk_run_queue and that kills/fails the IO that scsi_requeue_command had queued. Then blk_cleanup_queue could complete and we could end up doing the last put on the device and freeing the queue and sdev before scsi_requeue_command can call scsi_run_queue. scsi_run_queue could then be accessing freed memory.

I think we need a get/put:

scsi_requeue_command....

get_device(&sdev->sdev_gendev);

spin_lock_irqsave(q->queue_lock, flags);
scsi_unprep_request(req);
blk_requeue_request(q, req);
spin_unlock_irqrestore(q->queue_lock, flags);

scsi_run_queue(q);

put_device(&sdev->sdev_gendev);

This will prevent some other path from freeing the queue/sdev from under us.

Avoid that the code for requeueing SCSI requests triggers a crash by making sure that that code isn't scheduled anymore after a device has been removed.

Also, source code inspection of __scsi_remove_device() revealed a race condition in this function: no new SCSI requests must be accepted for a SCSI device after device removal started.

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

Reviewed-by Mike Christie <[email protected]>

Cc James Bottomley <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

Cc Jun'ichi Nomura <[email protected]>

Cc <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    7 ++++---
 drivers/scsi/scsi_sysfs.c |   11 +++++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

drivers/scsi/scsi_lib.c +0 -1

index 082c1e5..fc2b9f4 100644
@@ -155,13 +155,14 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 
 	/*
 	 * Requeue this command.  It will go before all other commands
-	 * that are already in the queue.