This series fixes the hung task bug in hub_port_init reported by
syzkaller at:
https://syzkaller.appspot.com/bug?id=08f12df95ae7da69814e64eb5515d5a85ed06b76
Stack:
INFO: task kworker/0:0:5 blocked for more than 143 seconds.
Not tainted 5.13.0-rc7-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:kworker/0:0 state:D
stack:27392 pid: 5 ppid: 2 flags:0x00004000
Workqueue: usb_hub_wq hub_event
Call Trace:
context_switch kernel/sched/core.c:4339 [inline]
__schedule+0x916/0x23e0 kernel/sched/core.c:5147
schedule+0xcf/0x270 kernel/sched/core.c:5226
usb_kill_urb.part.0+0x19c/0x220 drivers/usb/core/urb.c:711
usb_kill_urb+0x81/0xa0 drivers/usb/core/urb.c:706
usb_start_wait_urb+0x24a/0x4c0 drivers/usb/core/message.c:64
usb_internal_control_msg drivers/usb/core/message.c:102 [inline]
usb_control_msg+0x31c/0x4a0 drivers/usb/core/message.c:153
hub_port_init+0x82e/0x2db0 drivers/usb/core/hub.c:4759
hub_port_connect drivers/usb/core/hub.c:5210 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5418 [inline]
port_event drivers/usb/core/hub.c:5564 [inline]
hub_event+0x2190/0x4330 drivers/usb/core/hub.c:5646
process_one_work+0x98d/0x1600 kernel/workqueue.c:2276
worker_thread+0x64c/0x1120 kernel/workqueue.c:2422
kthread+0x3b1/0x4a0 kernel/kthread.c:313
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
The first patch in the series fixes the issue and the second patch does
some refactoring to avoid duplicate code.
Changes in v4:
- Got rid of the log messages as suggested by Shuah.
Changes in v3:
- Split the patch into two patches
- Remove the convenience wrappers as suggested by Shuah
- Remove the WARN as suggested by Greg
Link: https://lore.kernel.org/lkml/[email protected]/
Changes in v2:
Use WARN_ON() instead of BUG() when unlink_list is neither unlink_tx nor
unlink_rx.
Link: https://lore.kernel.org/lkml/[email protected]/
v1: https://lore.kernel.org/lkml/[email protected]/
Anirudh Rayabharam (2):
usbip: give back URBs for unsent unlink requests during cleanup
usbip: clean up code in vhci_device_unlink_cleanup
drivers/usb/usbip/vhci_hcd.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
--
2.26.2
The cleanup code for unlink_tx and unlink_rx lists is almost the same.
So, extract it into a new function and call it for both unlink_rx and
unlink_tx. Also, remove unnecessary log messages.
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
drivers/usb/usbip/vhci_hcd.c | 52 +++++++++---------------------------
1 file changed, 12 insertions(+), 40 deletions(-)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 190bd3d1c1f0..b5b31a1d40b6 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -945,7 +945,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
return 0;
}
-static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
+static void vhci_cleanup_unlink_list(struct vhci_device *vdev,
+ struct list_head *unlink_list)
{
struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
struct usb_hcd *hcd = vhci_hcd_to_hcd(vhci_hcd);
@@ -956,12 +957,9 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
spin_lock_irqsave(&vhci->lock, flags);
spin_lock(&vdev->priv_lock);
- list_for_each_entry_safe(unlink, tmp, &vdev->unlink_tx, list) {
+ list_for_each_entry_safe(unlink, tmp, unlink_list, list) {
struct urb *urb;
- /* give back urb of unsent unlink request */
- pr_info("unlink cleanup tx %lu\n", unlink->unlink_seqnum);
-
urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
if (!urb) {
list_del(&unlink->list);
@@ -986,45 +984,19 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
kfree(unlink);
}
- while (!list_empty(&vdev->unlink_rx)) {
- struct urb *urb;
-
- unlink = list_first_entry(&vdev->unlink_rx, struct vhci_unlink,
- list);
-
- /* give back URB of unanswered unlink request */
- pr_info("unlink cleanup rx %lu\n", unlink->unlink_seqnum);
-
- urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
- if (!urb) {
- pr_info("the urb (seqnum %lu) was already given back\n",
- unlink->unlink_seqnum);
- list_del(&unlink->list);
- kfree(unlink);
- continue;
- }
-
- urb->status = -ENODEV;
-
- usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- list_del(&unlink->list);
-
- spin_unlock(&vdev->priv_lock);
- spin_unlock_irqrestore(&vhci->lock, flags);
-
- usb_hcd_giveback_urb(hcd, urb, urb->status);
-
- spin_lock_irqsave(&vhci->lock, flags);
- spin_lock(&vdev->priv_lock);
-
- kfree(unlink);
- }
-
spin_unlock(&vdev->priv_lock);
spin_unlock_irqrestore(&vhci->lock, flags);
}
+static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
+{
+ /* give back URB of unsent unlink request */
+ vhci_cleanup_unlink_list(vdev, &vdev->unlink_tx);
+
+ /* give back URB of unanswered unlink request */
+ vhci_cleanup_unlink_list(vdev, &vdev->unlink_rx);
+}
+
/*
* The important thing is that only one context begins cleanup.
* This is why error handling and cleanup become simple.
--
2.26.2
In vhci_device_unlink_cleanup(), the URBs for unsent unlink requests are
not given back. This sometimes causes usb_kill_urb to wait indefinitely
for that urb to be given back. syzbot has reported a hung task issue [1]
for this.
To fix this, give back the urbs corresponding to unsent unlink requests
(unlink_tx list) similar to how urbs corresponding to unanswered unlink
requests (unlink_rx list) are given back.
[1]: https://syzkaller.appspot.com/bug?id=08f12df95ae7da69814e64eb5515d5a85ed06b76
Reported-by: [email protected]
Tested-by: [email protected]
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
drivers/usb/usbip/vhci_hcd.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 4ba6bcdaa8e9..190bd3d1c1f0 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -957,8 +957,32 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
spin_lock(&vdev->priv_lock);
list_for_each_entry_safe(unlink, tmp, &vdev->unlink_tx, list) {
+ struct urb *urb;
+
+ /* give back urb of unsent unlink request */
pr_info("unlink cleanup tx %lu\n", unlink->unlink_seqnum);
+
+ urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
+ if (!urb) {
+ list_del(&unlink->list);
+ kfree(unlink);
+ continue;
+ }
+
+ urb->status = -ENODEV;
+
+ usb_hcd_unlink_urb_from_ep(hcd, urb);
+
list_del(&unlink->list);
+
+ spin_unlock(&vdev->priv_lock);
+ spin_unlock_irqrestore(&vhci->lock, flags);
+
+ usb_hcd_giveback_urb(hcd, urb, urb->status);
+
+ spin_lock_irqsave(&vhci->lock, flags);
+ spin_lock(&vdev->priv_lock);
+
kfree(unlink);
}
--
2.26.2
On 8/20/21 1:01 PM, Anirudh Rayabharam wrote:
> In vhci_device_unlink_cleanup(), the URBs for unsent unlink requests are
> not given back. This sometimes causes usb_kill_urb to wait indefinitely
> for that urb to be given back. syzbot has reported a hung task issue [1]
> for this.
>
> To fix this, give back the urbs corresponding to unsent unlink requests
> (unlink_tx list) similar to how urbs corresponding to unanswered unlink
> requests (unlink_rx list) are given back.
>
> [1]: https://syzkaller.appspot.com/bug?id=08f12df95ae7da69814e64eb5515d5a85ed06b76
>
> Reported-by: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Anirudh Rayabharam <[email protected]>
> ---
> drivers/usb/usbip/vhci_hcd.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 4ba6bcdaa8e9..190bd3d1c1f0 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -957,8 +957,32 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
> spin_lock(&vdev->priv_lock);
>
> list_for_each_entry_safe(unlink, tmp, &vdev->unlink_tx, list) {
> + struct urb *urb;
> +
> + /* give back urb of unsent unlink request */
> pr_info("unlink cleanup tx %lu\n", unlink->unlink_seqnum);
> +
> + urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
> + if (!urb) {
> + list_del(&unlink->list);
> + kfree(unlink);
> + continue;
> + }
> +
> + urb->status = -ENODEV;
> +
> + usb_hcd_unlink_urb_from_ep(hcd, urb);
> +
> list_del(&unlink->list);
> +
> + spin_unlock(&vdev->priv_lock);
> + spin_unlock_irqrestore(&vhci->lock, flags);
> +
> + usb_hcd_giveback_urb(hcd, urb, urb->status);
> +
> + spin_lock_irqsave(&vhci->lock, flags);
> + spin_lock(&vdev->priv_lock);
> +
> kfree(unlink);
> }
>
>
Looks good.
Reviewed-by: Shuah Khan <[email protected]>
thanks,
-- Shuah