Possible bug in usb storage (2.6.11 kernel)

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

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: [email protected] 
Maintainer, Linux USB Mass Storage Driver

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

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

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

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

-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

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: [email protected] 
Maintainer, Linux USB Mass Storage Driver

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

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

diff -u linux-2.6.13/drivers/usb/storage/transport.c linux/drivers/usb/storage/transport.c
@@ -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

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

Yes I did. You can see it at

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

Alan Stern

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

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

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

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

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