Possible bug in usb storage (2.6.11 kernel)

Possible bug in usb storage (2.6.11 kernel)

[Jim Ramsay]

I think I have found a possible bug:

In usb/storage/transport.c, the routine usb_stor_Bulk_transport (and
perhaps other releated routines as well) has the following in the
"DATA STAGE" (around line 1020 in my file):

result = usb_stor_bulk_transfer_sg(us, pipe,
                                        srb->request_buffer, transfer_length,
                                        srb->use_sg, &srb->resid);

This looks fine, except that there is no guarantee that
srb->request_buffer is page-aligned or cache-aligned, as is required
according to Documentation/DMA-API.txt

In fact, on my MIPS platform, I sometimes get odd hangs and panics
because the later call to "dma_map_single" on this buffer causes a
cache invalidation, and since this memory is not necessarily
cache-aligned, sometimes too much cache is invalidated, which causes
interesting (and annoying) memory corruptions.  Usually I've seen it
change the 'request' pointer in us->current_urb to something slightly
different, or zero, depending on the system's mood, but I'm sure there
are other possible ugly side-effects of this.

The solution, as suggested by Documentation/DMA-API.txt is to ensure
that all buffers passed into dma_map_single are always cache-aligned
(or at least page-aligned).

I would suggest this should be fixed by using a new buffer allocated
by the usb storage layer and then copying the result back into the
srb->request_buffer.

I suppose the scsi code could be changed to guarantee that
srb->request_buffer is page-aligned or cache-aligned, but that seems
like the wrong solution for this bug.

Questions?  Comments?  Is this the right way to fix it?

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> I think I have found a possible bug:
> [...]
> I suppose the scsi code could be changed to guarantee that
> srb->request_buffer is page-aligned or cache-aligned, but that seems
> like the wrong solution for this bug.

Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
supposed to guarantee us that those buffers are suitable for DMA'ing, and
apparently it's violating that promise.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Jim Ramsay]

On 9/8/05, Matthew Dharm <mdharm-kernel@one-eyed-alien.net> wrote:
> On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> > I think I have found a possible bug:
> > [...]
> > I suppose the scsi code could be changed to guarantee that
> > srb->request_buffer is page-aligned or cache-aligned, but that seems
> > like the wrong solution for this bug.
> 
> Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
> supposed to guarantee us that those buffers are suitable for DMA'ing, and
> apparently it's violating that promise.

Thanks, I'll check on what buffer I'm actually getting, where it's
allocated, and post back what I find, or how I fixed it.

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Jim Ramsay]

On 9/8/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> On 9/8/05, Matthew Dharm <mdharm-kernel@one-eyed-alien.net> wrote:
> > On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> > > I think I have found a possible bug:
> > > [...]
> > > I suppose the scsi code could be changed to guarantee that
> > > srb->request_buffer is page-aligned or cache-aligned, but that seems
> > > like the wrong solution for this bug.
> >
> > Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
> > supposed to guarantee us that those buffers are suitable for DMA'ing, and
> > apparently it's violating that promise.
> 
> Thanks, I'll check on what buffer I'm actually getting, where it's
> allocated, and post back what I find, or how I fixed it.

More information:

The error only occurrs during device sensing when the
srb->request_buffer is assigned as follows, by usb/storage/transport.c
in the routine usb_stor_invoke_transport:

old_request_buffer = srb->request_buffer;
srb->request_buffer = srb->sense_buffer;

Now, this is a problem because srb->sense_buffer is defined as follows
in the struct scsi_cmnd:

#define SCSI_SENSE_BUFFERSIZE   96
        unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];

Since it is not allocated at runtime there is NO WAY the SCSI layer
can possibly guarantee it is page- or cache-aligned and ready for DMA.

Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
 Or should USB step up in this case and ensure this buffer is dma-safe
itself?

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Thu, 8 Sep 2005, Jim Ramsay wrote:

