[PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Alan Cox]

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 <alan@redhat.com>

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.c	2007-04-12 14:14:45.000000000 +0100
+++ linux-2.6.21-rc6-mm1/drivers/scsi/sr_ioctl.c	2007-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;

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Jens Axboe]

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 <alan@redhat.com>

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[James Bottomley]

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 <alan@redhat.com>
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> 
> 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-scsi&m=115981479822790&w=2

Can we just merge this instead?

James

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Jens Axboe]

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 <alan@redhat.com>
> > 
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > 
> > 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-scsi&m=115981479822790&w=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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[James Bottomley]

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 <alan@redhat.com>
> > > 
> > > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > > 
> > > 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-scsi&m=115981479822790&w=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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Jens Axboe]

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 <alan@redhat.com>
> > > > 
> > > > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > > > 
> > > > 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-scsi&m=115981479822790&w=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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Mike Christie]

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 <alan@redhat.com>
>> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
>>
>> 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-scsi&m=115981479822790&w=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.

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Jens Axboe]

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 <alan@redhat.com>
> >> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> >>
> >> 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-scsi&m=115981479822790&w=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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Alan Cox]

> 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-scsi&m=115981479822790&w=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.

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[James Bottomley]

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-scsi&m=115981479822790&w=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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[Alan Cox]

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

Re: [PATCH]: Fix old SCSI adapter crashes with CD-ROM (take 2)

[James Bottomley]

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