From: Boaz Harrosh <bharrosh@panasas.com>
To: James Bottomley <James.Bottomley@suse.de>,
linux-scsi <linux-scsi@vger.kernel.org>,
Alan Stern <stern@rowland.harvard.edu>,
Jeff Garzik <jeff@garzik.org>
Cc: Christoph Hellwig <hch@infradead.org>,
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] scsi_lib: BUG: Can't RETRY scsi_cmnd if some bytes were completed
Date: Mon, 08 Nov 2010 18:18:58 +0200 [thread overview]
Message-ID: <4CD822F2.6060101@panasas.com> (raw)
In-Reply-To: <4CD810FA.902@panasas.com>
Re-inspecting the code after the last cleanup actually exposed
a BAD bug for me. See below. James this is based on the last
patchset I sent.
Boaz
---
From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [PATCH] scsi_lib: BUG: Can't RETRY scsi_cmnd if some bytes were completed
In scsi_io_completion() there are many cases where action is
set to ACTION_RETRY or ACTION_DELAYED_RETRY. But we are not
allowed to just RETRY a command if some bytes where already
completed by blk_end_request(). This is a bad memory overrun
of DMA writing/reading random memory. We must re-prepare the
command in this case.
It is possible that all the cases that set ACTION_RETRY* have
actually come with good_bytes=0 (.i.e resid = everything) But
both the error and resid value come from LLDs and/or targets
and should not be trusted with such a BAD crash. Better safe
than sorry.
It is possible that this fix is actually not good enough and
in the case of some of these RETRYs we need to not call
blk_end_request() in the first place. But this calls for
a structural reorganisation of scsi_io_completion(). James
this is your turf please have a look.
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d76a69b..b78b34e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -827,6 +827,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
action = ACTION_FAIL;
}
+ if (action >= ACTION_RETRY && good_bytes)
+ /* We cannot just retry if above blk_end_request advanced on
+ * some good_bytes, so ACTION_REPREP
+ */
+ action = ACTION_REPREP;
+
switch (action) {
case ACTION_NEXT_CMND:
scsi_release_buffers(cmd);
--
1.7.2.3
prev parent reply other threads:[~2010-11-08 16:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-08 15:02 [PATCH 0/3] scsi_lib: Some love to scsi_lib Boaz Harrosh
2010-11-08 15:10 ` [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command Boaz Harrosh
2010-11-08 15:11 ` [PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption Boaz Harrosh
2010-11-08 15:15 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2010-11-08 16:18 ` Boaz Harrosh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4CD822F2.6060101@panasas.com \
--to=bharrosh@panasas.com \
--cc=James.Bottomley@suse.de \
--cc=fujita.tomonori@lab.ntt.co.jp \
--cc=hch@infradead.org \
--cc=jeff@garzik.org \
--cc=linux-scsi@vger.kernel.org \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox