sd: make error handling more robust (v2)

PATCH In review Tony Battersby 6 participants 15 messages Feb 1, 2008 – Feb 6, 2008

Subsystems (2)

SCSI Aacraid SCSI RAID driver

lore

Share & tools

lore

Tony Battersby

author

2008-02-01 17:03 UTC #0 lore raw

PATCH 1/1 sd: make error handling more robust (v2)

This patch fixes a problem with some out-of-spec SCSI disks that report hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a failed read command.

Signed-off-by Tony Battersby <[email protected]>

---

This is a simplified version of the original patch that fixes just the problem at hand, without trying to handle other theoretical out-of-spec cases.

Applies to kernels 2.6.18 - 2.6.24-git10+.

drivers/scsi/sd.c renamed from drivers/scsi/sd.c.orig +2 -0

@@ -990,6 +990,8 @@ static int sd_done(struct scsi_cmnd *SCp
 		/* This computation should always be done in terms of
 		 * the resolution of the device's medium.
 		 */
+		if (bad_lba < start_lba)
+			goto out;
 		good_bytes = (bad_lba - start_lba)*SCpnt->device->sector_size;
 		break;
 	case RECOVERED_ERROR:

Luben Tuikov

Acked-by on v1 1/1

2008-02-01 20:13 UTC #1 lore raw

Quoted 13 lines · Tony Battersby

From: Tony Battersby <[email protected]>
Subject: [PATCH] [SCSI] sd: make error handling more robust (v2)
To: "[email protected]" <[email protected]>, "James Bottomley" <[email protected]>
Cc: "Luben Tuikov" <[email protected]>, "Salyzyn, Mark" <[email protected]>
Date: Friday, February 1, 2008, 9:03 AM
This patch fixes a problem with some out-of-spec SCSI disks
that report
hardware or medium errors incorrectly. Without the patch,
the kernel
may silently ignore a failed write command or return
corrupted data on a
failed read command.

Quoted 1 line

Signed-off-by Tony Battersby <[email protected]>

Acked-by Luben Tuikov <[email protected]>

Quoted 3 lines

---
This is a simplified version of the original patch that

Very simplified. ;-)

Quoted 4 lines

fixes just the
problem at hand, without trying to handle other theoretical out-of-spec
cases.

Yes, it is chunk #2 of the patch I sent to you to
try on your hardware.

Quoted 26 lines

