Re: [PATCH] usbip: give back URBs for unsent unlink requests during cleanup

On Fri, Aug 06, 2021 at 10:46:17PM +0530, Anirudh Rayabharam wrote:

On Fri, Aug 06, 2021 at 06:47:54PM +0200, Greg Kroah-Hartman wrote:

quoted

On Fri, Aug 06, 2021 at 10:10:14PM +0530, Anirudh Rayabharam wrote:

quoted

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. Since the code is almost the
same, extract it into a new function and call it for both unlink_rx and
unlink_tx lists.

[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 <redacted>
---
 drivers/usb/usbip/vhci_hcd.c | 47 ++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 4ba6bcdaa8e9..45f98aa12895 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);

@@ -953,23 +954,25 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
 	struct vhci_unlink *unlink, *tmp;
 	unsigned long flags;
 
+	if (unlink_list != &vdev->unlink_tx
+			&& unlink_list != &vdev->unlink_rx) {
+		pr_err("Invalid list passed to __vhci_cleanup_unlink_list\n");
+		BUG();

Do not allow the system to crash, that is not ok.

quoted

+		return;

This call makes no sense as you just rebooted the machine :(

Handle errors properly and recover from them and move on.  A single tiny
driver should not take down the whole system.

The execution can reach only if there is a developer error and they passed
some random list in `unlink_list`.

Why would a developer do that?  As that is not the case in the kernel
tree, no need to check for it.

thanks,

greg k-h