mbox series

[v4,0/2] Fix syzkaller bug: hung task in hub_port_init

Message ID 20210820190122.16379-1-mail@anirudhrb.com
Headers show
Series Fix syzkaller bug: hung task in hub_port_init | expand

Message

Anirudh Rayabharam Aug. 20, 2021, 7:01 p.m. UTC
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/20210813182508.28127-1-mail@anirudhrb.com/

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/20210806181335.2078-1-mail@anirudhrb.com/

v1: https://lore.kernel.org/lkml/20210806164015.25263-1-mail@anirudhrb.com/

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(-)

Comments

Shuah Khan Aug. 23, 2021, 10:20 p.m. UTC | #1
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: syzbot+74d6ef051d3d2eacf428@syzkaller.appspotmail.com

> Tested-by: syzbot+74d6ef051d3d2eacf428@syzkaller.appspotmail.com

> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>

> ---

>   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 <skhan@linuxfoundation.org>


thanks,
-- Shuah