linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] brcmfmac: fix race during USB disconnect
@ 2019-03-08 15:25 Piotr Figiel
  2019-03-08 15:25 ` [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress Piotr Figiel
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Piotr Figiel @ 2019-03-08 15:25 UTC (permalink / raw)
  To: linux-wireless, arend.vanspriel, kvalo
  Cc: franky.lin, hante.meuleman, chi-hsien.lin, wright.feng,
	brcm80211-dev-list, Pawel Lenkow, Lech Perczak,
	Krzysztof Drobiński, Piotr Figiel

There is an issue in brcmfmac clean-up during USB disconnect which may lead to
usb_hub_wq lock-up and possible memory corruption. With this series fix the
race causing the issue and also do minor clean-up of brcmf_usb_free_q.

Piotr Figiel (3):
  brcmfmac: fix race during disconnect when USB completion is in
    progress
  brcmfmac: remove pending parameter from brcmf_usb_free_q
  brcmfmac: remove unused variable i from brcmf_usb_free_q

 .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 27 +++++++++++-----------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress
  2019-03-08 15:25 [PATCH 0/3] brcmfmac: fix race during USB disconnect Piotr Figiel
@ 2019-03-08 15:25 ` Piotr Figiel
  2019-04-04 10:11   ` Kalle Valo
  2019-03-08 15:25 ` [PATCH 2/3] brcmfmac: remove pending parameter from brcmf_usb_free_q Piotr Figiel
  2019-03-08 15:25 ` [PATCH 3/3] brcmfmac: remove unused variable i " Piotr Figiel
  2 siblings, 1 reply; 5+ messages in thread
From: Piotr Figiel @ 2019-03-08 15:25 UTC (permalink / raw)
  To: linux-wireless, arend.vanspriel, kvalo
  Cc: franky.lin, hante.meuleman, chi-hsien.lin, wright.feng,
	brcm80211-dev-list, Pawel Lenkow, Lech Perczak,
	Krzysztof Drobiński, Piotr Figiel

It was observed that rarely during USB disconnect happening shortly after
connect (before full initialization completes) usb_hub_wq would wait
forever for the dev_init_lock to be unlocked. dev_init_lock would remain
locked though because of infinite wait during usb_kill_urb:

[ 2730.656472] kworker/0:2     D    0   260      2 0x00000000
[ 2730.660700] Workqueue: events request_firmware_work_func
[ 2730.664807] [<809dca20>] (__schedule) from [<809dd164>] (schedule+0x4c/0xac)
[ 2730.670587] [<809dd164>] (schedule) from [<8069af44>] (usb_kill_urb+0xdc/0x114)
[ 2730.676815] [<8069af44>] (usb_kill_urb) from [<7f258b50>] (brcmf_usb_free_q+0x34/0xa8 [brcmfmac])
[ 2730.684833] [<7f258b50>] (brcmf_usb_free_q [brcmfmac]) from [<7f2517d4>] (brcmf_detach+0xa0/0xb8 [brcmfmac])
[ 2730.693557] [<7f2517d4>] (brcmf_detach [brcmfmac]) from [<7f251a34>] (brcmf_attach+0xac/0x3d8 [brcmfmac])
[ 2730.702094] [<7f251a34>] (brcmf_attach [brcmfmac]) from [<7f2587ac>] (brcmf_usb_probe_phase2+0x468/0x4a0 [brcmfmac])
[ 2730.711601] [<7f2587ac>] (brcmf_usb_probe_phase2 [brcmfmac]) from [<7f252888>] (brcmf_fw_request_done+0x194/0x220 [brcmfmac])
[ 2730.721795] [<7f252888>] (brcmf_fw_request_done [brcmfmac]) from [<805748e4>] (request_firmware_work_func+0x4c/0x88)
[ 2730.731125] [<805748e4>] (request_firmware_work_func) from [<80141474>] (process_one_work+0x228/0x808)
[ 2730.739223] [<80141474>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
[ 2730.746105] [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
[ 2730.752227] [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)

[ 2733.099695] kworker/0:3     D    0  1065      2 0x00000000
[ 2733.103926] Workqueue: usb_hub_wq hub_event
[ 2733.106914] [<809dca20>] (__schedule) from [<809dd164>] (schedule+0x4c/0xac)
[ 2733.112693] [<809dd164>] (schedule) from [<809e2a8c>] (schedule_timeout+0x214/0x3e4)
[ 2733.119621] [<809e2a8c>] (schedule_timeout) from [<809dde2c>] (wait_for_common+0xc4/0x1c0)
[ 2733.126810] [<809dde2c>] (wait_for_common) from [<7f258d00>] (brcmf_usb_disconnect+0x1c/0x4c [brcmfmac])
[ 2733.135206] [<7f258d00>] (brcmf_usb_disconnect [brcmfmac]) from [<8069e0c8>] (usb_unbind_interface+0x5c/0x1e4)
[ 2733.143943] [<8069e0c8>] (usb_unbind_interface) from [<8056d3e8>] (device_release_driver_internal+0x164/0x1fc)
[ 2733.152769] [<8056d3e8>] (device_release_driver_internal) from [<8056c078>] (bus_remove_device+0xd0/0xfc)
[ 2733.161138] [<8056c078>] (bus_remove_device) from [<8056977c>] (device_del+0x11c/0x310)
[ 2733.167939] [<8056977c>] (device_del) from [<8069cba8>] (usb_disable_device+0xa0/0x1cc)
[ 2733.174743] [<8069cba8>] (usb_disable_device) from [<8069507c>] (usb_disconnect+0x74/0x1dc)
[ 2733.181823] [<8069507c>] (usb_disconnect) from [<80695e88>] (hub_event+0x478/0xf88)
[ 2733.188278] [<80695e88>] (hub_event) from [<80141474>] (process_one_work+0x228/0x808)
[ 2733.194905] [<80141474>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
[ 2733.201724] [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
[ 2733.207913] [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)

It was traced down to a case where usb_kill_urb would be called on an URB
structure containing more or less random data, including large number in
its use_count. During the debugging it appeared that in brcmf_usb_free_q()
the traversal over URBs' lists is not synchronized with operations on those
lists in brcmf_usb_rx_complete() leading to handling
brcmf_usbdev_info structure (holding lists' head) as lists' element and in
result causing above problem.

Fix it by walking through all URBs during brcmf_cancel_all_urbs using the
arrays of requests instead of linked lists.

Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index e9cbfd0..a775409 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -682,12 +682,18 @@ static int brcmf_usb_up(struct device *dev)
 
 static void brcmf_cancel_all_urbs(struct brcmf_usbdev_info *devinfo)
 {
+	int i;
+
 	if (devinfo->ctl_urb)
 		usb_kill_urb(devinfo->ctl_urb);
 	if (devinfo->bulk_urb)
 		usb_kill_urb(devinfo->bulk_urb);
-	brcmf_usb_free_q(&devinfo->tx_postq, true);
-	brcmf_usb_free_q(&devinfo->rx_postq, true);
+	if (devinfo->tx_reqs)
+		for (i = 0; i < devinfo->bus_pub.ntxq; i++)
+			usb_kill_urb(devinfo->tx_reqs[i].urb);
+	if (devinfo->rx_reqs)
+		for (i = 0; i < devinfo->bus_pub.nrxq; i++)
+			usb_kill_urb(devinfo->rx_reqs[i].urb);
 }
 
 static void brcmf_usb_down(struct device *dev)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/3] brcmfmac: remove pending parameter from brcmf_usb_free_q
  2019-03-08 15:25 [PATCH 0/3] brcmfmac: fix race during USB disconnect Piotr Figiel
  2019-03-08 15:25 ` [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress Piotr Figiel
@ 2019-03-08 15:25 ` Piotr Figiel
  2019-03-08 15:25 ` [PATCH 3/3] brcmfmac: remove unused variable i " Piotr Figiel
  2 siblings, 0 replies; 5+ messages in thread
From: Piotr Figiel @ 2019-03-08 15:25 UTC (permalink / raw)
  To: linux-wireless, arend.vanspriel, kvalo
  Cc: franky.lin, hante.meuleman, chi-hsien.lin, wright.feng,
	brcm80211-dev-list, Pawel Lenkow, Lech Perczak,
	Krzysztof Drobiński, Piotr Figiel

brcmf_usb_free_q is no longer called with pending=true thus this boolean
parameter is no longer needed.

Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index a775409..5ab397d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -445,9 +445,10 @@ brcmf_usbdev_qinit(struct list_head *q, int qsize)
 
 }
 
