Skip to contentSkip to conversation

SCSI bug fixes for 2.6.36-rc7

GIT PULL James Bottomley 4 participants 6 messages Oct 13, 2010 – Oct 14, 2010

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

This patch set fixes a couple of longstanding bugs in SCSI. The VPD patch fix a possible buffer overrun in the VPD code (only tripped if the device has a huge number of VPD pages). The other is where the status return code on our BSG interface is wrongly right shifted one place.

The patch is available here:

master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-rc-fixes-2.6.git

The short changelog is:

FUJITA Tomonori (1):
bsg: fix incorrect device_status value

Martin K. Petersen (1):
Fix VPD inquiry page wrapper

The diffstat is:

 block/bsg.c         |    2 +-
 drivers/scsi/scsi.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

And the full diff is below.

James

---

block/bsg.c +1 -1
index 82d5882..5974682 100644
@@ -426,7 +426,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	/*
 	 * fill in all the output members
 	 */
-	hdr->device_status = status_byte(rq->errors);
+	hdr->device_status = rq->errors && 0xff;
 	hdr->transport_status = host_byte(rq->errors);
 	hdr->driver_status = driver_byte(rq->errors);
 	hdr->info = 0;
drivers/scsi/scsi.c +2 -2
index ad0ed21..348fba0 100644
@@ -1046,13 +1046,13 @@ int scsi_get_vpd_page(struct scsi_device *sdev, u8 page, unsigned char *buf,
 
 	/* If the user actually wanted this page, we can skip the rest */
 	if (page == 0)
-		return -EINVAL;
+		return 0;
 
 	for (i = 0; i < min((int)buf[3], buf_len - 4); i++)
 		if (buf[i + 4] == page)
 			goto found;
 
-	if (i < buf[3] && i > buf_len)
+	if (i < buf[3] && i >= buf_len - 4)
 		/* ran off the end of the buffer, give us benefit of doubt */
 		goto found;
 	/* The device claims it doesn't support the requested page */
#1 lore raw
Quoted 4 lines · James Bottomley

This patch set fixes a couple of longstanding bugs in SCSI.  The VPD patch fix a possible buffer overrun in the VPD code (only tripped if the device has a huge number of VPD pages).  The other is where the status return code on our BSG interface is wrongly right shifted one place.

Umm. That other patch seems to be utter crap.

Quoted 2 lines

-       hdr->device_status = status_byte(rq->errors);
+       hdr->device_status = rq->errors && 0xff;

That's pure shit. Please look at it for one second, and don't send me crap like this.

Linus "grumpy" Torvalds

#2 lore raw
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · James Bottomley

This patch set fixes a couple of longstanding bugs in SCSI. The VPD patch fix a possible buffer overrun in the VPD code (only tripped if the device has a huge number of VPD pages). The other is where the status return code on our BSG interface is wrongly right shifted one place.

Quoted 7 lines

Umm. That other patch seems to be utter crap.

block/bsg.c static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr, · line 429

Quoted 2 lines
block/bsg.c +1 -1
-       hdr->device_status = status_byte(rq->errors);
+       hdr->device_status = rq->errors && 0xff;

That's pure shit. Please look at it for one second, and don't send me crap like this.

What's wrong with it? device_status() is an ancient SCSI-1 macro that dates from the time the status was left shifted by 1, which makes the value wrong according to the sg interface spec. We should really remove it, but there's still a few ancient drivers using it. the SG interface requires the full status byte, which is what the bottom most byte of req_errors is ... hence the change.

James

#3 lore raw
Quoted 13 lines · James Bottomley
Quoted 11 lines · Linus Torvalds

On Wed, Oct 13, 2010 at 6:45 AM, James Bottomley
<[email protected]> wrote:

Umm. That other patch seems to be utter crap.

Quoted 2 lines

-       hdr->device_status = status_byte(rq->errors);
+       hdr->device_status = rq->errors && 0xff;

That's pure shit. Please look at it for one second, and don't send me crap like this.

Quoted 7 lines

What's wrong with it? device_status() is an ancient SCSI-1 macro that dates from the time the status was left shifted by 1, which makes the value wrong according to the sg interface spec.  We should really remove it, but there's still a few ancient drivers using it.  the SG interface requires the full status byte, which is what the bottom most byte of req_errors is ... hence the change.

How about & instead of &&

Harvey
--
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

#4 lore raw
In reply to James Bottomley
Quoted 16 lines · James Bottomley
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · James Bottomley

This patch set fixes a couple of longstanding bugs in SCSI. The VPD patch fix a possible buffer overrun in the VPD code (only tripped if the device has a huge number of VPD pages). The other is where the status return code on our BSG interface is wrongly right shifted one place.

Quoted 7 lines

Umm. That other patch seems to be utter crap.

block/bsg.c static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr, · line 429

Quoted 2 lines
block/bsg.c +1 -1
-       hdr->device_status = status_byte(rq->errors);
+       hdr->device_status = rq->errors && 0xff;

That's pure shit. Please look at it for one second, and don't send me crap like this.

Quoted 6 lines

What's wrong with it? device_status() is an ancient SCSI-1 macro that dates from the time the status was left shifted by 1, which makes the value wrong according to the sg interface spec. We should really remove it, but there's still a few ancient drivers using it. the SG interface requires the full status byte, which is what the bottom most byte of req_errors is ... hence the change.

OK, right ... several people pointed out in private email that it was && not & ... I'll fix it when I'm slightly less jet lagged.

James

maintainer
#5 lore raw
Quoted 24 lines · On Wed, 13 Oct 2010 20:34:37 -0400 James Bottomley <[email protected]>…
Quoted 16 lines · James Bottomley
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · James Bottomley

This patch set fixes a couple of longstanding bugs in SCSI. The VPD patch fix a possible buffer overrun in the VPD code (only tripped if the device has a huge number of VPD pages). The other is where the status return code on our BSG interface is wrongly right shifted one place.

Quoted 7 lines

Umm. That other patch seems to be utter crap.

Review

Quoted 2 lines
(quoted diff) +1 -1
-       hdr->device_status = status_byte(rq->errors);
+       hdr->device_status = rq->errors && 0xff;

That's pure shit. Please look at it for one second, and don't send me crap like this.

Quoted 6 lines

What's wrong with it? device_status() is an ancient SCSI-1 macro that dates from the time the status was left shifted by 1, which makes the value wrong according to the sg interface spec. We should really remove it, but there's still a few ancient drivers using it. the SG interface requires the full status byte, which is what the bottom most byte of req_errors is ... hence the change.

Quoted 2 lines

OK, right ... several people pointed out in private email that it was && not & ... I'll fix it when I'm slightly less jet lagged.

Really sorry about that.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] bsg: fix incorrect device_status value

bsg incorrectly returns sg's masked_status value for device_status.

---
 block/bsg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
block/bsg.c +1 -1
index 82d5882..0c00870 100644
@@ -426,7 +426,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 	/*
 	 * fill in all the output members
 	 */
-	hdr->device_status = status_byte(rq->errors);
+	hdr->device_status = rq->errors & 0xff;
 	hdr->transport_status = host_byte(rq->errors);
 	hdr->driver_status = driver_byte(rq->errors);
 	hdr->info = 0;
Signature · 1 line
1.7.1