Re: [PATCH] brcmfmac: unlink URB when request timed out
On 10-11-14 17:08, Mathy Vanhoef wrote: On 11/10/2014 06:18 AM, Arend van Spriel wrote: On 09-11-14 19:10, Mathy Vanhoef wrote: From: Mathy Vanhoef vanho...@gmail.com Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This assures the URB is never submitted twice, preventing a driver crash. Hi Mathy, What driver crash are you referring to? The log only shows the WARNING ending in a USB disconnect but no actual crash. Does your patch get the driver running properly or does it only avoid the warning. Hi Arend, It shows a warning, after which the device doesn't work (but the computer is still usable). But I've noticed that when *unplugging* the USB cable the OS may freeze. This doesn't always happen though, sometimes unplugging works OK. The patch both avoids the warning, and gets the device/driver running properly (unplugging also works OK). With that said, it seems there is some need for improvement, but I also notice you are running this on a virtual machine so could that affect the timeout to kick in before completion. Could you try to increase the timeout. Still when a timeout occurs this needs to be handled properly. Could you also try the following patch? I did a few additional tests: 1. When increasing IOCTL_RESP_TIMEOUT to 2 (ten times the normal value) the timeout and warning still occur. Device/driver doesn't work. 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the normal value) everything works. Device/driver works. This means the ctl_urb completes on your system within 3sec, but not within 2.1sec. After discussing this with my colleague, we think you should use usb_kill_urb() instead of usb_unlink_urb() as it assures the completion handler is called. Could you retest that and let us know. Regards, Arend 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non- virtualized Linux install: In that case everything worked fine. So the bug may only be triggered in a virtualized environment / VMWare. 4. When applying your patch, the driver stops early during initialization of the device. I included a WARN_ONCE before returning EINVAL and got the output below. Kind regards, Mathy --- [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004 [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter [ 221.487806] usb 1-1: Manufacturer: Broadcom [ 221.487808] usb 1-1: SerialNumber: 0001 [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004 [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter [ 221.490480] brcmfmac: brcmf_usb_attach Enter [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter [ 221.490495] [ cut here ] [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]() [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0 [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2 [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013 [ 221.490528] Workqueue: usb_hub_wq hub_event [ 221.490530] eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58 [ 221.490534] eecffbb4 0064 f11cbc84 02cc f11c1595 f11c1595 c0044000 ffea [ 221.490537] ef726000 eecffba0 c103ee4e 0009 eecffb98 f11cbc58 eecffbb4 eecffbd0 [ 221.490541] Call Trace: [ 221.490550] [c1711f4a] dump_stack+0x41/0x52 [ 221.490558] [c103edaf] warn_slowpath_common+0x7f/0xa0 [ 221.490563] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490567] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490570] [c103ee4e] warn_slowpath_fmt+0x2e/0x30 [ 221.490575] [f11c1595] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490580] [f11c2cd8] brcmf_usb_probe+0x3c8/0x640 [brcmfmac] [ 221.490583] [c1717d53] ? mutex_lock+0x13/0x32 [ 221.490586] [c1493ae3] usb_probe_interface+0xa3/0x180 [ 221.490590] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490592] [c13f546e] driver_probe_device+0x5e/0x1f0 [ 221.490595] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490597] [c13f56c9] __device_attach+0x39/0x50 [ 221.490600] [c13f3d84] bus_for_each_drv+0x34/0x70 [ 221.490602] [c13f53db] device_attach+0x7b/0x90 [ 221.490604] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490607] [c13f4b8f] bus_probe_device+0x6f/0x90 [ 221.490609] [c13f3256] device_add+0x426/0x520 [ 221.490611] [c1491503] ? usb_control_msg+0xb3/0xd0 [ 221.490614]
Re: [PATCH] brcmfmac: unlink URB when request timed out
Using usb_kill_urb() instead of usb_unlink_urb() seems to work without any problems. On 11/11/2014 06:05 AM, Arend van Spriel wrote: On 10-11-14 17:08, Mathy Vanhoef wrote: On 11/10/2014 06:18 AM, Arend van Spriel wrote: On 09-11-14 19:10, Mathy Vanhoef wrote: From: Mathy Vanhoef vanho...@gmail.com Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This assures the URB is never submitted twice, preventing a driver crash. Hi Mathy, What driver crash are you referring to? The log only shows the WARNING ending in a USB disconnect but no actual crash. Does your patch get the driver running properly or does it only avoid the warning. Hi Arend, It shows a warning, after which the device doesn't work (but the computer is still usable). But I've noticed that when *unplugging* the USB cable the OS may freeze. This doesn't always happen though, sometimes unplugging works OK. The patch both avoids the warning, and gets the device/driver running properly (unplugging also works OK). With that said, it seems there is some need for improvement, but I also notice you are running this on a virtual machine so could that affect the timeout to kick in before completion. Could you try to increase the timeout. Still when a timeout occurs this needs to be handled properly. Could you also try the following patch? I did a few additional tests: 1. When increasing IOCTL_RESP_TIMEOUT to 2 (ten times the normal value) the timeout and warning still occur. Device/driver doesn't work. 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the normal value) everything works. Device/driver works. This means the ctl_urb completes on your system within 3sec, but not within 2.1sec. After discussing this with my colleague, we think you should use usb_kill_urb() instead of usb_unlink_urb() as it assures the completion handler is called. Could you retest that and let us know. Regards, Arend 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non- virtualized Linux install: In that case everything worked fine. So the bug may only be triggered in a virtualized environment / VMWare. 4. When applying your patch, the driver stops early during initialization of the device. I included a WARN_ONCE before returning EINVAL and got the output below. Kind regards, Mathy --- [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004 [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter [ 221.487806] usb 1-1: Manufacturer: Broadcom [ 221.487808] usb 1-1: SerialNumber: 0001 [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004 [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter [ 221.490480] brcmfmac: brcmf_usb_attach Enter [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter [ 221.490495] [ cut here ] [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]() [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0 [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2 [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013 [ 221.490528] Workqueue: usb_hub_wq hub_event [ 221.490530] eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58 [ 221.490534] eecffbb4 0064 f11cbc84 02cc f11c1595 f11c1595 c0044000 ffea [ 221.490537] ef726000 eecffba0 c103ee4e 0009 eecffb98 f11cbc58 eecffbb4 eecffbd0 [ 221.490541] Call Trace: [ 221.490550] [c1711f4a] dump_stack+0x41/0x52 [ 221.490558] [c103edaf] warn_slowpath_common+0x7f/0xa0 [ 221.490563] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490567] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490570] [c103ee4e] warn_slowpath_fmt+0x2e/0x30 [ 221.490575] [f11c1595] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490580] [f11c2cd8] brcmf_usb_probe+0x3c8/0x640 [brcmfmac] [ 221.490583] [c1717d53] ? mutex_lock+0x13/0x32 [ 221.490586] [c1493ae3] usb_probe_interface+0xa3/0x180 [ 221.490590] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490592] [c13f546e] driver_probe_device+0x5e/0x1f0 [ 221.490595] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490597] [c13f56c9] __device_attach+0x39/0x50 [ 221.490600] [c13f3d84] bus_for_each_drv+0x34/0x70 [ 221.490602] [c13f53db] device_attach+0x7b/0x90 [ 221.490604] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490607] [c13f4b8f]
Re: [PATCH] brcmfmac: unlink URB when request timed out
On 11-11-14 18:35, Mathy Vanhoef wrote: Using usb_kill_urb() instead of usb_unlink_urb() seems to work without any problems. Ok, as usb_kill_urb() assures the completion handler is called there is no need to check the ctl_completed flag, ie. my patch. Go ahead and resubmit your patch replacing usb_unlink_urb() by usb_kill_urb(). Do you mean you end up with a working usb device providing a wireless interface? Or does probe still fail, but you do not get a crash. Regards, Arend On 11/11/2014 06:05 AM, Arend van Spriel wrote: On 10-11-14 17:08, Mathy Vanhoef wrote: On 11/10/2014 06:18 AM, Arend van Spriel wrote: On 09-11-14 19:10, Mathy Vanhoef wrote: From: Mathy Vanhoef vanho...@gmail.com Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This assures the URB is never submitted twice, preventing a driver crash. Hi Mathy, What driver crash are you referring to? The log only shows the WARNING ending in a USB disconnect but no actual crash. Does your patch get the driver running properly or does it only avoid the warning. Hi Arend, It shows a warning, after which the device doesn't work (but the computer is still usable). But I've noticed that when *unplugging* the USB cable the OS may freeze. This doesn't always happen though, sometimes unplugging works OK. The patch both avoids the warning, and gets the device/driver running properly (unplugging also works OK). With that said, it seems there is some need for improvement, but I also notice you are running this on a virtual machine so could that affect the timeout to kick in before completion. Could you try to increase the timeout. Still when a timeout occurs this needs to be handled properly. Could you also try the following patch? I did a few additional tests: 1. When increasing IOCTL_RESP_TIMEOUT to 2 (ten times the normal value) the timeout and warning still occur. Device/driver doesn't work. 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the normal value) everything works. Device/driver works. This means the ctl_urb completes on your system within 3sec, but not within 2.1sec. After discussing this with my colleague, we think you should use usb_kill_urb() instead of usb_unlink_urb() as it assures the completion handler is called. Could you retest that and let us know. Regards, Arend 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non- virtualized Linux install: In that case everything worked fine. So the bug may only be triggered in a virtualized environment / VMWare. 4. When applying your patch, the driver stops early during initialization of the device. I included a WARN_ONCE before returning EINVAL and got the output below. Kind regards, Mathy --- [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004 [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter [ 221.487806] usb 1-1: Manufacturer: Broadcom [ 221.487808] usb 1-1: SerialNumber: 0001 [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004 [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter [ 221.490480] brcmfmac: brcmf_usb_attach Enter [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter [ 221.490495] [ cut here ] [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]() [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0 [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2 [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013 [ 221.490528] Workqueue: usb_hub_wq hub_event [ 221.490530] eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58 [ 221.490534] eecffbb4 0064 f11cbc84 02cc f11c1595 f11c1595 c0044000 ffea [ 221.490537] ef726000 eecffba0 c103ee4e 0009 eecffb98 f11cbc58 eecffbb4 eecffbd0 [ 221.490541] Call Trace: [ 221.490550] [c1711f4a] dump_stack+0x41/0x52 [ 221.490558] [c103edaf] warn_slowpath_common+0x7f/0xa0 [ 221.490563] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490567] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490570] [c103ee4e] warn_slowpath_fmt+0x2e/0x30 [ 221.490575] [f11c1595] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490580] [f11c2cd8] brcmf_usb_probe+0x3c8/0x640 [brcmfmac] [ 221.490583] [c1717d53] ? mutex_lock+0x13/0x32 [ 221.490586] [c1493ae3] usb_probe_interface+0xa3/0x180 [ 221.490590]
Re: [PATCH] brcmfmac: unlink URB when request timed out
On 11/11/2014 01:00 PM, Arend van Spriel wrote: On 11-11-14 18:35, Mathy Vanhoef wrote: Using usb_kill_urb() instead of usb_unlink_urb() seems to work without any problems. Ok, as usb_kill_urb() assures the completion handler is called there is no need to check the ctl_completed flag, ie. my patch. Go ahead and resubmit your patch replacing usb_unlink_urb() by usb_kill_urb(). Do you mean you end up with a working usb device providing a wireless interface? Or does probe still fail, but you do not get a crash. I end up with a working device which can connect to networks. Kind regards, Mathy Regards, Arend On 11/11/2014 06:05 AM, Arend van Spriel wrote: On 10-11-14 17:08, Mathy Vanhoef wrote: On 11/10/2014 06:18 AM, Arend van Spriel wrote: On 09-11-14 19:10, Mathy Vanhoef wrote: From: Mathy Vanhoef vanho...@gmail.com Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This assures the URB is never submitted twice, preventing a driver crash. Hi Mathy, What driver crash are you referring to? The log only shows the WARNING ending in a USB disconnect but no actual crash. Does your patch get the driver running properly or does it only avoid the warning. Hi Arend, It shows a warning, after which the device doesn't work (but the computer is still usable). But I've noticed that when *unplugging* the USB cable the OS may freeze. This doesn't always happen though, sometimes unplugging works OK. The patch both avoids the warning, and gets the device/driver running properly (unplugging also works OK). With that said, it seems there is some need for improvement, but I also notice you are running this on a virtual machine so could that affect the timeout to kick in before completion. Could you try to increase the timeout. Still when a timeout occurs this needs to be handled properly. Could you also try the following patch? I did a few additional tests: 1. When increasing IOCTL_RESP_TIMEOUT to 2 (ten times the normal value) the timeout and warning still occur. Device/driver doesn't work. 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the normal value) everything works. Device/driver works. This means the ctl_urb completes on your system within 3sec, but not within 2.1sec. After discussing this with my colleague, we think you should use usb_kill_urb() instead of usb_unlink_urb() as it assures the completion handler is called. Could you retest that and let us know. Regards, Arend 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non- virtualized Linux install: In that case everything worked fine. So the bug may only be triggered in a virtualized environment / VMWare. 4. When applying your patch, the driver stops early during initialization of the device. I included a WARN_ONCE before returning EINVAL and got the output below. Kind regards, Mathy --- [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004 [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter [ 221.487806] usb 1-1: Manufacturer: Broadcom [ 221.487808] usb 1-1: SerialNumber: 0001 [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004 [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter [ 221.490480] brcmfmac: brcmf_usb_attach Enter [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter [ 221.490495] [ cut here ] [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]() [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0 [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2 [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013 [ 221.490528] Workqueue: usb_hub_wq hub_event [ 221.490530] eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58 [ 221.490534] eecffbb4 0064 f11cbc84 02cc f11c1595 f11c1595 c0044000 ffea [ 221.490537] ef726000 eecffba0 c103ee4e 0009 eecffb98 f11cbc58 eecffbb4 eecffbd0 [ 221.490541] Call Trace: [ 221.490550] [c1711f4a] dump_stack+0x41/0x52 [ 221.490558] [c103edaf] warn_slowpath_common+0x7f/0xa0 [ 221.490563] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490567] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490570] [c103ee4e] warn_slowpath_fmt+0x2e/0x30 [ 221.490575] [f11c1595] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490580] [f11c2cd8]
Re: [PATCH] brcmfmac: unlink URB when request timed out
On 11/10/2014 04:08 AM, Oliver Neukum wrote:
On Sun, 2014-11-09 at 13:10 -0500, Mathy Vanhoef wrote:
From: Mathy Vanhoef vanho...@gmail.com
Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
assures the URB is never submitted twice, preventing a driver crash.
Hi,
I am afrad this patch is no good. The diagnosis is good,
but the fix introduces serious problems.
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
index 5265aa7..1bc7858 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
@@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info
*devinfo, u8 cmd,
goto finalize;
}
- if (!brcmf_usb_ioctl_resp_wait(devinfo))
+ if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
+ usb_unlink_urb(devinfo-ctl_urb);
This is the asynchronous unlink. You have no guarantee it is finished
after this point.
ret = -ETIMEDOUT;
- else
+ } else {
memcpy(buffer, tmpbuf, buflen);
+ }
finalize:
kfree(tmpbuf);
Which means that you are freeing memory that may still be used by DMA
at this time.
In addition you have no guarantee that the unlink is indeed finished
by the time the URB is reused.
If you wish to take this approach you better forget about this URB
and allocate a new one and free the buffer from the callback.
Hi Oliver,
Good catch. I think the DMA issue is also present in the current driver: it
frees the buffer without unlinking/killing the URB at all. Can a malicious USB
device force a timeout to occur (i.e. delay the call to the completion
handler)? If so this might be a use-after-free vulnerability.
It seems using usb_kill_urb instead of usb_unlink_urb in the patch prevents any
possible use-after-free. Can someone double check?
Kind regards,
Mathy
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] brcmfmac: unlink URB when request timed out
On Sun, 2014-11-09 at 13:10 -0500, Mathy Vanhoef wrote:
From: Mathy Vanhoef vanho...@gmail.com
Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
assures the URB is never submitted twice, preventing a driver crash.
Hi,
I am afrad this patch is no good. The diagnosis is good,
but the fix introduces serious problems.
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
index 5265aa7..1bc7858 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
@@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info
*devinfo, u8 cmd,
goto finalize;
}
- if (!brcmf_usb_ioctl_resp_wait(devinfo))
+ if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
+ usb_unlink_urb(devinfo-ctl_urb);
This is the asynchronous unlink. You have no guarantee it is finished
after this point.
ret = -ETIMEDOUT;
- else
+ } else {
memcpy(buffer, tmpbuf, buflen);
+ }
finalize:
kfree(tmpbuf);
Which means that you are freeing memory that may still be used by DMA
at this time.
In addition you have no guarantee that the unlink is indeed finished
by the time the URB is reused.
If you wish to take this approach you better forget about this URB
and allocate a new one and free the buffer from the callback.
Regards
Oliver
--
Oliver Neukum oneu...@suse.de
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] brcmfmac: unlink URB when request timed out
On 11/10/2014 06:18 AM, Arend van Spriel wrote: On 09-11-14 19:10, Mathy Vanhoef wrote: From: Mathy Vanhoef vanho...@gmail.com Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This assures the URB is never submitted twice, preventing a driver crash. Hi Mathy, What driver crash are you referring to? The log only shows the WARNING ending in a USB disconnect but no actual crash. Does your patch get the driver running properly or does it only avoid the warning. Hi Arend, It shows a warning, after which the device doesn't work (but the computer is still usable). But I've noticed that when *unplugging* the USB cable the OS may freeze. This doesn't always happen though, sometimes unplugging works OK. The patch both avoids the warning, and gets the device/driver running properly (unplugging also works OK). With that said, it seems there is some need for improvement, but I also notice you are running this on a virtual machine so could that affect the timeout to kick in before completion. Could you try to increase the timeout. Still when a timeout occurs this needs to be handled properly. Could you also try the following patch? I did a few additional tests: 1. When increasing IOCTL_RESP_TIMEOUT to 2 (ten times the normal value) the timeout and warning still occur. Device/driver doesn't work. 2. When increasing BRCMF_USB_RESET_GETVER_SPINWAIT to 1000 (ten timers the normal value) everything works. Device/driver works. 3. Quick test using backports-3.18-rc1-1 (aka unpatched driver) on a non- virtualized Linux install: In that case everything worked fine. So the bug may only be triggered in a virtualized environment / VMWare. 4. When applying your patch, the driver stops early during initialization of the device. I included a WARN_ONCE before returning EINVAL and got the output below. Kind regards, Mathy --- [ 220.955647] usb 1-1: new high-speed USB device number 3 using ehci-pci [ 221.487797] usb 1-1: New USB device found, idVendor=043e, idProduct=3004 [ 221.487802] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 221.487804] usb 1-1: Product: Remote Download Wireless Adapter [ 221.487806] usb 1-1: Manufacturer: Broadcom [ 221.487808] usb 1-1: SerialNumber: 0001 [ 221.490472] brcmfmac: brcmf_usb_probe Enter 0x043e:0x3004 [ 221.490476] brcmfmac: brcmf_usb_probe Broadcom high speed USB WLAN interface detected [ 221.490477] brcmfmac: brcmf_usb_probe_cb Enter [ 221.490480] brcmfmac: brcmf_usb_attach Enter [ 221.490494] brcmfmac: brcmf_usb_dlneeded Enter [ 221.490495] [ cut here ] [ 221.490503] WARNING: CPU: 0 PID: 100 at drivers/net/wireless/brcm80211/brcmfmac/usb.c:716 brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac]() [ 221.490505] EINVAL devinfo=c0044000 ctl_rub=ef898380 completed=0 [ 221.490506] Modules linked in: brcmfmac brcmutil vmw_pvscsi pcnet32 mptspi mptscsih mptbase [ 221.490514] CPU: 0 PID: 100 Comm: kworker/0:1 Not tainted 3.18.0-rc3-wl+ #2 [ 221.490515] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013 [ 221.490528] Workqueue: usb_hub_wq hub_event [ 221.490530] eecffb58 c1711f4a eecffb98 eecffb88 c103edaf f11cbc58 [ 221.490534] eecffbb4 0064 f11cbc84 02cc f11c1595 f11c1595 c0044000 ffea [ 221.490537] ef726000 eecffba0 c103ee4e 0009 eecffb98 f11cbc58 eecffbb4 eecffbd0 [ 221.490541] Call Trace: [ 221.490550] [c1711f4a] dump_stack+0x41/0x52 [ 221.490558] [c103edaf] warn_slowpath_common+0x7f/0xa0 [ 221.490563] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490567] [f11c1595] ? brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490570] [c103ee4e] warn_slowpath_fmt+0x2e/0x30 [ 221.490575] [f11c1595] brcmf_usb_dl_cmd+0x75/0x1a0 [brcmfmac] [ 221.490580] [f11c2cd8] brcmf_usb_probe+0x3c8/0x640 [brcmfmac] [ 221.490583] [c1717d53] ? mutex_lock+0x13/0x32 [ 221.490586] [c1493ae3] usb_probe_interface+0xa3/0x180 [ 221.490590] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490592] [c13f546e] driver_probe_device+0x5e/0x1f0 [ 221.490595] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490597] [c13f56c9] __device_attach+0x39/0x50 [ 221.490600] [c13f3d84] bus_for_each_drv+0x34/0x70 [ 221.490602] [c13f53db] device_attach+0x7b/0x90 [ 221.490604] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490607] [c13f4b8f] bus_probe_device+0x6f/0x90 [ 221.490609] [c13f3256] device_add+0x426/0x520 [ 221.490611] [c1491503] ? usb_control_msg+0xb3/0xd0 [ 221.490614] [c1717d53] ? mutex_lock+0x13/0x32 [ 221.490627] [c14922f8] usb_set_configuration+0x3f8/0x700 [ 221.490630] [c13f5690] ? __driver_attach+0x90/0x90 [ 221.490633] [c149ac7b] generic_probe+0x2b/0x90 [ 221.490637] [c1188bc0] ? sysfs_create_link+0x20/0x40 [ 221.490639] [c1492bec] usb_probe_device+0xc/0x10 [ 221.490641] [c13f546e] driver_probe_device+0x5e/0x1f0 [ 221.490644] [c13f5690] ?