-static void brcmf_usb_free_q(struct list_head *q, bool pending)
+static void brcmf_usb_free_q(struct list_head *q)
 {
 	struct brcmf_usbreq *req, *next;
+
 	int i = 0;
 	list_for_each_entry_safe(req, next, q, list) {
 		if (!req->urb) {
@@ -455,12 +456,8 @@ static void brcmf_usb_free_q(struct list_head *q, bool pending)
 			break;
 		}
 		i++;
-		if (pending) {
-			usb_kill_urb(req->urb);
-		} else {
-			usb_free_urb(req->urb);
-			list_del_init(&req->list);
-		}
+		usb_free_urb(req->urb);
+		list_del_init(&req->list);
 	}
 }
 
@@ -1029,8 +1026,8 @@ static void brcmf_usb_detach(struct brcmf_usbdev_info *devinfo)
 	brcmf_dbg(USB, "Enter, devinfo %p\n", devinfo);
 
 	/* free the URBS */
-	brcmf_usb_free_q(&devinfo->rx_freeq, false);
-	brcmf_usb_free_q(&devinfo->tx_freeq, false);
+	brcmf_usb_free_q(&devinfo->rx_freeq);
+	brcmf_usb_free_q(&devinfo->tx_freeq);
 
 	usb_free_urb(devinfo->ctl_urb);
 	usb_free_urb(devinfo->bulk_urb);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 3/3] brcmfmac: remove unused variable i from brcmf_usb_free_q
  2019-03-08 15:25 [PATCH 0/3] brcmfmac: fix race during USB disconnect Piotr Figiel
  2019-03-08 15:25 ` [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress Piotr Figiel
  2019-03-08 15:25 ` [PATCH 2/3] brcmfmac: remove pending parameter from brcmf_usb_free_q Piotr Figiel
