On Wed, 2018-11-14 at 09:26 -0700, Keith Busch wrote: +AD4 The scsi timeout error handling had been directly updating the request +AD4 state to prevent a natural completion and error handling from completing +AD4 the same request twice. Fix this layering violation by having scsi +AD4 control the fate of its commands with scsi owned flags rather than +AD4 use blk-mq's. +AD4 +AD4 Signed-off-by: Keith Busch +ADw-keith.busch+AEA-intel.com+AD4 +AD4 --- +AD4 drivers/scsi/scsi+AF8-error.c +AHw 17 +-+-+--------------- +AD4 drivers/scsi/scsi+AF8-lib.c +AHw 6 +-+-+-+-+-- +AD4 include/scsi/scsi+AF8-cmnd.h +AHw 5 +-+-+-+-- +AD4 3 files changed, 12 insertions(+-), 16 deletions(-) +AD4 +AD4 diff --git a/drivers/scsi/scsi+AF8-error.c b/drivers/scsi/scsi+AF8-error.c +AD4 index c736d61b1648..f89e829a1c51 100644 +AD4 --- a/drivers/scsi/scsi+AF8-error.c +AD4 +-+-+- b/drivers/scsi/scsi+AF8-error.c +AD4 +AEAAQA -199,6 +-199,9 +AEAAQA scsi+AF8-abort+AF8-command(struct scsi+AF8-cmnd +ACo-scmd) +AD4 return FAILED+ADs +AD4 +AH0 +AD4 +AD4 +- if (test+AF8-and+AF8-set+AF8-bit(+AF8AXw-SCMD+AF8-COMPLETE, +ACY-scmd-+AD4-flags)) +AD4 +- return SUCCESS+ADs +AD4 +- +AD4 spin+AF8-lock+AF8-irqsave(shost-+AD4-host+AF8-lock, flags)+ADs +AD4 if (shost-+AD4-eh+AF8-deadline +ACEAPQ -1 +ACYAJg +ACE-shost-+AD4-last+AF8-reset) +AD4 shost-+AD4-last+AF8-reset +AD0 jiffies+ADs +AD4 +AEAAQA -296,20 +-299,6 +AEAAQA enum blk+AF8-eh+AF8-timer+AF8-return scsi+AF8-times+AF8-out(struct request +AD4 +ACo-req) +AD4 rtn +AD0 host-+AD4-hostt-+AD4-eh+AF8-timed+AF8-out(scmd)+ADs +AD4 +AD4 if (rtn +AD0APQ BLK+AF8-EH+AF8-DONE) +AHs +AD4 - /+ACo +AD4 - +ACo For blk-mq, we must set the request state to complete +AD4 now +AD4 - +ACo before sending the request to the scsi error handler. +AD4 This +AD4 - +ACo will prevent a use-after-free in the event the LLD +AD4 manages +AD4 - +ACo to complete the request before the error handler +AD4 finishes +AD4 - +ACo processing this timed out request. +AD4 - +ACo +AD4 - +ACo If the request was already completed, then the LLD beat +AD4 the +AD4 - +ACo time out handler from transferring the request to the +AD4 scsi +AD4 - +ACo error handler. In that case we can return immediately +AD4 as no +AD4 - +ACo further action is required. +AD4 - +ACo-/ +AD4 - if (req-+AD4-q-+AD4-mq+AF8-ops +ACYAJg +ACE-blk+AF8-mq+AF8-mark+AF8-complete(req)) +AD4 - return rtn+ADs +AD4 if (scsi+AF8-abort+AF8-command(scmd) +ACEAPQ SUCCESS) +AHs +AD4 set+AF8-host+AF8-byte(scmd, DID+AF8-TIME+AF8-OUT)+ADs +AD4 scsi+AF8-eh+AF8-scmd+AF8-add(scmd)+ADs +AD4 diff --git a/drivers/scsi/scsi+AF8-lib.c b/drivers/scsi/scsi+AF8-lib.c +AD4 index c7fccbb8f554..1e74137f1073 100644 +AD4 --- a/drivers/scsi/scsi+AF8-lib.c +AD4 +-+-+- b/drivers/scsi/scsi+AF8-lib.c +AD4 +AEAAQA -2044,8 +-2044,11 +AEAAQA static int scsi+AF8-mq+AF8-prep+AF8-fn(struct request +ACo-req) +AD4 +AD4 static void scsi+AF8-mq+AF8-done(struct scsi+AF8-cmnd +ACo-cmd) +AD4 +AHs +AD4 +- if (test+AF8-and+AF8-set+AF8-bit(+AF8AXw-SCMD+AF8-COMPLETE, +ACY-cmd-+AD4-flags)) +AD4 +- return+ADs +AD4 trace+AF8-scsi+AF8-dispatch+AF8-cmd+AF8-done(cmd)+ADs +AD4 - blk+AF8-mq+AF8-complete+AF8-request(cmd-+AD4-request)+ADs +AD4 +- if (unlikely(+ACE-blk+AF8-mq+AF8-complete+AF8-request(cmd-+AD4-request))) +AD4 +- clear+AF8-bit(+AF8AXw-SCMD+AF8-COMPLETE, +ACY-cmd-+AD4-flags)+ADs +AD4 +AH0 +AD4 +AD4 static void scsi+AF8-mq+AF8-put+AF8-budget(struct blk+AF8-mq+AF8-hw+AF8-ctx +ACo-hctx) +AD4 +AEAAQA -2104,6 +-2107,7 +AEAAQA static blk+AF8-status+AF8-t scsi+AF8-queue+AF8-rq(struct +AD4 blk+AF8-mq+AF8-hw+AF8-ctx +ACo-hctx, +AD4 goto out+AF8-dec+AF8-host+AF8-busy+ADs +AD4 req-+AD4-rq+AF8-flags +AHwAPQ RQF+AF8-DONTPREP+ADs +AD4 +AH0 else +AHs +AD4 +- cmd-+AD4-flags +ACYAPQ +AH4-SCMD+AF8-COMPLETE+ADs +AD4 blk+AF8-mq+AF8-start+AF8-request(req)+ADs +AD4 +AH0 Hi Keith, Please Cc Martin Petersen and the scsi mailing list for SCSI patches. Regarding this patch: I think this patch introduces a subtle but severe bug in the SCSI core, namely that if an abort is processed concurrently with request completion with +ACI-fake timeout+ACI enabled that the abort is ignored. Bart.
