| Message ID | 34115.192.168.0.40.1232483361.squirrel@server |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: "Christian Eggers" <ceggers@gmx.de> Date: Tue, 20 Jan 2009 21:29:21 +0100 (CET) > This is my first patch submission for Linux. I hope everything is fine. Unfortuntately not, your email client has corrupted the patch changing tab characters into spaces. Please resubmit this after correcting this problem. See: Documentation/email-clients.txt in the kernel source for some help with this issue. Thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 20 January 2009, you wrote: > From: Christian Eggers <christian.eggers@kathrein.de> > > mcs7830_set_reg() and mcs7830_get_reg() are called with buffers > from stack which must not be used directly for USB transfers. > This causes corruption of the stack particulary on non x86 > architectures because DMA may be used for these transfers. > > Signed-off-by: Christian Eggers <christian.eggers@kathrein.de> Have you observed problems with this, or just suspected trouble? When I wrote this code, I looked at other code doing the same and assumed it was ok, because usb_control_msg waits for the DMA to complete before returning. Is the problem only on systems that have noncoherent DMA, or something else? > This is my first patch submission for Linux. I hope everything is fine. I was about to say that you should have Cc:'d me, but then I noticed that I'm not listed in the driver as maintainer, nor in the MAINTAINERS file, so I can't really complain here ;-) Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arnd Bergmann <arnd@arndb.de> Date: Tue, 20 Jan 2009 23:45:47 +0100 > On Tuesday 20 January 2009, you wrote: > > From: Christian Eggers <christian.eggers@kathrein.de> > > > > mcs7830_set_reg() and mcs7830_get_reg() are called with buffers > > from stack which must not be used directly for USB transfers. > > This causes corruption of the stack particulary on non x86 > > architectures because DMA may be used for these transfers. > > > > Signed-off-by: Christian Eggers <christian.eggers@kathrein.de> > > Have you observed problems with this, or just suspected trouble? Yes, this is in response to SH platform failures. You cannot DMA from/to the kernel stack, because it might not be in the aliased linear mapping of physical memory. It could even be vmalloc()'d memory on some platforms. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Tuesday 20 January 2009 23:45:47 schrieb Arnd Bergmann: > Have you observed problems with this, or just suspected trouble? > > When I wrote this code, I looked at other code doing the same > and assumed it was ok, because usb_control_msg waits for the > DMA to complete before returning. > > Is the problem only on systems that have noncoherent DMA, or > something else? That's not enough. Tasks can leave pointers to variables on the stack to other tasks. You must under no circumstances do DMA on the stack if the driver may run on system that have noncoherent DMA. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 20 January 2009, David Miller wrote: > You cannot DMA from/to the kernel stack, because it might not be in > the aliased linear mapping of physical memory. It could even be > vmalloc()'d memory on some platforms. Ok, I see. It seems a bit of a waste to do a kmalloc for something that is guaranteed to be just a few bytes, but allocating the buffer per-device would mean another mutex, which has about the same overhead, so I'm basically ok with the patch. > + buffer = kmalloc(size, GFP_NOIO); GFP_NOIO seems out of place in a network driver: there is nothing wrong with waiting for I/O here, so plain GFP_KERNEL should be fine. > + if (buffer == NULL) > + return -ENOMEM; I'd prefer to write if (!buffer) here, as I do elsewhere in the driver. Otherwise, Acked-by: Arnd Bergmann <arnd@arndb.de> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arnd Bergmann <arnd@arndb.de> Date: Wed, 21 Jan 2009 00:17:18 +0100 > On Tuesday 20 January 2009, David Miller wrote: > > + buffer = kmalloc(size, GFP_NOIO); > > GFP_NOIO seems out of place in a network driver: there is nothing > wrong with waiting for I/O here, so plain GFP_KERNEL should be fine. There seems to be a large precendence for this in other USB drivers, both for networking and storage. Probably a mutex or other locking hierarchy issue. Really, I would just apply this patch as-is. It works, it's pretty clean, and every retort has been a misunderstanding or extreme nit-picking :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Wednesday 21 January 2009 00:17:18 schrieb Arnd Bergmann: > > + buffer = kmalloc(size, GFP_NOIO); > > GFP_NOIO seems out of place in a network driver: there is nothing > wrong with waiting for I/O here, so plain GFP_KERNEL should be fine. What happens if you run nfs over that link? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Wednesday 21 January 2009 00:23:45 schrieb David Miller: > > GFP_NOIO seems out of place in a network driver: there is nothing > > wrong with waiting for I/O here, so plain GFP_KERNEL should be fine. > > There seems to be a large precendence for this in other USB drivers, > both for networking and storage. Probably a mutex or other locking > hierarchy issue. Usb storage uses a lot of infrastructure in error handling. As a storage interface and another interface can share a device and be reset only together. All drivers' reset handling must be written as if they were block devices. Therefore you see a lot of GFP_NOIO in USB. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 21 January 2009, David Miller wrote: > There seems to be a large precendence for this in other USB drivers, > both for networking and storage. Probably a mutex or other locking > hierarchy issue. > > Really, I would just apply this patch as-is. It works, it's pretty > clean, and every retort has been a misunderstanding or extreme > nit-picking :-) Ok, fair enough. Please add my Acked-by then. On a related topic, can we put something in place that can check for this error at run-time, like a WARN_ON(is_kernel_stack(addr)) in dma_map_single? Since I only copied this code from elsewhere, I would suspect that there are lots of similar bugs that never get found on common hardware otherwise. Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arnd Bergmann <arnd@arndb.de> Date: Wed, 21 Jan 2009 00:37:51 +0100 > On a related topic, can we put something in place that can check for > this error at run-time, like a WARN_ON(is_kernel_stack(addr)) in > dma_map_single? That would be something for the DMA API debugging patches that have been floating around lately. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -uprN linux-2.6.28.1.orig/drivers/net/usb/mcs7830.c linux-2.6.28.1/drivers/net/usb/mcs7830.c --- linux-2.6.28.1.orig/drivers/net/usb/mcs7830.c 2009-01-18 19:45:37.000000000 +0100 +++ linux-2.6.28.1/drivers/net/usb/mcs7830.c 2009-01-20 20:53:59.000000000 +0100 @@ -94,10 +94,18 @@ static int mcs7830_get_reg(struct usbnet { struct usb_device *xdev = dev->udev; int ret; + void *buffer; + + buffer = kmalloc(size, GFP_NOIO); + if (buffer == NULL) + return -ENOMEM; ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ, - MCS7830_RD_BMREQ, 0x0000, index, data, + MCS7830_RD_BMREQ, 0x0000, index, buffer, size, MCS7830_CTRL_TIMEOUT); + memcpy(data, buffer, size); + kfree(buffer); + return ret; } @@ -105,10 +113,18 @@ static int mcs7830_set_reg(struct usbnet { struct usb_device *xdev = dev->udev; int ret; + void *buffer; + + buffer = kmalloc(size, GFP_NOIO); + if (buffer == NULL) + return -ENOMEM; + + memcpy(buffer, data, size); ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ, - MCS7830_WR_BMREQ, 0x0000, index, data, + MCS7830_WR_BMREQ, 0x0000, index, buffer, size, MCS7830_CTRL_TIMEOUT); + kfree(buffer); return ret; }