Re: [PATCH] drivers/scsi/sd.c: fix uninitialized variable in handling medium errors

[James Bottomley <James.Bottomley@SteelEye.com>]

On Wed, 2006-04-26 at 16:27 -0400, Mark Lord wrote:
> From: Mark Lord <lkml@rtr.ca>
> 
> 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.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c647d85..fff423e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -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)