drivers/scsi/sd.c: fix uninitialized variable in handling medium errors

PATCH In review Mark Lord 6 participants 14 messages Apr 26, 2006 – Apr 27, 2006 SCSI

lore

Share & tools

lore

Mark Lord

author

2006-04-26 20:27 UTC #0 lore raw

PATCH 1/1 drivers/scsi/sd.c: fix uninitialized variable in handling medium errors

From: Mark Lord <[email protected]>

I am looking into how SCSI/SATA handle medium (disk) errors, and the observed behaviour is a little more random than expected, due to a bug in sd.c.

When scsi_get_sense_info_fld() fails (returns 0), it does NOT update the value of first_err_block. But sd_rw_intr() merrily continues to use that variable regardless, possibly making incorrect decisions about retries and the like.

This patch removes the randomness there, by using the first sector of the request (SCpnt->request->sector) in such cases, instead of first_err_block.

The patch shows more context than usual, to help see what's going on.

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

---

drivers/scsi/sd.c +2 -0

@@ -930,64 +930,66 @@
 			info_valid = scsi_get_sense_info_fld(
 				SCpnt->sense_buffer, SCSI_SENSE_BUFFERSIZE,
 				&first_err_block);
 			/*
 			 * May want to warn and skip if following cast results
 			 * in actual truncation (if sector_t < 64 bits)
 			 */
 			error_sector = (sector_t)first_err_block;
 			if (SCpnt->request->bio != NULL)
 				block_sectors = bio_sectors(SCpnt->request->bio);
 			switch (SCpnt->device->sector_size) {
 			case 1024:
 				error_sector <<= 1;
 				if (block_sectors < 2)
 					block_sectors = 2;
 				break;
 			case 2048:
 				error_sector <<= 2;
 				if (block_sectors < 4)
 					block_sectors = 4;
 				break;
 			case 4096:
 				error_sector <<=3;
 				if (block_sectors < 8)
 					block_sectors = 8;
 				break;
 			case 256:
 				error_sector >>= 1;
 				break;
 			default:
 				break;
 			}
+			if (!info_valid)
+				error_sector = SCpnt->request->sector;
 
 			error_sector &= ~(block_sectors - 1);
 			good_bytes = (error_sector - SCpnt->request->sector) << 9;
 			if (good_bytes < 0 || good_bytes >= this_count)
 				good_bytes = 0;
 			break;
 
 		case RECOVERED_ERROR: /* an error occurred, but it recovered */
 		case NO_SENSE: /* LLDD got sense data */
 			/*
 			 * Inform the user, but make sure that it's not treated
 			 * as a hard error.
 			 */
 			scsi_print_sense("sd", SCpnt);
 			SCpnt->result = 0;
 			memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 			good_bytes = this_count;
 			break;
 
 		case ILLEGAL_REQUEST:
 			if (SCpnt->device->use_10_for_rw &&
 			    (SCpnt->cmnd[0] == READ_10 ||
 			     SCpnt->cmnd[0] == WRITE_10))
 				SCpnt->device->use_10_for_rw = 0;
 			if (SCpnt->device->use_10_for_ms &&
 			    (SCpnt->cmnd[0] == MODE_SENSE_10 ||
 			     SCpnt->cmnd[0] == MODE_SELECT_10))
 				SCpnt->device->use_10_for_ms = 0;
 			break;
 
 		default:
 			break;

Mark Lord

author

2006-04-26 20:52 UTC #1 lore raw

Quoted 7 lines · Mark Lord

When scsi_get_sense_info_fld() fails (returns 0), it does NOT update the value of first_err_block. But sd_rw_intr() merrily continues to use that variable regardless, possibly making incorrect decisions about retries and the like.
This patch removes the randomness there, by using the first sector of the request (SCpnt->request->sector) in such cases, instead of first_err_block.

Note that this bug has been around for a while, and is also present in 2.6.16.xx. This same patch applies there too.

Cheers

James Bottomley

2006-04-26 22:56 UTC #2 lore raw

In reply to Mark Lord

Quoted 6 lines · Mark Lord

From: Mark Lord <[email protected]>
I am looking into how SCSI/SATA handle medium (disk) errors, and the observed behaviour is a little more random than expected, due to a bug in sd.c.