@ 2019-03-08 15:25 ` Piotr Figiel
  2 siblings, 0 replies; 5+ messages in thread
From: Piotr Figiel @ 2019-03-08 15:25 UTC (permalink / raw)
  To: linux-wireless, arend.vanspriel, kvalo
  Cc: franky.lin, hante.meuleman, chi-hsien.lin, wright.feng,
	brcm80211-dev-list, Pawel Lenkow, Lech Perczak,
	Krzysztof Drobiński, Piotr Figiel

Variable i is not used so remove it.

Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 5ab397d..c00b9fd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -449,13 +449,11 @@ static void brcmf_usb_free_q(struct list_head *q)
 {
 	struct brcmf_usbreq *req, *next;
 
-	int i = 0;
 	list_for_each_entry_safe(req, next, q, list) {
 		if (!req->urb) {
 			brcmf_err("bad req\n");
 			break;
 		}
-		i++;
 		usb_free_urb(req->urb);
 		list_del_init(&req->list);
 	}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress
  2019-03-08 15:25 ` [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress Piotr Figiel
@ 2019-04-04 10:11   ` Kalle Valo
  0 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-04-04 10:11 UTC (permalink / raw)
  To: Piotr Figiel
  Cc: linux-wireless, arend.vanspriel, franky.lin, hante.meuleman,
	chi-hsien.lin, wright.feng, brcm80211-dev-list, Pawel Lenkow,
	Lech Perczak, Krzysztof Drobiński, Piotr Figiel

Piotr Figiel <p.figiel@camlintechnologies.com> wrote:

> It was observed that rarely during USB disconnect happening shortly after
> connect (before full initialization completes) usb_hub_wq would wait
> forever for the dev_init_lock to be unlocked. dev_init_lock would remain
> locked though because of infinite wait during usb_kill_urb:
> 
> [ 2730.656472] kworker/0:2     D    0   260      2 0x00000000
> [ 2730.660700] Workqueue: events request_firmware_work_func
> [ 2730.664807] [<809dca20>] (__schedule) from [<809dd164>] (schedule+0x4c/0xac)
> [ 2730.670587] [<809dd164>] (schedule) from [<8069af44>] (usb_kill_urb+0xdc/0x114)
> [ 2730.676815] [<8069af44>] (usb_kill_urb) from [<7f258b50>] (brcmf_usb_free_q+0x34/0xa8 [brcmfmac])
> [ 2730.684833] [<7f258b50>] (brcmf_usb_free_q [brcmfmac]) from [<7f2517d4>] (brcmf_detach+0xa0/0xb8 [brcmfmac])
> [ 2730.693557] [<7f2517d4>] (brcmf_detach [brcmfmac]) from [<7f251a34>] (brcmf_attach+0xac/0x3d8 [brcmfmac])
> [ 2730.702094] [<7f251a34>] (brcmf_attach [brcmfmac]) from [<7f2587ac>] (brcmf_usb_probe_phase2+0x468/0x4a0 [brcmfmac])
> [ 2730.711601] [<7f2587ac>] (brcmf_usb_probe_phase2 [brcmfmac]) from [<7f252888>] (brcmf_fw_request_done+0x194/0x220 [brcmfmac])
> [ 2730.721795] [<7f252888>] (brcmf_fw_request_done [brcmfmac]) from [<805748e4>] (request_firmware_work_func+0x4c/0x88)
> [ 2730.731125] [<805748e4>] (request_firmware_work_func) from [<80141474>] (process_one_work+0x228/0x808)
> [ 2730.739223] [<80141474>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
> [ 2730.746105] [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
> [ 2730.752227] [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
> 
> [ 2733.099695] kworker/0:3     D    0  1065      2 0x00000000
> [ 2733.103926] Workqueue: usb_hub_wq hub_event
> [ 2733.106914] [<809dca20>] (__schedule) from [<809dd164>] (schedule+0x4c/0xac)
> [ 2733.112693] [<809dd164>] (schedule) from [<809e2a8c>] (schedule_timeout+0x214/0x3e4)
> [ 2733.119621] [<809e2a8c>] (schedule_timeout) from [<809dde2c>] (wait_for_common+0xc4/0x1c0)
> [ 2733.126810] [<809dde2c>] (wait_for_common) from [<7f258d00>] (brcmf_usb_disconnect+0x1c/0x4c [brcmfmac])
> [ 2733.135206] [<7f258d00>] (brcmf_usb_disconnect [brcmfmac]) from [<8069e0c8>] (usb_unbind_interface+0x5c/0x1e4)
> [ 2733.143943] [<8069e0c8>] (usb_unbind_interface) from [<8056d3e8>] (device_release_driver_internal+0x164/0x1fc)
> [ 2733.152769] [<8056d3e8>] (device_release_driver_internal) from [<8056c078>] (bus_remove_device+0xd0/0xfc)
> [ 2733.161138] [<8056c078>] (bus_remove_device) from [<8056977c>] (device_del+0x11c/0x310)
> [ 2733.167939] [<8056977c>] (device_del) from [<8069cba8>] (usb_disable_device+0xa0/0x1cc)
> [ 2733.174743] [<8069cba8>] (usb_disable_device) from [<8069507c>] (usb_disconnect+0x74/0x1dc)
> [ 2733.181823] [<8069507c>] (usb_disconnect) from [<80695e88>] (hub_event+0x478/0xf88)
> [ 2733.188278] [<80695e88>] (hub_event) from [<80141474>] (process_one_work+0x228/0x808)
> [ 2733.194905] [<80141474>] (process_one_work) from [<80141a80>] (worker_thread+0x2c/0x564)
> [ 2733.201724] [<80141a80>] (worker_thread) from [<80147bcc>] (kthread+0x13c/0x16c)
> [ 2733.207913] [<80147bcc>] (kthread) from [<801010b4>] (ret_from_fork+0x14/0x20)
> 
> It was traced down to a case where usb_kill_urb would be called on an URB
> structure containing more or less random data, including large number in
> its use_count. During the debugging it appeared that in brcmf_usb_free_q()
> the traversal over URBs' lists is not synchronized with operations on those
> lists in brcmf_usb_rx_complete() leading to handling
> brcmf_usbdev_info structure (holding lists' head) as lists' element and in
> result causing above problem.
> 
> Fix it by walking through all URBs during brcmf_cancel_all_urbs using the
> arrays of requests instead of linked lists.
> 
> Signed-off-by: Piotr Figiel <p.figiel@camlintechnologies.com>

3 patches applied to wireless-drivers-next.git, thanks.

db3b9e2e1d58 brcmfmac: fix race during disconnect when USB completion is in progress
2b78e5f52236 brcmfmac: remove pending parameter from brcmf_usb_free_q
504f06725d01 brcmfmac: remove unused variable i from brcmf_usb_free_q

-- 
https://patchwork.kernel.org/patch/10845051/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-04 10:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-08 15:25 [PATCH 0/3] brcmfmac: fix race during USB disconnect Piotr Figiel
2019-03-08 15:25 ` [PATCH 1/3] brcmfmac: fix race during disconnect when USB completion is in progress Piotr Figiel
2019-04-04 10:11   ` Kalle Valo
2019-03-08 15:25 ` [PATCH 2/3] brcmfmac: remove pending parameter from brcmf_usb_free_q Piotr Figiel
2019-03-08 15:25 ` [PATCH 3/3] brcmfmac: remove unused variable i " Piotr Figiel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).