Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM
On Tue, 2007-05-08 at 17:03 +0100, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. OK, the alternative fix for this is in the vanilla tree. Could you make sure it works ... if it does, I think it's a candidate for 2.6.21 stable as well. James - 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
[PATCH]: Fix old SCSI adapter crashes with CD-ROM
The CD-ROM layer doesn't bounce requests for old ISA controllers (and
nor should it). However they get injected into the SCSI layer via
sr_ioctl which also doesn't bounce them and SCSI then passes the buffer
along to a device with unchecked_isa_dma set which either panics or
truncates the buffer to 24bits.
According to Jens the right long term fix is for the CD layer to route
the requests differently but in the mean time this has been tested by a
victim and verified to sort the problem out. For the other 99.9% of users
it's a no-op and doesn't bounce data.
Signed-off-by: Alan Cox [EMAIL PROTECTED]
diff -u --new-file --recursive --exclude-from /usr/src/exclude
linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
--- linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-04-12
14:14:45.0 +0100
+++ linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-05-01
14:23:40.0 +0100
@@ -187,9 +187,10 @@
struct scsi_sense_hdr sshdr;
int result, err = 0, retries = 0;
struct request_sense *sense = cgc-sense;
-
+ void *zebedee = cgc-buffer;
+
SDev = cd-device;
-
+
if (!sense) {
sense = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
if (!sense) {
@@ -197,7 +198,22 @@
goto out;
}
}
-
+
+ if (cgc-buflen cd-device-host-unchecked_isa_dma) {
+ switch(cgc-data_direction) {
+ case DMA_NONE:
+ break;
+ case DMA_FROM_DEVICE:
+ case DMA_TO_DEVICE:
+ zebedee = kmalloc(cgc-buflen,
GFP_KERNEL|GFP_DMA);
+ if (zebedee ==NULL) {
+ err = -ENOMEM;
+ goto out;
+ }
+ }
+ if (cgc-data_direction == DMA_TO_DEVICE)
+ memcpy(zebedee, cgc-buffer, cgc-buflen);
+ }
retry:
if (!scsi_block_when_processing_errors(SDev)) {
err = -ENODEV;
@@ -206,10 +222,16 @@
memset(sense, 0, sizeof(*sense));
result = scsi_execute(SDev, cgc-cmd, cgc-data_direction,
- cgc-buffer, cgc-buflen, (char *)sense,
+ zebedee, cgc-buflen, (char *)sense,
cgc-timeout, IOCTL_RETRIES, 0);
scsi_normalize_sense((char *)sense, sizeof(*sense), sshdr);
+
+ if (zebedee != cgc-buffer) {
+ if (cgc-data_direction == DMA_FROM_DEVICE)
+ memcpy(cgc-buffer, zebedee, cgc-buflen);
+ kfree(zebedee); /* Time for bed */
+ }
/* Minimal error checking. Ignore cases we know about, and report the
rest. */
if (driver_byte(result) != 0) {
-
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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM
On Tue, 8 May 2007 17:03:35 +0100 Alan Cox [EMAIL PROTECTED] wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. Umm duh there's a memory leak in an error case there still. Please discard and I'll post a new one shortly - 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
[PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
The CD-ROM layer doesn't bounce requests for old ISA controllers (and
nor should it). However they get injected into the SCSI layer via
sr_ioctl which also doesn't bounce them and SCSI then passes the buffer
along to a device with unchecked_isa_dma set which either panics or
truncates the buffer to 24bits.
According to Jens the right long term fix is for the CD layer to route
the requests differently but in the mean time this has been tested by a
victim and verified to sort the problem out. For the other 99.9% of users
it's a no-op and doesn't bounce data.
Signed-off-by: Alan Cox [EMAIL PROTECTED]
diff -u --new-file --recursive --exclude-from /usr/src/exclude
linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c
--- linux.vanilla-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-04-12
14:14:45.0 +0100
+++ linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c2007-05-08
16:55:01.446661608 +0100
@@ -187,9 +187,10 @@
struct scsi_sense_hdr sshdr;
int result, err = 0, retries = 0;
struct request_sense *sense = cgc-sense;
-
+ void *zebedee = cgc-buffer;
+
SDev = cd-device;
-
+
if (!sense) {
sense = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
if (!sense) {
@@ -197,7 +198,22 @@
goto out;
}
}
-
+
+ if (cgc-buflen cd-device-host-unchecked_isa_dma) {
+ switch(cgc-data_direction) {
+ case DMA_NONE:
+ break;
+ case DMA_FROM_DEVICE:
+ case DMA_TO_DEVICE:
+ zebedee = kmalloc(cgc-buflen,
GFP_KERNEL|GFP_DMA);
+ if (zebedee ==NULL) {
+ err = -ENOMEM;
+ goto out;
+ }
+ }
+ if (cgc-data_direction == DMA_TO_DEVICE)
+ memcpy(zebedee, cgc-buffer, cgc-buflen);
+ }
retry:
if (!scsi_block_when_processing_errors(SDev)) {
err = -ENODEV;
@@ -206,10 +222,15 @@
memset(sense, 0, sizeof(*sense));
result = scsi_execute(SDev, cgc-cmd, cgc-data_direction,
- cgc-buffer, cgc-buflen, (char *)sense,
+ zebedee, cgc-buflen, (char *)sense,
cgc-timeout, IOCTL_RETRIES, 0);
scsi_normalize_sense((char *)sense, sizeof(*sense), sshdr);
+
+ if (zebedee != cgc-buffer) {
+ if (cgc-data_direction == DMA_FROM_DEVICE)
+ memcpy(cgc-buffer, zebedee, cgc-buflen);
+ }
/* Minimal error checking. Ignore cases we know about, and report the
rest. */
if (driver_byte(result) != 0) {
@@ -266,6 +287,8 @@
/* Wake up a process waiting for device */
out:
+ if (zebedee != cgc-buffer)
+ kfree(zebedee); /* Time for bed */
if (!cgc-sense)
kfree(sense);
cgc-stat = err;
-
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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. -- Jens Axboe - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? James - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. -- Jens Axboe - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Hey Jens and James, one thing I forgot to mention is that I could not remember if I needed an extra bio_get in there. I thought I did not because the caller is not touching the bio after the bio_endio calls like is done with the blk/bio_map_user path. But I did that patch so long ago I do not remember now. - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:40 +0200, Jens Axboe wrote: On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. It won't? I thought the issue (from the fix) is that cgc-buffer is outside of the device accessibility mask. The scsi_execute path allocates a request and then calls blk_rq_map_kern on the buffer (in this case cgc-buffer) ... the problem is that blk_rq_map_kern() doesn't currently bounce the buffer, but if it did (which is the functionality Mike's patch adds), surely the need to bounce it in the ioctl path would go away? James - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, Mike Christie wrote: James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Hey Jens and James, one thing I forgot to mention is that I could not remember if I needed an extra bio_get in there. I thought I did not because the caller is not touching the bio after the bio_endio calls like is done with the blk/bio_map_user path. But I did that patch so long ago I do not remember now. If you don't touch it after bio_endio(), then you don't need to hold an extra reference to it. I'll add your patch. -- Jens Axboe - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:40 +0200, Jens Axboe wrote: On Tue, May 08 2007, James Bottomley wrote: On Tue, 2007-05-08 at 18:14 +0200, Jens Axboe wrote: On Tue, May 08 2007, Alan Cox wrote: The CD-ROM layer doesn't bounce requests for old ISA controllers (and nor should it). However they get injected into the SCSI layer via sr_ioctl which also doesn't bounce them and SCSI then passes the buffer along to a device with unchecked_isa_dma set which either panics or truncates the buffer to 24bits. According to Jens the right long term fix is for the CD layer to route the requests differently but in the mean time this has been tested by a victim and verified to sort the problem out. For the other 99.9% of users it's a no-op and doesn't bounce data. Signed-off-by: Alan Cox [EMAIL PROTECTED] Signed-off-by: Jens Axboe [EMAIL PROTECTED] Christoph passed me his patch to get rid of -generic_packet() in the cdrom layer, so the work is almost complete. This patch is fine as a work-around until that gets merged, though. Actually, I think the new scsi request infrastructure should be doing the bouncing (rather than have it done in each problem path we discover). Of course, bouncing should only be done in one layer (the block layer). Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? That's another issue. The problem here are requests (cgc's) initiated by the cdrom.c layer. Those _should_ get mapped to a request and put on the queue for the device, and thus get bounced by the block layer if appropriate. Mike's fix looks legit and should be merged as well, but it wont fix this issue. It won't? I thought the issue (from the fix) is that cgc-buffer is outside of the device accessibility mask. The scsi_execute path allocates a request and then calls blk_rq_map_kern on the buffer (in this case cgc-buffer) ... the problem is that blk_rq_map_kern() doesn't currently bounce the buffer, but if it did (which is the functionality Mike's patch adds), surely the need to bounce it in the ioctl path would go away? You are right, I missed that scsi_execute() actually builds a request in the proper manner. Mikes patch is in the for-2.6.22 block branch and I asked Linus to pull, so all should be well. -- Jens Axboe - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? Short answer: No Long answer - it doesn't take this path. Different bug, both want fixing I suspect. - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:42 +0100, Alan Cox wrote: Mike Christie tells me we're missing bouncing by accident in the scsi_execute path (but not the scsi_execute_async path). He says this is the fix he proposed: http://marc.info/?l=linux-scsim=115981479822790w=2 Can we just merge this instead? Short answer: No Long answer - it doesn't take this path. Different bug, both want fixing I suspect. Actually, it does take this path ... one of the things we've been doing in SCSI is slowly eliminating the old direct submission paths in favour of sending everything through the correct block layer paths. scsi_execute(), which the sr ioctl uses is just such a fixed path ... the bug is that it should be bouncing the request but because of an oversight (which Mike's patch corrects) it doesn't. The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. James - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
Long answer - it doesn't take this path. Different bug, both want fixing I suspect. Actually, it does take this path ... one of the things we've been doing in SCSI is slowly eliminating the old direct submission paths in favour of sending everything through the correct block layer paths. scsi_execute(), which the sr ioctl uses is just such a fixed path ... the bug is that it should be bouncing the request but because of an oversight (which Mike's patch corrects) it doesn't. Well if Mike's patch is going in and it fixes this then I'll be more than happy to withdraw the pending one. The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. About time ;) Alan - 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
Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)
On Tue, 2007-05-08 at 18:53 +0100, Alan Cox wrote: The ultimate goal is to be able to eliminate the unchecked_isa_dma flag entirely and have the block layer (or device mask allocations) fix all of this in every ULD. About time ;) Actually, I should point out (before those who did the work get justifiably irritated) that the overall goal is to remove the special case non-scatter/gather path from all the drivers ... eliminating the need to worry about DMA zone allocations is just a nice bonus as a side effect of sending everything through the block layer. James - 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