Quoted 8 lines

When scsi_get_sense_info_fld() fails (returns 0), it does NOT update the value of first_err_block. But sd_rw_intr() merrily continues to use that variable regardless, possibly making incorrect decisions about retries and the like.
This patch removes the randomness there, by using the first sector of the request (SCpnt->request->sector) in such cases, instead of first_err_block.
The patch shows more context than usual, to help see what's going on.

Thanks for finding the bug. Your solution is a bit, um, convoluted. What it should really be doing if we find no valid information field is a break so we go out with the default good_sectors of zero (rather than arriving at that value via a circuitous route).

And, of course, I couldn't resist eliminating the superfluous info_valid variable and tidying the logic to be programmatic instead of a switch case. How does this work?

James

drivers/scsi/sd.c +31 ~~-32~~

index c647d85..fff423e 100644
@@ -894,12 +894,10 @@ static void sd_rw_intr(struct scsi_cmnd 
 	int this_count = SCpnt->bufflen;
 	int good_bytes = (result == 0 ? this_count : 0);
 	sector_t block_sectors = 1;
-	u64 first_err_block;
 	sector_t error_sector;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;
-	int info_valid;
 
 	if (result) {
 		sense_valid = scsi_command_normalize_sense(SCpnt, &sshdr);

@@ -923,43 +921,44 @@ #endif
 	 */
 	if (driver_byte(result) != 0 &&
 		 sense_valid && !sense_deferred) {
+		u64 first_err_block;
+		int sector_size;
+
 		switch (sshdr.sense_key) {
 		case MEDIUM_ERROR:
 			if (!blk_fs_request(SCpnt->request))
 				break;
-			info_valid = scsi_get_sense_info_fld(
-				SCpnt->sense_buffer, SCSI_SENSE_BUFFERSIZE,
-				&first_err_block);
-			/*
-			 * May want to warn and skip if following cast results
-			 * in actual truncation (if sector_t < 64 bits)
-			 */
-			error_sector = (sector_t)first_err_block;
-			if (SCpnt->request->bio != NULL)
-				block_sectors = bio_sectors(SCpnt->request->bio);
-			switch (SCpnt->device->sector_size) {
-			case 1024:
-				error_sector <<= 1;
-				if (block_sectors < 2)
-					block_sectors = 2;
-				break;
-			case 2048:
-				error_sector <<= 2;
-				if (block_sectors < 4)
-					block_sectors = 4;
-				break;
-			case 4096:
-				error_sector <<=3;
-				if (block_sectors < 8)
-					block_sectors = 8;
-				break;
-			case 256:
-				error_sector >>= 1;
-				break;
-			default:
+			if (!scsi_get_sense_info_fld(SCpnt->sense_buffer,
+						    SCSI_SENSE_BUFFERSIZE,
+						    &first_err_block))
 				break;
+
+			sector_size = SCpnt->device->sector_size / 512;
+
+			if (SCpnt->request->bio != NULL)
+				block_sectors = 
+					bio_sectors(SCpnt->request->bio);
+			if (block_sectors < sector_size)
+				block_sectors = sector_size;
+
+			error_sector = (sector_t)first_err_block;
+			/* 
+			 * convert from physical sector size to
+			 * standard 512 byte sized sectors
+			 */
+			if (sector_size)
+				error_sector *= sector_size;
+			else {
+				int sector_size_div =
+					512 / SCpnt->device->sector_size;
+				error_sector /= sector_size_div;
 			}
 
+			/*
+			 * if the device has a larger logical than
+			 * physical sector size, round down to the
+			 * beginning of a logical sector
+			 */
 			error_sector &= ~(block_sectors - 1);
 			good_bytes = (error_sector - SCpnt->request->sector) << 9;
 			if (good_bytes < 0 || good_bytes >= this_count)

Mark Lord

author

2006-04-26 23:05 UTC #3 lore raw

Quoted 2 lines · James Bottomley

Quoted 1 line · Mark Lord
From: Mark Lord <[email protected]>

Quoted 6 lines

Quoted 6 lines
When scsi_get_sense_info_fld() fails (returns 0), it does NOT update the value of first_err_block. But sd_rw_intr() merrily continues to use that variable regardless, possibly making incorrect decisions about retries and the like.
This patch removes the randomness there, by using the first sector of the request (SCpnt->request->sector) in such cases, instead of first_err_block.

Quoted 1 line

Thanks for finding the bug. Your solution is a bit, um, convoluted.

Heh, no, that's the original sd.c. I just tried to keep the patch as tiny as possible to show what is needed. You are welcome to add complexity to it, though. The surrounding code is, uhm, rather interesting.

Quoted 3 lines

What it should really be doing if we find no valid information field is a break so we go out with the default good_sectors of zero (rather than arriving at that value via a circuitous route).

Yes, but the difficulty there is that all of the convoluted logic seems to still be wanted to set a correct "block_sectors" value, needed as a parameter on the final call:

   scsi_io_completion(SCpnt, good_bytes, block_sectors << 9);

Quoted 1 line

How does this work?

...

Looks potentially buggy (still).

Cheers

James Bottomley

2006-04-26 23:18 UTC #4 lore raw

Quoted 5 lines · Mark Lord

Yes, but the difficulty there is that all of the convoluted logic seems to still be wanted to set a correct "block_sectors" value, needed as a parameter on the final call:
    scsi_io_completion(SCpnt, good_bytes, block_sectors << 9);

Erm, but that's only used for volume overflow (or other single sector errors), which this isn't ... Actually, as far as I can tell, the whole block_sectors calculation can be killed as well.

Quoted 4 lines

Quoted 1 line
How does this work?
...
Looks potentially buggy (still).

Where?

James

Mark Lord

author

2006-04-26 23:29 UTC #5 lore raw

Quoted 10 lines · James Bottomley

Quoted 5 lines · Mark Lord
Yes, but the difficulty there is that all of the convoluted logic seems to still be wanted to set a correct "block_sectors" value, needed as a parameter on the final call:
    scsi_io_completion(SCpnt, good_bytes, block_sectors << 9);
Erm, but that's only used for volume overflow (or other single sector errors), which this isn't ... Actually, as far as I can tell, the whole block_sectors calculation can be killed as well.

I wonder if it can be done away with as a parameter for scsi_io_completion() ? If not, then we'll need some nice big comments in that function to warn against relying on a valid value for that parameter in specific cases.

Cheers

Andrew Morton

2006-04-26 23:12 UTC #6 lore raw

In reply to James Bottomley

Quoted 17 lines · James Bottomley

Quoted 6 lines · Mark Lord
From: Mark Lord <[email protected]>
I am looking into how SCSI/SATA handle medium (disk) errors, and the observed behaviour is a little more random than expected, due to a bug in sd.c.
Quoted 8 lines
When scsi_get_sense_info_fld() fails (returns 0), it does NOT update the value of first_err_block. But sd_rw_intr() merrily continues to use that variable regardless, possibly making incorrect decisions about retries and the like.
This patch removes the randomness there, by using the first sector of the request (SCpnt->request->sector) in such cases, instead of first_err_block.
The patch shows more context than usual, to help see what's going on.

Quoted 8 lines

Thanks for finding the bug. Your solution is a bit, um, convoluted. What it should really be doing if we find no valid information field is a break so we go out with the default good_sectors of zero (rather than arriving at that value via a circuitous route).
And, of course, I couldn't resist eliminating the superfluous info_valid variable and tidying the logic to be programmatic instead of a switch case. How does this work?

It'd be nice to have something simple-and-obvious for the simple-and-obvious -stable maintainers. That's if we think -stable needs this fixed.

drivers/scsi/sd.c #endif · line 954

Quoted 3 lines

drivers/scsi/sd.c +3 -0

+				int sector_size_div =
+					512 / SCpnt->device->sector_size;
+				error_sector /= sector_size_div;

You sure about this bit?

Mark Lord

author

2006-04-26 23:20 UTC #7 lore raw

Quoted 3 lines · Andrew Morton

It'd be nice to have something simple-and-obvious for the simple-and-obvious -stable maintainers.

That's why I kept the original patch very simple and focused, rather than trying to fix all of the convoluted code around it.

It's nice and simple, and *looks* correct.

A longer term cleanup of that function is better left to James!

Quoted 1 line

That's if we think -stable needs this fixed.

Let's say a bunch of read bio's get coalesced into a single 200+ sector request. This then fails on one single bad sector out of the 200+. Without the patch, there is a very good chance that sd.c will simply fail the entire request, all 200+ sectors.

With the patch, it will fail the first block, and then retry the remaining blocks. And repeat this until something works, or until everything has failed one by one.

Better, but still not the best.

What I need to have happen when a request is failed due to bad-media, is have it split the request into a sequence of single-block requests that are passed to the LLD one at a time. The ones with real bad sectors will then be independently failed, and the rest will get done.

Much better. Much more complex.

I'm thinking about something like that, just not sure whether to put it (initially) in libata, sd.c, or the block layer.

Cheers

Andrew Morton

2006-04-26 23:33 UTC #8 lore raw

Quoted 11 lines · Mark Lord

Quoted 2 lines
That's if we think -stable needs this fixed.
Let's say a bunch of read bio's get coalesced into a single 200+ sector request. This then fails on one single bad sector out of the 200+. Without the patch, there is a very good chance that sd.c will simply fail the entire request, all 200+ sectors.
With the patch, it will fail the first block, and then retry the remaining blocks. And repeat this until something works, or until everything has failed one by one.

Yowch. I have the feeling that this'll take our EIO-handling time from far-too-long to far-too-long*200.

I am still traumatised by my recent ten-minute wait for a dodgy DVD to become ejectable.

I don't think -stable needs this, personally.

Quoted 11 lines

Better, but still not the best.
What I need to have happen when a request is failed due to bad-media, is have it split the request into a sequence of single-block requests that are passed to the LLD one at a time. The ones with real bad sectors will then be independently failed, and the rest will get done.
Much better. Much more complex.
I'm thinking about something like that, just not sure whether to put it (initially) in libata, sd.c, or the block layer.

block, I suspect. My DVD trauma was IDE-induced. Jens is mulling the problem - I'd suggest you coordinate with him.

It would be a good thing to fix.

It's moderately hard to test, though. Easy enough for DVDs and CDs, but it's harder to take a marker pen to a hard drive.

Mark Lord

author

2006-04-27 01:43 UTC #9 lore raw

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Mark Lord
Quoted 1 line
That's if we think -stable needs this fixed.
Let's say a bunch of read bio's get coalesced into a single 200+ sector request. This then fails on one single bad sector out of the 200+. Without the patch, there is a very good chance that sd.c will simply fail the entire request, all 200+ sectors.

Quoted 6 lines

Quoted 3 lines
With the patch, it will fail the first block, and then retry the remaining blocks. And repeat this until something works, or until everything has failed one by one.
Yowch. I have the feeling that this'll take our EIO-handling time from far-too-long to far-too-long*200.

That's how it always used to work (eg. SUSE9 2.6.5+ kernels; Jens?).

Quoted 5 lines

Quoted 5 lines
What I need to have happen when a request is failed due to bad-media, is have it split the request into a sequence of single-block requests that are passed to the LLD one at a time. The ones with real bad sectors will then be independently failed, and the rest will get done.

Quoted 7 lines

Quoted 4 lines
Much better. Much more complex.
I'm thinking about something like that, just not sure whether to put it (initially) in libata, sd.c, or the block layer.
block, I suspect. My DVD trauma was IDE-induced. Jens is mulling the problem - I'd suggest you coordinate with him.

I've been pinging Jens about it for a couple of weeks now; no response.

Quoted 4 lines

It would be a good thing to fix.
It's moderately hard to test, though. Easy enough for DVDs and CDs, but it's harder to take a marker pen to a hard drive.

I have a bunch of "pre-marked" SATA drives here just for the purpose..

Cheers

Rogier Wolff

2006-04-27 09:28 UTC #10 lore raw

In reply to Andrew Morton

Quoted 9 lines · Andrew Morton

Quoted 8 lines · Mark Lord
Quoted 2 lines
That's if we think -stable needs this fixed.
Let's say a bunch of read bio's get coalesced into a single 200+ sector request. This then fails on one single bad sector out of the 200+. Without the patch, there is a very good chance that sd.c will simply fail the entire request, all 200+ sectors.

Quoted 6 lines

Quoted 3 lines
With the patch, it will fail the first block, and then retry the remaining blocks. And repeat this until something works, or until everything has failed one by one.
Yowch. I have the feeling that this'll take our EIO-handling time from far-too-long to far-too-long*200.

Whoa! Can't get worse.

Hmm. OK. Maybe it can. It seems that IDE at least used to do the "*200" behaviour.

I now issue a read for (1k-)block 1024 on my disk, which has blocks 1024 ... 1536 being bad. Kernel has decided i'm doing linear reads, so it will issue a readahead for 1024-1536.

I then don't want to have the kernel continue the 1025-1536 readahead.

Also, if just block 1027 is bad, a normal read of block 1024 will read the page 1024-1027, and return EIO on 1024 because it couldn't read the whole page. We've learned to live with this, but if you guys are going to look into it, keep this in mind, please....

Quoted 2 lines

It's moderately hard to test, though. Easy enough for DVDs and CDs, but it's harder to take a marker pen to a hard drive.

I (accidentally) scratched a SCSI drive with a paperclip some weeks ago. 65 bad blocks. (36 files hurt). I did manage to use the paperclip to push the metal part that had fallen off the inside of the drive somewhere to a spot where I could remove it. That metal part was preventing the head from leaving the parking zone.

Anyway, I have enough disks with bad blocks that I can make the following offer: Serious kernel developpers that need a drive-with-bad-blocks for testing should submit their snail-mail address. I'll see what I can do. (Specify SCSI or IDE or both).

Roger Wolff
[email protected]

Signature · 5 lines

** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ

Avi Kivity

2006-04-27 09:37 UTC #11 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

block, I suspect. My DVD trauma was IDE-induced. Jens is mulling the problem - I'd suggest you coordinate with him.
It would be a good thing to fix.
It's moderately hard to test, though. Easy enough for DVDs and CDs, but it's harder to take a marker pen to a hard drive.

If it's in the block layer, perhaps using a device mapper error target can help in testing. One could write a table to map an entire partition to a logical volume, then replace it with a table that maps a few sectors to the error target.

Signature · 1 line

error compiling committee.c: too many arguments to function

Mark Lord

2006-04-27 16:03 UTC #12 lore raw

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 9 lines · Mark Lord
Quoted 1 line
That's if we think -stable needs this fixed.
Let's say a bunch of read bio's get coalesced into a single 200+ sector request. This then fails on one single bad sector out of the 200+. Without the patch, there is a very good chance that sd.c will simply fail the entire request, all 200+ sectors.
With the patch, it will fail the first block, and then retry the remaining blocks. And repeat this until something works, or until everything has failed one by one.

Quoted 7 lines

Yowch. I have the feeling that this'll take our EIO-handling time from far-too-long to far-too-long*200.
I am still traumatised by my recent ten-minute wait for a dodgy DVD to become ejectable.
I don't think -stable needs this, personally.

Perhaps, perhaps not. The current behaviour is semi *random*, though. Sometimes it may just fail the entire request (wrong!), sometimes it may do the (almost as wrong) fail a block at a time from the beginning, until the bad sector is passed, and then succeed on the remainder.

Ugh.

Quoted 6 lines

Quoted 6 lines
What I need to have happen when a request is failed due to bad-media, is have it split the request into a sequence of single-block requests that are passed to the LLD one at a time. The ones with real bad sectors will then be independently failed, and the rest will get done.
Much better. Much more complex.

Signature · 3 lines

Mark Lord
Real-Time Remedies Inc.
[email protected]

James Bottomley

2006-04-26 23:22 UTC #13 lore raw

In reply to Andrew Morton

Quoted 3 lines · Andrew Morton

It'd be nice to have something simple-and-obvious for the simple-and-obvious -stable maintainers. That's if we think -stable needs this fixed.

Well .. the original will do for that.

Quoted 5 lines

drivers/scsi/sd.c #endif · line 954
Quoted 3 lines
drivers/scsi/sd.c +3 -0
+				int sector_size_div =
+					512 / SCpnt->device->sector_size;
+				error_sector /= sector_size_div;
You sure about this bit?

Yes. If the <hardware sector size> is < 512 bytes then to convert the listed error sector to the standard 512 byte sector size block index, we have to divide (512/<hardware sector size>).

James