> More information:
> 
> The error only occurrs during device sensing when the
> srb->request_buffer is assigned as follows, by usb/storage/transport.c
> in the routine usb_stor_invoke_transport:
> 
> old_request_buffer = srb->request_buffer;
> srb->request_buffer = srb->sense_buffer;
> 
> Now, this is a problem because srb->sense_buffer is defined as follows
> in the struct scsi_cmnd:
> 
> #define SCSI_SENSE_BUFFERSIZE   96
>         unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> 
> Since it is not allocated at runtime there is NO WAY the SCSI layer
> can possibly guarantee it is page- or cache-aligned and ready for DMA.
> 
> Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
>  Or should USB step up in this case and ensure this buffer is dma-safe
> itself?

Aha!

I've long thought that usb-storage should allocate its own transfer buffer 
for sense data.  In the past people have said, "No, don't bother, it's not 
really needed."  Here's a good reason for doing it.

Expect a patch before long.

Alan Stern

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On Thu, Sep 08, 2005 at 02:28:09PM -0600, Jim Ramsay wrote:
> On 9/8/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> > On 9/8/05, Matthew Dharm <mdharm-kernel@one-eyed-alien.net> wrote:
> > > On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> > > > I think I have found a possible bug:
> > > > [...]
> > > > I suppose the scsi code could be changed to guarantee that
> > > > srb->request_buffer is page-aligned or cache-aligned, but that seems
> > > > like the wrong solution for this bug.
> > >
> > > Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
> > > supposed to guarantee us that those buffers are suitable for DMA'ing, and
> > > apparently it's violating that promise.
> > 
> > Thanks, I'll check on what buffer I'm actually getting, where it's
> > allocated, and post back what I find, or how I fixed it.
> 
> More information:
> 
> The error only occurrs during device sensing when the
> srb->request_buffer is assigned as follows, by usb/storage/transport.c
> in the routine usb_stor_invoke_transport:
> 
> old_request_buffer = srb->request_buffer;
> srb->request_buffer = srb->sense_buffer;
> 
> Now, this is a problem because srb->sense_buffer is defined as follows
> in the struct scsi_cmnd:
> 
> #define SCSI_SENSE_BUFFERSIZE   96
>         unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> 
> Since it is not allocated at runtime there is NO WAY the SCSI layer
> can possibly guarantee it is page- or cache-aligned and ready for DMA.
> 
> Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
>  Or should USB step up in this case and ensure this buffer is dma-safe
> itself?

Ah, that buffer doesn't come from SCSI (tho I've long thought they should
provide us with a sense data buffer).  So this is a real usb-storage bug.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Thu, 8 Sep 2005 16:40:16 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> I've long thought that usb-storage should allocate its own
stern> transfer buffer for sense data.  In the past people have said,
stern> "No, don't bother, it's not really needed."  Here's a good
stern> reason for doing it.

stern> Expect a patch before long.

Did you already create the patch?  If not, how about this (against 2.6.13) ?


Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff -u linux-2.6.13/drivers/usb/storage/transport.c linux/drivers/usb/storage/transport.c
--- linux-2.6.13/drivers/usb/storage/transport.c	2005-08-29 08:41:01.000000000 +0900
+++ linux/drivers/usb/storage/transport.c	2005-09-27 18:03:32.000000000 +0900
@@ -638,7 +638,8 @@
 
 		/* use the new buffer we have */
 		old_request_buffer = srb->request_buffer;
-		srb->request_buffer = srb->sense_buffer;
+		srb->request_buffer =
+			kmalloc(max(dma_get_cache_alignment(), 18), GFP_NOIO);
 
 		/* set the buffer length for transfer */
 		old_request_bufflen = srb->request_bufflen;
@@ -655,7 +656,13 @@
 		/* issue the auto-sense command */
 		old_resid = srb->resid;
 		srb->resid = 0;
-		temp_result = us->transport(us->srb, us);
+		if (srb->request_buffer) {
+			temp_result = us->transport(us->srb, us);
+			memcpy(srb->sense_buffer, srb->request_buffer, 18);
+			kfree(srb->request_buffer);
+		} else {
+			temp_result = USB_STOR_TRANSPORT_FAILED;
+		}
 
 		/* let's clean up right away */
 		srb->resid = old_resid;


