* [RFC] usb: Check results of dma_map_single
@ 2009-11-04 5:48 Larry Finger
2009-11-04 12:50 ` Paulius Zaleckas
2009-11-04 19:49 ` Christian Lamparter
0 siblings, 2 replies; 7+ messages in thread
From: Larry Finger @ 2009-11-04 5:48 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-wireless, linux-usb
At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
with DMA buffer processing was corrected for the libertas driver. Because
routine usb_fill_bulk_urb() does not check that DMA is possible when
dma_map_single() is called, this condition was not detected until the buffer
was unmapped. By this time memory corruption had occurred.
The situation is fixed by testing the returned DMA address. If not a legal
address, a WARN_ON(1) is executed to provide traceback and the error is
returned.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
Index: linux-2.6/drivers/usb/core/hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/hcd.c
+++ linux-2.6/drivers/usb/core/hcd.c
@@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
urb->setup_packet,
sizeof(struct usb_ctrlrequest),
DMA_TO_DEVICE);
+ ret = dma_mapping_error(hcd->self.controller,
+ urb->setup_dma);
+ /* warn if DMA mapping failed */
+ if (ret) {
+ WARN_ON(1);
+ return ret;
+ }
else if (hcd->driver->flags & HCD_LOCAL_MEM)
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
@@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
urb->transfer_buffer,
urb->transfer_buffer_length,
dir);
+ ret = dma_mapping_error(hcd->self.controller,
+ urb->transfer_dma);
+ /* warn if DMA mapping failed */
+ if (ret) {
+ WARN_ON(1);
+ return ret;
+ }
else if (hcd->driver->flags & HCD_LOCAL_MEM) {
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 5:48 [RFC] usb: Check results of dma_map_single Larry Finger
@ 2009-11-04 12:50 ` Paulius Zaleckas
2009-11-04 14:06 ` Larry Finger
2009-11-04 19:49 ` Christian Lamparter
1 sibling, 1 reply; 7+ messages in thread
From: Paulius Zaleckas @ 2009-11-04 12:50 UTC (permalink / raw)
To: Larry Finger; +Cc: Greg Kroah-Hartman, linux-wireless, linux-usb
On 11/04/2009 07:48 AM, Larry Finger wrote:
> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
> with DMA buffer processing was corrected for the libertas driver. Because
> routine usb_fill_bulk_urb() does not check that DMA is possible when
> dma_map_single() is called, this condition was not detected until the buffer
> was unmapped. By this time memory corruption had occurred.
>
> The situation is fixed by testing the returned DMA address. If not a legal
> address, a WARN_ON(1) is executed to provide traceback and the error is
> returned.
>
> Signed-off-by: Larry Finger<Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>
> Index: linux-2.6/drivers/usb/core/hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/core/hcd.c
> +++ linux-2.6/drivers/usb/core/hcd.c
> @@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
> urb->setup_packet,
> sizeof(struct usb_ctrlrequest),
> DMA_TO_DEVICE);
> + ret = dma_mapping_error(hcd->self.controller,
> + urb->setup_dma);
> + /* warn if DMA mapping failed */
> + if (ret) {
> + WARN_ON(1);
> + return ret;
> + }
First of all you forgot to add { } around everything under if statement.
I don't think WARN_ON is needed...
dma_mapping_error under most architectures return 0 or 1 so it would be
better to make some real error value. EAGAIN seems to be proper error,
since documentation says that driver should try again later.
I would write this error handler like this:
if (dma_mapping_error(hcd->self.controller, urb->setup_dma))
ret = -EAGAIN;
> else if (hcd->driver->flags& HCD_LOCAL_MEM)
> ret = hcd_alloc_coherent(
> urb->dev->bus, mem_flags,
> @@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
> urb->transfer_buffer,
> urb->transfer_buffer_length,
> dir);
> + ret = dma_mapping_error(hcd->self.controller,
> + urb->transfer_dma);
> + /* warn if DMA mapping failed */
> + if (ret) {
> + WARN_ON(1);
> + return ret;
> + }
ditto
> else if (hcd->driver->flags& HCD_LOCAL_MEM) {
> ret = hcd_alloc_coherent(
> urb->dev->bus, mem_flags,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 12:50 ` Paulius Zaleckas
@ 2009-11-04 14:06 ` Larry Finger
2009-11-04 14:51 ` Michael Buesch
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2009-11-04 14:06 UTC (permalink / raw)
To: Paulius Zaleckas; +Cc: Greg Kroah-Hartman, linux-wireless, linux-usb
Paulius Zaleckas wrote:
> On 11/04/2009 07:48 AM, Larry Finger wrote:
>> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
>> with DMA buffer processing was corrected for the libertas driver. Because
>> routine usb_fill_bulk_urb() does not check that DMA is possible when
>> dma_map_single() is called, this condition was not detected until the
>> buffer
>> was unmapped. By this time memory corruption had occurred.
>>
>> The situation is fixed by testing the returned DMA address. If not a
>> legal
>> address, a WARN_ON(1) is executed to provide traceback and the error is
>> returned.
>>
>> Signed-off-by: Larry
>> Finger<Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
>> ---
>>
>> Index: linux-2.6/drivers/usb/core/hcd.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/usb/core/hcd.c
>> +++ linux-2.6/drivers/usb/core/hcd.c
>> @@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
>> urb->setup_packet,
>> sizeof(struct usb_ctrlrequest),
>> DMA_TO_DEVICE);
>> + ret = dma_mapping_error(hcd->self.controller,
>> + urb->setup_dma);
>> + /* warn if DMA mapping failed */
>> + if (ret) {
>> + WARN_ON(1);
>> + return ret;
>> + }
>
> First of all you forgot to add { } around everything under if statement.
>
> I don't think WARN_ON is needed...
> dma_mapping_error under most architectures return 0 or 1 so it would be
> better to make some real error value. EAGAIN seems to be proper error,
> since documentation says that driver should try again later.
>
> I would write this error handler like this:
>
> if (dma_mapping_error(hcd->self.controller, urb->setup_dma))
> ret = -EAGAIN;
>
>> else if (hcd->driver->flags& HCD_LOCAL_MEM)
>> ret = hcd_alloc_coherent(
>> urb->dev->bus, mem_flags,
>> @@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
>> urb->transfer_buffer,
>> urb->transfer_buffer_length,
>> dir);
>> + ret = dma_mapping_error(hcd->self.controller,
>> + urb->transfer_dma);
>> + /* warn if DMA mapping failed */
>> + if (ret) {
>> + WARN_ON(1);
>> + return ret;
>> + }
>
> ditto
>
>> else if (hcd->driver->flags& HCD_LOCAL_MEM) {
>> ret = hcd_alloc_coherent(
>> urb->dev->bus, mem_flags,
>
Thank you for your review and comments. I'll wait a bit to see what
other comments are offered, but I have implemented all your changes.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 14:06 ` Larry Finger
@ 2009-11-04 14:51 ` Michael Buesch
2009-11-04 15:16 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Michael Buesch @ 2009-11-04 14:51 UTC (permalink / raw)
To: Larry Finger
Cc: Paulius Zaleckas, Greg Kroah-Hartman, linux-wireless, linux-usb
On Wednesday 04 November 2009 15:06:25 Larry Finger wrote:
> Thank you for your review and comments. I'll wait a bit to see what
> other comments are offered, but I have implemented all your changes.
If you use a WARN_ON, please use WARN_ON_ONCE to avoid possible spamming of the logs.
--
Greetings, Michael.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 14:51 ` Michael Buesch
@ 2009-11-04 15:16 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2009-11-04 15:16 UTC (permalink / raw)
To: Michael Buesch
Cc: Paulius Zaleckas, Greg Kroah-Hartman, linux-wireless, linux-usb
On 11/04/2009 08:51 AM, Michael Buesch wrote:
> On Wednesday 04 November 2009 15:06:25 Larry Finger wrote:
>> Thank you for your review and comments. I'll wait a bit to see what
>> other comments are offered, but I have implemented all your changes.
>
> If you use a WARN_ON, please use WARN_ON_ONCE to avoid possible spamming of the logs.
Good point.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 5:48 [RFC] usb: Check results of dma_map_single Larry Finger
2009-11-04 12:50 ` Paulius Zaleckas
@ 2009-11-04 19:49 ` Christian Lamparter
2009-11-05 2:41 ` Larry Finger
1 sibling, 1 reply; 7+ messages in thread
From: Christian Lamparter @ 2009-11-04 19:49 UTC (permalink / raw)
To: Larry Finger; +Cc: Greg Kroah-Hartman, linux-wireless, linux-usb
On Wednesday 04 November 2009 06:48:51 Larry Finger wrote:
> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
> with DMA buffer processing was corrected for the libertas driver. Because
> routine usb_fill_bulk_urb() does not check that DMA is possible when
^^^
hmm, usb_fill_bulk_urb? No, that should be usb_submit_urb :)
http://osdir.com/ml/linux-wireless/2009-10/msg01182.html
> dma_map_single() is called, this condition was not detected until the buffer
> was unmapped. By this time memory corruption had occurred.
>
> The situation is fixed by testing the returned DMA address. If not a legal
> address, a WARN_ON(1) is executed to provide traceback and the error is
> returned.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] usb: Check results of dma_map_single
2009-11-04 19:49 ` Christian Lamparter
@ 2009-11-05 2:41 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2009-11-05 2:41 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Greg Kroah-Hartman, linux-wireless, linux-usb
On 11/04/2009 01:49 PM, Christian Lamparter wrote:
> On Wednesday 04 November 2009 06:48:51 Larry Finger wrote:
>> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
>> with DMA buffer processing was corrected for the libertas driver. Because
>> routine usb_fill_bulk_urb() does not check that DMA is possible when
> ^^^
> hmm, usb_fill_bulk_urb? No, that should be usb_submit_urb :)
Thanks. The actual routine modified is map_urb_for_dma.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-11-05 2:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-04 5:48 [RFC] usb: Check results of dma_map_single Larry Finger
2009-11-04 12:50 ` Paulius Zaleckas
2009-11-04 14:06 ` Larry Finger
2009-11-04 14:51 ` Michael Buesch
2009-11-04 15:16 ` Larry Finger
2009-11-04 19:49 ` Christian Lamparter
2009-11-05 2:41 ` Larry Finger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.