Applies to kernels 2.6.18 - 2.6.24-git10+.
--- linux-2.6.24-git10/drivers/scsi/sd.c.orig 2008-02-01
11:24:37.000000000 -0500
+++ linux-2.6.24-git10/drivers/scsi/sd.c 2008-02-01
11:26:12.000000000 -0500
drivers/scsi/sd.c renamed from drivers/scsi/sd.c.orig +2 -0
@@ -990,6 +990,8 @@ static int sd_done(struct scsi_cmnd
 *SCp
 		/* This computation should always be done in terms of
 		 * the resolution of the device's medium.
 		 */
+		if (bad_lba < start_lba)
+			goto out;
 		good_bytes = (bad_lba -
 start_lba)*SCpnt->device->sector_size;
(quoted diff) +0 -1
 		break;
 	case RECOVERED_ERROR:
 
 
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

Salyzyn, Mark

2008-02-01 20:47 UTC #2 lore raw

In reply to Tony Battersby

ACK

Signed-off-by Mark Salyzyn <[email protected]>

Mike Snitzer tested this also in conjunction with the aacraid driver and it resolved the error propagation problem he experienced with MD on a 2.6.22.16 kernel, recommend that this also be added to any stabilization for these other kernels. By his permission:

Signed-off-by Mike Snitzer <[email protected]>

Sincerely -- Mark Salyzyn

Quoted 41 lines

-----Original Message-----
From: Tony Battersby [mailto:[email protected]]
Sent: Friday, February 01, 2008 12:03 PM
To: [email protected]; James Bottomley
Cc: Luben Tuikov; Salyzyn, Mark
Subject: [PATCH] [SCSI] sd: make error handling more robust (v2)
This patch fixes a problem with some out-of-spec SCSI disks that report
hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a
failed read command.
Signed-off-by Tony Battersby <[email protected]>
---
This is a simplified version of the original patch that fixes just the problem at hand, without trying to handle other theoretical out-of-spec
cases.
Applies to kernels 2.6.18 - 2.6.24-git10+.
--- linux-2.6.24-git10/drivers/scsi/sd.c.orig 2008-02-01
11:24:37.000000000 -0500
+++ linux-2.6.24-git10/drivers/scsi/sd.c 2008-02-01
11:26:12.000000000 -0500
drivers/scsi/sd.c renamed from drivers/scsi/sd.c.orig +2 -0
@@ -990,6 +990,8 @@ static int sd_done(struct scsi_cmnd *SCp
                /* This computation should always be done in terms of
                 * the resolution of the device's medium.
                 */
+               if (bad_lba < start_lba)
+                       goto out;
                good_bytes = (bad_lba -
 start_lba)*SCpnt->device->sector_size;
                break;
case RECOVERED_ERROR:

Mike Snitzer

2008-02-02 00:43 UTC #3 lore raw

Hi Greg,

Given your recent call for review of the next 2.6.22-stable I'd appreciate it if you and the rest of the stable team would strongly consider this SCSI IO error propagation fix for inclusion in 2.6.22.17 (as well as the other stable trees).

please advise, thanks.
Mike

Quoted 52 lines · Mark

ACK
Signed-off-by Mark Salyzyn <[email protected]>
Mike Snitzer tested this also in conjunction with the aacraid driver and it resolved the error propagation problem he experienced with MD on a 2.6.22.16 kernel, recommend that this also be added to any stabilization for these other kernels. By his permission:
Signed-off-by Mike Snitzer <[email protected]>
Sincerely -- Mark Salyzyn
Quoted 41 lines
-----Original Message-----
From: Tony Battersby [mailto:[email protected]]
Sent: Friday, February 01, 2008 12:03 PM
To: [email protected]; James Bottomley
Cc: Luben Tuikov; Salyzyn, Mark
Subject: [PATCH] [SCSI] sd: make error handling more robust (v2)
This patch fixes a problem with some out-of-spec SCSI disks that report
hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a
failed read command.
Signed-off-by Tony Battersby <[email protected]>
---
This is a simplified version of the original patch that fixes just the problem at hand, without trying to handle other theoretical out-of-spec
cases.
Applies to kernels 2.6.18 - 2.6.24-git10+.
--- linux-2.6.24-git10/drivers/scsi/sd.c.orig 2008-02-01
11:24:37.000000000 -0500
+++ linux-2.6.24-git10/drivers/scsi/sd.c 2008-02-01
11:26:12.000000000 -0500
drivers/scsi/sd.c renamed from drivers/scsi/sd.c.orig +2 -0
@@ -990,6 +990,8 @@ static int sd_done(struct scsi_cmnd *SCp
                /* This computation should always be done in terms of
                 * the resolution of the device's medium.
                 */
+               if (bad_lba < start_lba)
+                       goto out;
                good_bytes = (bad_lba -
 start_lba)*SCpnt->device->sector_size;
                break;
case RECOVERED_ERROR:

Greg KH

2008-02-02 20:40 UTC #4 lore raw

Quoted 6 lines · Mike Snitzer

Hi Greg,
Given your recent call for review of the next 2.6.22-stable I'd appreciate it if you and the rest of the stable team would strongly consider this SCSI IO error propagation fix for inclusion in 2.6.22.17 (as well as the other stable trees).

Is the patch in Linus's tree already?

If so, what git commit id? That's a requirement to go into the stable tree...

If not, we need to wait until it is.

thanks,

greg k-h

Mike Snitzer

2008-02-02 20:56 UTC #5 lore raw

Quoted 8 lines · Greg KH

Quoted 6 lines · Mike Snitzer
Hi Greg,
Given your recent call for review of the next 2.6.22-stable I'd appreciate it if you and the rest of the stable team would strongly consider this SCSI IO error propagation fix for inclusion in 2.6.22.17 (as well as the other stable trees).

Quoted 6 lines

Is the patch in Linus's tree already?
If so, what git commit id? That's a requirement to go into the stable tree...
If not, we need to wait until it is.

Fair enough. Thanks for the clarification.

That likely leaves us with having James add it to his git tree for Linus (provided he agrees with the change). Which he and Luben seemed to agree was the reasonable thin layer of protection that would be allowed for such out of spec SCSI devices.

James, how would you like to proceed? The patch fixes the good_bytes != 0 on HARDWARE_ERROR issue I have been chasing with Mark Salyzyn.

Thanks,
Mike

Greg KH

2008-02-02 21:17 UTC #6 lore raw

Quoted 18 lines · Mike Snitzer

Quoted 8 lines · Greg KH
Quoted 6 lines · Mike Snitzer
Hi Greg,
Given your recent call for review of the next 2.6.22-stable I'd appreciate it if you and the rest of the stable team would strongly consider this SCSI IO error propagation fix for inclusion in 2.6.22.17 (as well as the other stable trees).
Quoted 6 lines
Is the patch in Linus's tree already?
If so, what git commit id? That's a requirement to go into the stable tree...
If not, we need to wait until it is.
Fair enough. Thanks for the clarification.

Quoted 7 lines

That likely leaves us with having James add it to his git tree for Linus (provided he agrees with the change). Which he and Luben seemed to agree was the reasonable thin layer of protection that would be allowed for such out of spec SCSI devices.
James, how would you like to proceed? The patch fixes the good_bytes != 0 on HARDWARE_ERROR issue I have been chasing with Mark Salyzyn.

After it goes in, can someone notify [email protected] that it went in, the git id, and that it should be applied to all older kernel trees?

thanks,

greg "drowning in -stable patches" k-h

James Bottomley

maintainer

2008-02-02 22:06 UTC #7 lore raw

In reply to Tony Battersby

Quoted 11 lines · Tony Battersby

This patch fixes a problem with some out-of-spec SCSI disks that report hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a failed read command.
Signed-off-by Tony Battersby <[email protected]>
---
This is a simplified version of the original patch that fixes just the problem at hand, without trying to handle other theoretical out-of-spec cases.

Actually, to restore the original check, this is what we want, isn't it? Ok, so I also made the sector division logic futureproof for the day we have > 4096 sector devices ...

James

---

Quoted 1 line

From 5ae2e4a8ff095aab5997f17068d3e4212c33f039 Mon Sep 17 00:00:00 2001

From: Tony Battersby <[email protected]>
Date: Fri, 1 Feb 2008 12:03:27 -0500
Subject: [SCSI] sd: make error handling more robust

Signed-off-by Tony Battersby <[email protected]>

Cc Stable Tree <[email protected]>

Signed-off-by James Bottomley <[email protected]>

---
 drivers/scsi/sd.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/scsi/sd.c +16 ~~-18~~

Index: BUILD-2.6/drivers/scsi/sd.c
===================================================================
@@ -929,6 +929,7 @@
 	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
+	u64 end_lba = SCpnt->request->sector + (xfer_size / 512);
  	u64 bad_lba;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;

@@ -967,26 +968,23 @@
 			goto out;
 		if (xfer_size <= SCpnt->device->sector_size)
 			goto out;
-		switch (SCpnt->device->sector_size) {
-		case 256:
+		if (SCpnt->device->sector_size < 512) {
+			/* only legitimate sector_size here is 256 */
 			start_lba <<= 1;
-			break;
-		case 512:
-			break;
-		case 1024:
-			start_lba >>= 1;
-			break;
-		case 2048:
-			start_lba >>= 2;
-			break;
-		case 4096:
-			start_lba >>= 3;
-			break;
-		default:
-			/* Print something here with limiting frequency. */
-			goto out;
-			break;
+			end_lba <<= 1;
+		} else {
+			/* be careful ... don't want any overflows */
+			u64 factor = SCpnt->device->sector_size / 512;
+			do_div(start_lba, factor);
+			do_div(end_lba, factor);
 		}
+
+		if (bad_lba < start_lba  || bad_lba >= end_lba)
+			/* the bad lba was reported incorrectly, we have
+			 * no idea where the error is
+			 */
+			goto out;
+
 		/* This computation should always be done in terms of
 		 * the resolution of the device's medium.
 		 */

Mike Snitzer

Tested-by

2008-02-03 15:15 UTC #8 lore raw

Quoted 27 lines · James Bottomley

Quoted 11 lines · Tony Battersby
This patch fixes a problem with some out-of-spec SCSI disks that report hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a failed read command.
Signed-off-by Tony Battersby <[email protected]>
---
This is a simplified version of the original patch that fixes just the problem at hand, without trying to handle other theoretical out-of-spec cases.
Actually, to restore the original check, this is what we want, isn't it? Ok, so I also made the sector division logic futureproof for the day we have > 4096 sector devices ...
James
---
Quoted 1 line
From 5ae2e4a8ff095aab5997f17068d3e4212c33f039 Mon Sep 17 00:00:00 2001
From: Tony Battersby <[email protected]>
Date: Fri, 1 Feb 2008 12:03:27 -0500
Subject: [SCSI] sd: make error handling more robust

Quoted 8 lines

This patch fixes a problem with some out-of-spec SCSI disks that report hardware or medium errors incorrectly. Without the patch, the kernel may silently ignore a failed write command or return corrupted data on a failed read command.
Signed-off-by Tony Battersby <[email protected]>
Cc Stable Tree <[email protected]>
Signed-off-by James Bottomley <[email protected]>

I've verified that this patch fixes the 2.6.22.16 SCSI IO error propagation issue I had when physically pulling a drive from an enclosure connected to an aacraid controller.

Tested-by Mike Snitzer <[email protected]>

Mike

Luben Tuikov

2008-02-04 09:14 UTC #9 lore raw

Quoted 56 lines · Mike Snitzer

From: Mike Snitzer <[email protected]>
Subject: Re: [PATCH] [SCSI] sd: make error handling more robust (v2)
To: "James Bottomley" <[email protected]>
Cc: "Tony Battersby" <[email protected]>, "[email protected]" <[email protected]>, "Luben Tuikov" <[email protected]>, "Salyzyn, Mark" <[email protected]>
Date: Sunday, February 3, 2008, 7:14 AM
Quoted 2 lines · James Bottomley
On Fri, 2008-02-01 at 12:03 -0500, Tony Battersby
Quoted 1 line · wrote:
Quoted 1 line
This patch fixes a problem with some out-of-spec
SCSI disks that report
Quoted 1 line
Quoted 1 line
hardware or medium errors incorrectly. Without
the patch, the kernel
Quoted 1 line
Quoted 1 line
may silently ignore a failed write command or
return corrupted data on a
Quoted 3 lines
Quoted 3 lines
failed read command.
Signed-off-by Tony Battersby
<[email protected]>
Quoted 3 lines
Quoted 3 lines
---
This is a simplified version of the original
patch that fixes just the
Quoted 1 line
Quoted 1 line
problem at hand, without trying to handle other
theoretical out-of-spec
Quoted 3 lines
Quoted 1 line
cases.
Actually, to restore the original check, this is what
we want, isn't it?
Quoted 1 line
Ok, so I also made the sector division logic
futureproof for the day we
Quoted 7 lines
have > 4096 sector devices ...
James
---
Quoted 1 line
From 5ae2e4a8ff095aab5997f17068d3e4212c33f039 Mon
Sep 17 00:00:00 2001
Quoted 5 lines
From: Tony Battersby <[email protected]>
Date: Fri, 1 Feb 2008 12:03:27 -0500
Subject: [SCSI] sd: make error handling more robust
This patch fixes a problem with some out-of-spec SCSI
disks that report
Quoted 1 line
hardware or medium errors incorrectly. Without the
patch, the kernel
Quoted 1 line
may silently ignore a failed write command or return
corrupted data on a
Quoted 3 lines
failed read command.
Signed-off-by Tony Battersby
<[email protected]>
Quoted 2 lines
Cc Stable Tree <[email protected]>
Signed-off-by James Bottomley

Quoted 7 lines

<[email protected]>
I've verified that this patch fixes the 2.6.22.16 SCSI IO error
propagation issue I had when physically pulling a drive from an
enclosure connected to an aacraid controller.

Just as in your case and Tony's case, which I presume uses the same RAID firmware vendor, it would've probably been better if the RAID firmware vendor fixed the firmware to not set the VALID bit if the INFORMATION field is not valid.

Luben

Salyzyn, Mark

2008-02-06 21:54 UTC #10 lore raw

PATCH 1/1 aacraid: do not set valid bit in sense information

Luben Tuikov [mailto:[email protected]] sez:

Quoted 5 lines

Just as in your case and Tony's case, which I presume uses the same RAID firmware vendor, it would've probably been better if the RAID firmware vendor fixed the firmware to not set the VALID bit if the INFORMATION field is not valid.

Point taken regarding the aacraid driver. Dropped the VALID bit, and then did some cleanup/simplification of the set_sense procedure and the associated parameters. Mike did some preliminary tests when the VALID bit was dropped before the 'Re: [PATCH] [SCSI] sd: make error handling more robust' patches came on the scene. The change in the SCSI subsystem does make this enclosed aacraid patch unnecessary, so this aacraid patch is merely post battle ground cleanup. If the simplification is an issue, repugnant, too much for a back-port to the stable trees or clouds the point, this patch could be happily distilled down to:

drivers/scsi/aacraid/aachba.c 2008-02-06 16:32:01.109035329 -0500 renamed from drivers/scsi/aacraid/aachba.c 2008-02-06 16:26:45.834938955 -0500 +1 -1

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
@@ -865,7 +865,7 @@
                         u32 residue)
 {
-        sense_buf[0] = 0xF0;    /* Sense data valid, err code 70h (current error) */
+        sense_buf[0] = 0x70;    /* Sense data invalid, err code 70h (current error) */
         sense_buf[1] = 0;       /* Segment number, always zero */
 
         if (incorrect_length) {

This patch is against current scsi-misc-2.6

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments. Please use the attached patch and not any inline content.

Signed-off-by Mark Salyzyn <[email protected]>

 drivers/scsi/aacraid/aachba.c |   81 +++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 51 deletions(-)

Sincerely -- Mark Salyzyn

drivers/scsi/aacraid/aachba.c +30 ~~-51~~

136 lines not on this page · raw message

Luben Tuikov

2008-02-06 22:19 UTC #11 lore raw

Re: [PATCH 1/1] aacraid: do not set valid bit in sense information

Quoted 13 lines · Mark

From: Salyzyn, Mark <[email protected]>
Subject: [PATCH 1/1] aacraid: do not set valid bit in sense information
To: "[email protected]" <[email protected]>
Cc: "Tony Battersby" <[email protected]>, "[email protected]" <[email protected]>, "Mike Snitzer" <[email protected]>
Date: Wednesday, February 6, 2008, 1:54 PM
Luben Tuikov [mailto:[email protected]] sez:
Quoted 1 line
Just as in your case and Tony's case, which I
presume
Quoted 4 lines
uses the same RAID firmware vendor, it would've probably been better if the RAID firmware vendor fixed the firmware to not set the VALID bit if the INFORMATION field is not valid.

Quoted 12 lines

Point taken regarding the aacraid driver. Dropped the VALID bit, and then did some cleanup/simplification of the set_sense procedure and the associated parameters. Mike did some preliminary tests when the VALID bit was dropped before the 'Re: [PATCH] [SCSI] sd: make error handling more robust' patches came on the scene. The change in the SCSI subsystem does make this enclosed aacraid patch unnecessary, so this aacraid patch is merely post battle ground cleanup. If the simplification is an issue, repugnant, too much for a back-port to the stable trees or clouds the point, this patch could be happily distilled down to:

Sounds good.

Luben

Quoted 32 lines

diff -ru a/drivers/scsi/aacraid/aachba.c
b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c 2008-02-06
16:26:45.834938955 -0500
+++ b/drivers/scsi/aacraid/aachba.c 2008-02-06
16:32:01.109035329 -0500
drivers/scsi/aacraid/aachba.c 2008-02-06 16:32:01.109035329 -0500 renamed from drivers/scsi/aacraid/aachba.c 2008-02-06 16:26:45.834938955 -0500 +0 -1
@@ -865,7 +865,7 @@
                         u32 residue)
 {
-        sense_buf[0] = 0xF0;    /* Sense data valid, err
code 70h (current error) */
+ sense_buf[0] = 0x70; /* Sense data invalid, err
code 70h (current error) */
sense_buf[1] = 0; /* Segment number, always
zero */
         if (incorrect_length) {
This patch is against current scsi-misc-2.6
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments. Please use the attached patch and not any inline content.
Signed-off-by Mark Salyzyn <[email protected]>
 drivers/scsi/aacraid/aachba.c |   81
+++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 51 deletions(-)
Sincerely -- Mark Salyzyn

Luben Tuikov

2008-02-04 09:11 UTC #12 lore raw

In reply to James Bottomley

Quoted 103 lines · James Bottomley

From: James Bottomley <[email protected]>
Subject: Re: [PATCH] [SCSI] sd: make error handling more robust (v2)
To: "Tony Battersby" <[email protected]>
Cc: "[email protected]" <[email protected]>, "Luben Tuikov" <[email protected]>, "Salyzyn, Mark" <[email protected]>
Date: Saturday, February 2, 2008, 2:06 PM
Quoted 1 line · Tony Battersby
This patch fixes a problem with some out-of-spec SCSI
disks that report
Quoted 1 line
hardware or medium errors incorrectly. Without the
patch, the kernel
Quoted 1 line
may silently ignore a failed write command or return
corrupted data on a
Quoted 3 lines
failed read command.
Signed-off-by Tony Battersby
<[email protected]>
Quoted 3 lines
---
This is a simplified version of the original patch
that fixes just the
Quoted 1 line
problem at hand, without trying to handle other
theoretical out-of-spec
Quoted 1 line
cases.
Actually, to restore the original check, this is what we want, isn't it?
Ok, so I also made the sector division logic futureproof for the day we
have > 4096 sector devices ...
James
---
Quoted 1 line
From 5ae2e4a8ff095aab5997f17068d3e4212c33f039 Mon Sep
17 00:00:00 2001
From: Tony Battersby <[email protected]>
Date: Fri, 1 Feb 2008 12:03:27 -0500
Subject: [SCSI] sd: make error handling more robust
This patch fixes a problem with some out-of-spec SCSI disks that report
hardware or medium errors incorrectly. Without the patch, the kernel
may silently ignore a failed write command or return corrupted data on a
failed read command.
Signed-off-by Tony Battersby <[email protected]>
Cc Stable Tree <[email protected]>
Signed-off-by James Bottomley
<[email protected]>
---
 drivers/scsi/sd.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Index: BUILD-2.6/drivers/scsi/sd.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/sd.c 2008-02-02
15:43:20.000000000 -0600
+++ BUILD-2.6/drivers/scsi/sd.c 2008-02-02
16:01:24.000000000 -0600
drivers/scsi/sd.c +1 -0
@@ -929,6 +929,7 @@
 	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
+	u64 end_lba = SCpnt->request->sector + (xfer_size /
 512);
  	u64 bad_lba;
 	struct scsi_sense_hdr sshdr;
drivers/scsi/sd.c +8 ~~-18~~
 	int sense_valid = 0;
@@ -967,26 +968,23 @@
 			goto out;
 		if (xfer_size <= SCpnt->device->sector_size)
 			goto out;
-		switch (SCpnt->device->sector_size) {
-		case 256:
+		if (SCpnt->device->sector_size < 512) {
+			/* only legitimate sector_size here is 256 */
 			start_lba <<= 1;
-			break;
-		case 512:
-			break;
-		case 1024:
-			start_lba >>= 1;
-			break;
-		case 2048:
-			start_lba >>= 2;
-			break;
-		case 4096:
-			start_lba >>= 3;
-			break;
-		default:
-			/* Print something here with limiting frequency. */
-			goto out;
-			break;
+			end_lba <<= 1;
+		} else {
+			/* be careful ... don't want any overflows */
+			u64 factor = SCpnt->device->sector_size / 512;
+			do_div(start_lba, factor);
+			do_div(end_lba, factor);
 		}

drivers/scsi/sd.c line 990

Quoted 10 lines

drivers/scsi/sd.c +7 -0

+
+		if (bad_lba < start_lba  || bad_lba >= end_lba)
+			/* the bad lba was reported incorrectly, we have
+			 * no idea where the error is
+			 */
+			goto out;
+
 		/* This computation should always be done in terms of
 		 * the resolution of the device's medium.
 		 */

Looks good except that "End LBA" is usually defined to be something of the sort of "the LBA of the last logical block accessed by the command" or "the LBA of the logical block on which the command failed".

A spec savvy editor of this code would be "pleasantly" surprised if they had to use "end_lba", and didn't pay attention that it was actually "End LBA" + 1.

Luben

James Bottomley

maintainer

2008-02-04 21:22 UTC #13 lore raw

Quoted 9 lines · Luben Tuikov

Looks good except that "End LBA" is usually defined to be something of the sort of "the LBA of the last logical block accessed by the command" or "the LBA of the logical block on which the command failed".
A spec savvy editor of this code would be "pleasantly" surprised if they had to use "end_lba", and didn't pay attention that it was actually "End LBA" + 1.

Heh, well, that's where spec people and programmers part company. The universal expectation of a programmer in looping is

for (a = beginning; a < end; a++)

rather than <= if end were actually to point to last rather than last + 1.

James

Luben Tuikov

2008-02-04 23:23 UTC #14 lore raw

Quoted 20 lines · James Bottomley

Quoted 1 line · Luben Tuikov
Looks good except that "End LBA" is usually
defined
Quoted 1 line
to be something of the sort of "the LBA of the
last
Quoted 1 line
logical block accessed by the command" or
"the LBA
Quoted 1 line
of the logical block on which the command
failed".
Quoted 3 lines
A spec savvy editor of this code would be
"pleasantly" surprised if they had to use
"end_lba",
Quoted 2 lines
and didn't pay attention that it was actually
"End LBA" + 1.
Heh, well, that's where spec people and programmers part company. The
universal expectation of a programmer in looping is

Quoted 5 lines

for (a = beginning; a < end; a++)
rather than <= if end were actually to point to last rather than last +
1.

For loop invariants that's true, although I didn't see a loop in sd_done().

Luben