---
Atsushi Nemoto

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Tue, 27 Sep 2005, Atsushi Nemoto wrote:

> >>>>> On Thu, 8 Sep 2005 16:40:16 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> I've long thought that usb-storage should allocate its own
> stern> transfer buffer for sense data.  In the past people have said,
> stern> "No, don't bother, it's not really needed."  Here's a good
> stern> reason for doing it.
> 
> stern> Expect a patch before long.
> 
> Did you already create the patch?  If not, how about this (against 2.6.13) ?

Yes I did.  You can see it at

https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html

Alan Stern

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Tue, 27 Sep 2005 10:21:17 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> Yes I did.  You can see it at
stern> https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html

Thank you.  But 'kmalloc(US_SENSE_SIZE, GFP_KERNEL)' is not enough (at
least) for MIPS since some MIPS chips have 32 byte cacheline and
ARCH_KMALLOC_MINALIGN is 8 on linux-mips.

Using 'max(dma_get_cache_alignment(), US_SENSE_SIZE)' would be OK.

---
Atsushi Nemoto

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Tue, 27 Sep 2005, Atsushi Nemoto wrote:

> >>>>> On Tue, 27 Sep 2005 10:21:17 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> Yes I did.  You can see it at
> stern> https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html
> 
> Thank you.  But 'kmalloc(US_SENSE_SIZE, GFP_KERNEL)' is not enough (at
> least) for MIPS since some MIPS chips have 32 byte cacheline and
> ARCH_KMALLOC_MINALIGN is 8 on linux-mips.
> 
> Using 'max(dma_get_cache_alignment(), US_SENSE_SIZE)' would be OK.

If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN is 
supposed to be at least as large as a cacheline.  See this comment in 
mm/slab.c:

/*
 * Enforce a minimum alignment for the kmalloc caches.
 * Usually, the kmalloc caches are cache_line_size() aligned, except when
 * DEBUG and FORCED_DEBUG are enabled, then they are BYTES_PER_WORD aligned.
 * Some archs want to perform DMA into kmalloc caches and need a guaranteed
 * alignment larger than BYTES_PER_WORD. ARCH_KMALLOC_MINALIGN allows that.
 * Note that this flag disables some debug features.
 */

and also this comment (referring to the kmalloc caches):

		/*
		 * For performance, all the general caches are L1 aligned.
		 * This should be particularly beneficial on SMP boxes, as it
		 * eliminates "false sharing".
		 * Note for systems short on memory removing the alignment will
		 * allow tighter packing of the smaller caches.
		 */

Alan Stern

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Tue, 27 Sep 2005 11:38:35 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN
stern> is supposed to be at least as large as a cacheline.  See this
stern> comment in mm/slab.c:

Thank you for pointing out this.

Some time ago I also supposed so, but I was told to use
dma_get_cache_alignment() instead.  The comment was not exist at that
time...

OK, I will talk to MIPS maintainar again.
---
Atsushi Nemoto

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Ralf Baechle]

On Thu, Sep 29, 2005 at 01:27:05AM +0900, Atsushi Nemoto wrote:

> >>>>> On Tue, 27 Sep 2005 11:38:35 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN
> stern> is supposed to be at least as large as a cacheline.  See this
> stern> comment in mm/slab.c:
> 
> Thank you for pointing out this.
> 
> Some time ago I also supposed so, but I was told to use
> dma_get_cache_alignment() instead.  The comment was not exist at that
> time...

ARCH_KMALLOC_MINALIGN is set to 8 on MIPS because we used to have 64-bit
members in struct semaphore but on 32-bit kernels kmalloc would return 4-byte
allocated memory only.  Whoops, an ooops ;)  Obviously that's been a thinko
as it was done without consideration for DMA.

Coherent I/O isn't affected, also there are a few R3000-class processors where
8 bytes happens to be just the right value.  For anything else this is a bug
which we probably get away most of the time and the value should would be
the largest size of any cacheline in the cache hierarchy, so upto 128 for some
systems.

  Ralf