blk-mq: export setting request completion state

PATCH In review Keith Busch 6 participants 11 messages Jul 23, 2018 – Jul 25, 2018

Subsystems (3)

SCSI Block layer NVMe

lore

Share & tools

lore

Keith Busch

author

2018-07-23 14:37 UTC #0 lore raw

PATCH v2 1/2 blk-mq: export setting request completion state

This is preparing for drivers that want to directly alter the state of their requests. No functional change here.

Signed-off-by Keith Busch <[email protected]>

---
v1 -> v2:

Reversed the return value: 'true' for success instead of failure.

Document the caller's responsibilities after a successful state transtion.

 block/blk-mq.c         |  4 +---
 include/linux/blk-mq.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

block/blk-mq.c +1 -3

index 22326612a5d3..8f01cd7fd182 100644
@@ -558,10 +558,8 @@ static void __blk_mq_complete_request(struct request *rq)
 	bool shared = false;
 	int cpu;
 
-	if (cmpxchg(&rq->state, MQ_RQ_IN_FLIGHT, MQ_RQ_COMPLETE) !=
-			MQ_RQ_IN_FLIGHT)
+	if (!blk_mq_mark_complete(rq))
 		return;
-
 	if (rq->internal_tag != -1)
 		blk_mq_sched_completed_request(rq);

include/linux/blk-mq.h +14 -0

index 9b0fd11ce89a..4d41ed16314c 100644
@@ -289,6 +289,20 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues);
 
 void blk_mq_quiesce_queue_nowait(struct request_queue *q);
 
+/**
+ * blk_mq_mark_complete() - Set request state to complete
+ * @rq: request to set to complete state
+ *
+ * Returns true if request state was successfully set to complete. If
+ * successful, the caller is responsibile for seeing this request is ended, as
+ * blk_mq_complete_request will not work again.
+ */
+static inline bool blk_mq_mark_complete(struct request *rq)
+{
+	return cmpxchg(&rq->state, MQ_RQ_IN_FLIGHT, MQ_RQ_COMPLETE) ==
+			MQ_RQ_IN_FLIGHT;
+}
+
 /*
  * Driver command data is immediately after the request. So subtract request
  * size to get back to the original request, add request size to get the PDU.

Signature · 1 line

2.14.4

Keith Busch

author

2018-07-23 14:37 UTC #1 lore raw

PATCH v2 2/2 scsi: set timed out out mq requests to complete

The scsi block layer requires requests claimed by the error handling be completed by the error handler. A previous commit allowed completions to proceed for blk-mq, breaking that assumption.

This patch prevents completions that may race with the timeout handler by marking the state to complete, restoring the previous behavior.

Fixes 12f5b931 ("blk-mq: Remove generation seqeunce")

Signed-off-by Keith Busch <[email protected]>

---
v1 -> v2:

Document why this is necessary in code comments.

Update to API's changed return value

 drivers/scsi/scsi_error.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

drivers/scsi/scsi_error.c +14 -0

index 8932ae81a15a..2715cdaa669c 100644
@@ -296,6 +296,20 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_DONE) {
+		/*
+		 * For blk-mq, we must set the request state to complete now
+		 * before sending the request to the scsi error handler. This
+		 * will prevent a use-after-free in the event the LLD manages
+		 * to complete the request before the error handler finishes
+		 * processing this timed out request.
+		 *
+		 * If the request was already completed, then the LLD beat the
+		 * time out handler from transferring the request to the scsi
+		 * error handler. In that case we can return immediately as no
+		 * further action is required.
+		 */
+		if (req->q->mq_ops && !blk_mq_mark_complete(req))
+			return rtn;
 		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);

Signature · 1 line

2.14.4

Christoph Hellwig

maintainer Reviewed-by on v2 2/2

2018-07-24 07:56 UTC #2 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

Quoted 9 lines · Keith Busch

The scsi block layer requires requests claimed by the error handling be completed by the error handler. A previous commit allowed completions to proceed for blk-mq, breaking that assumption.
This patch prevents completions that may race with the timeout handler by marking the state to complete, restoring the previous behavior.
Fixes 12f5b931 ("blk-mq: Remove generation seqeunce")
Signed-off-by Keith Busch <[email protected]>

Looks good enough (reluctantly) for this merge window:

Reviewed-by Christoph Hellwig <[email protected]>

Bart Van Assche

2018-07-24 22:46 UTC #3 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

In reply to Keith Busch

Quoted 15 lines · Keith Busch

drivers/scsi/scsi_error.c +7 -0

index 8932ae81a15a..2715cdaa669c 100644
@@ -296,6 +296,20 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
  		rtn = host->hostt->eh_timed_out(scmd);
 
  	if (rtn == BLK_EH_DONE) {
+		/*
+		 * For blk-mq, we must set the request state to complete now
+		 * before sending the request to the scsi error handler. This
+		 * will prevent a use-after-free in the event the LLD manages
+		 * to complete the request before the error handler finishes
+		 * processing this timed out request.
+		 *

drivers/scsi/scsi_error.c enum blk_eh_timer_return scsi_times_out(struct request *req) · line 315

Quoted 10 lines

drivers/scsi/scsi_error.c +7 -0

+		 * If the request was already completed, then the LLD beat the
+		 * time out handler from transferring the request to the scsi
+		 * error handler. In that case we can return immediately as no
+		 * further action is required.
+		 */
+		if (req->q->mq_ops && !blk_mq_mark_complete(req))
+			return rtn;
  		if (scsi_abort_command(scmd) != SUCCESS) {
  			set_host_byte(scmd, DID_TIME_OUT);
  			scsi_eh_scmd_add(scmd);

This change looks incomplete to me. I think the following scenario is not handled properly by the above patch:
- host->hostt->eh_timed_out() gets called.
- The request "req" completes from another context while
host->hostt->eh_timed_out() is in progress.
- host->hostt->eh_timed_out() calls scsi_finish_command().

I think that scenario will lead to a double completion.

Bart.

Keith Busch

2018-07-25 01:15 UTC #4 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

Quoted 14 lines · Bart Van Assche

drivers/scsi/scsi_error.c enum blk_eh_timer_return scsi_times_out(struct request *req) · line 315
Quoted 5 lines · Keith Busch
drivers/scsi/scsi_error.c +2 -0
+		if (req->q->mq_ops && !blk_mq_mark_complete(req))
+			return rtn;
  		if (scsi_abort_command(scmd) != SUCCESS) {
  			set_host_byte(scmd, DID_TIME_OUT);
  			scsi_eh_scmd_add(scmd);
This change looks incomplete to me. I think the following scenario is not handled properly by the above patch:
- host->hostt->eh_timed_out() gets called.
- The request "req" completes from another context while host->hostt->eh_timed_out() is in progress.
- host->hostt->eh_timed_out() calls scsi_finish_command().

Quoted 1 line

I think that scenario will lead to a double completion.

A bit of a moot point, isn't it? Not a single scsi lld directly calls scsi_finish_command() from anywhere, much less through eh_timed_out().

Douglas Gilbert

2018-07-25 01:56 UTC #5 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

Quoted 15 lines · Keith Busch

Quoted 14 lines · Bart Van Assche
drivers/scsi/scsi_error.c enum blk_eh_timer_return scsi_times_out(struct request *req) · line 315
Quoted 5 lines · Keith Busch
drivers/scsi/scsi_error.c +2 -0
+		if (req->q->mq_ops && !blk_mq_mark_complete(req))
+			return rtn;
   		if (scsi_abort_command(scmd) != SUCCESS) {
   			set_host_byte(scmd, DID_TIME_OUT);
   			scsi_eh_scmd_add(scmd);
This change looks incomplete to me. I think the following scenario is not handled properly by the above patch:
- host->hostt->eh_timed_out() gets called.
- The request "req" completes from another context while host->hostt->eh_timed_out() is in progress.
- host->hostt->eh_timed_out() calls scsi_finish_command().

Quoted 4 lines

Quoted 1 line
I think that scenario will lead to a double completion.
A bit of a moot point, isn't it? Not a single scsi lld directly calls scsi_finish_command() from anywhere, much less through eh_timed_out().

" * scsi_finish_command - cleanup and pass command back to upper layer"

That is from the comment block above the definition of scsi_finish_command(). To me that means the mid-level has already got the response from the LLD (directly via a queuecommand() return, or asynchronously via the scsi_done() callback) and this function will call the "upper layer" which will be the one of the sd, sr, st, sg, ses, etc drivers that originated the command for that UL-driver's completion.

In USB Type C speak, the SCSI mid level has two interfaces, one downward facing (toward LLDs) and one upward facing (toward the drivers listed above). No LLD should be calling scsi_finish_command() IMO.

Doug Gilbert

Keith Busch

2018-07-25 02:48 UTC #6 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

Quoted 1 line · On Tue, Jul 24, 2018 at 09:56:35PM -0400, [email protected] wrote:

No LLD should be calling scsi_finish_command() IMO.

Agreed. I don't even think scsi's error handler should directly call it either, but detangling that may take some time, if ever.

Bart Van Assche

2018-07-25 15:52 UTC #7 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

In reply to Keith Busch

drivers/scsi/scsi_er=

Quoted 1 line · Keith Busch

drivers/scsi/scsi_er= renamed from drivers/scsi/scsi_error.c +0 -0

ror.c

Quoted 4 lines

index 8932ae81a15a..2715cdaa669c 100644
--- a/drivers/scsi/scsi_error.c
+-+-+- b/drivers/scsi/scsi_error.c
@@ -296,6 +-296,20 @@ enum blk_eh_timer_retur=

n scsi_times_out(struct request *req)

Quoted 11 lines

rtn = host->hostt->eh_timed_out(scmd);
=20
if (rtn == BLK_EH_DONE) {

+-		/*
+-		 * For blk-mq, we must set the request state to complete now
+-		 * before sending the request to the scsi error handler. This
+-		 * will prevent a use-after-free in the event the LLD manages
+-		 * to complete the request before the error handler finishes
+-		 * processing this timed out request.
+-		 *
+-		 * If the request was already completed, then the LLD beat th=

Quoted 2 lines

+-		 * time out handler from transferring the request to the scsi
+-		 * error handler. In that case we can return immediately as n=

Quoted 3 lines

+-		 * further action is required.
+-		 */
+-		if (req->q->mq_ops && !blk_mq_mark+=

AF8-complete(req))

drivers/scsi/scsi_er=

Quoted 4 lines

+- return rtn;
drivers/scsi/scsi_er= renamed from drivers/scsi/scsi_error.c +0 -0
 		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);

Hello Keith,

What will happen if a completion occurs after scsi_times_out() has = started and
before or during the host->hostt->eh_timed_out()? Can that = cause a use-after-free
in .eh_timed_out()? Can that cause .eh_timed_out() to retur= n BLK_EH_RESET_TIMER
when it should return BLK_EH_DONE? Can that cause blk_mq_rq= _timed_out() to call
blk_add_timer() when that function shouldn't be called?

Thanks,

Bart.

Keith Busch

2018-07-25 16:48 UTC #8 lore raw

Re: [PATCHv2 2/2] scsi: set timed out out mq requests to complete

Quoted 29 lines · Bart Van Assche

Quoted 15 lines · Keith Busch

drivers/scsi/scsi_error.c +7 -0

index 8932ae81a15a..2715cdaa669c 100644
@@ -296,6 +296,20 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_DONE) {
+		/*
+		 * For blk-mq, we must set the request state to complete now
+		 * before sending the request to the scsi error handler. This
+		 * will prevent a use-after-free in the event the LLD manages
+		 * to complete the request before the error handler finishes
+		 * processing this timed out request.
+		 *

drivers/scsi/scsi_error.c enum blk_eh_timer_return scsi_times_out(struct request *req) · line 315

Quoted 10 lines

drivers/scsi/scsi_error.c +7 -0

+		 * If the request was already completed, then the LLD beat the
+		 * time out handler from transferring the request to the scsi
+		 * error handler. In that case we can return immediately as no
+		 * further action is required.
+		 */
+		if (req->q->mq_ops && !blk_mq_mark_complete(req))
+			return rtn;
 		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);

Hello Keith,

Quoted 5 lines

What will happen if a completion occurs after scsi_times_out() has started and before or during the host->hostt->eh_timed_out()? Can that cause a use-after-free in .eh_timed_out()? Can that cause .eh_timed_out() to return BLK_EH_RESET_TIMER when it should return BLK_EH_DONE? Can that cause blk_mq_rq_timed_out() to call blk_add_timer() when that function shouldn't be called?

That's what the request's refcount protects. The whole point was that driver returning RESET_TIMER doesn't lose the completion. In the worst case scenario, the blk-mq timeout work spends some CPU cycles re-arming a timer that it didn't need to.

Christoph Hellwig

maintainer Reviewed-by on v2 1/2

2018-07-24 07:56 UTC #9 lore raw

In reply to Keith Busch

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Jens Axboe

maintainer

2018-07-24 20:42 UTC #10 lore raw

In reply to Keith Busch

Quoted 2 lines · Keith Busch

This is preparing for drivers that want to directly alter the state of their requests. No functional change here.

Added this (and 2/2) for 4.18...

Signature · 1 line

Jens Axboe