On Fri, 24 Apr 2020, syzbot wrote: > Hello, > > syzbot has tested the proposed patch but the reproducer still triggered crash: > WARNING in usb_queue_reset_device > > ------------[ cut here ]------------ > usbhid 3-1:0.0: Device reset ... > hid_reset+0x219/0x3e0 drivers/hid/usbhid/hid-core.c:138 > process_one_work+0x94b/0x1620 kernel/workqueue.c:2266 > worker_thread+0x96/0xe20 kernel/workqueue.c:2412 Okay, that's weird. hid_reset is already running in a work queue thread; it has no need to queue a request for a device reset. It can just reset the device directly. Anyway, let's see where it gets called from. Alan Stern #syz test: https://github.com/google/kasan.git 0fa84af8 Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -4440,6 +4440,7 @@ void usb_ep0_reinit(struct usb_device *u usb_disable_endpoint(udev, 0 + USB_DIR_IN, true); usb_disable_endpoint(udev, 0 + USB_DIR_OUT, true); usb_enable_endpoint(udev, &udev->ep0, true); + udev->alan1 = 0; } EXPORT_SYMBOL_GPL(usb_ep0_reinit); @@ -4471,6 +4472,7 @@ static int hub_set_address(struct usb_de update_devnum(udev, devnum); /* Device now using proper address. */ usb_set_device_state(udev, USB_STATE_ADDRESS); + udev->alan1 = 1; usb_ep0_reinit(udev); } return retval; @@ -4838,6 +4840,7 @@ hub_port_init(struct usb_hub *hub, struc else dev_warn(&udev->dev, "Using ep0 maxpacket: %d\n", i); udev->ep0.desc.wMaxPacketSize = cpu_to_le16(i); + udev->alan1 = 2; usb_ep0_reinit(udev); } @@ -5226,6 +5229,7 @@ static void hub_port_connect(struct usb_ loop_disable: hub_port_disable(hub, port1, 1); loop: + udev->alan1 = 3; usb_ep0_reinit(udev); release_devnum(udev); hub_free_dev(udev); @@ -5766,6 +5770,7 @@ static int usb_reset_and_verify_device(s /* ep0 maxpacket size may change; let the HCD know about it. * Other endpoints will be handled by re-enumeration. */ + udev->alan1 = 4; usb_ep0_reinit(udev); ret = hub_port_init(parent_hub, udev, port1, i); if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV) @@ -6007,6 +6012,8 @@ EXPORT_SYMBOL_GPL(usb_reset_device); */ void usb_queue_reset_device(struct usb_interface *iface) { + dev_WARN(&iface->dev, "Device reset\n"); + if (schedule_work(&iface->reset_ws)) usb_get_intf(iface); } Index: usb-devel/drivers/usb/core/urb.c =================================================================== --- usb-devel.orig/drivers/usb/core/urb.c +++ usb-devel/drivers/usb/core/urb.c @@ -204,8 +204,12 @@ int usb_urb_ep_type_check(const struct u const struct usb_host_endpoint *ep; ep = usb_pipe_endpoint(urb->dev, urb->pipe); - if (!ep) + if (!ep) { + dev_info(&urb->dev->dev, "Ep %d disabled: %d\n", + usb_pipeendpoint(urb->pipe), + urb->dev->alan1); return -EINVAL; + } if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) return -EINVAL; return 0; Index: usb-devel/include/linux/usb.h =================================================================== --- usb-devel.orig/include/linux/usb.h +++ usb-devel/include/linux/usb.h @@ -629,6 +629,7 @@ struct usb3_lpm_parameters { * usb_set_device_state(). */ struct usb_device { + int alan1; int devnum; char devpath[16]; u32 route; Index: usb-devel/drivers/hid/usbhid/hid-core.c =================================================================== --- usb-devel.orig/drivers/hid/usbhid/hid-core.c +++ usb-devel/drivers/hid/usbhid/hid-core.c @@ -135,7 +135,7 @@ static void hid_reset(struct work_struct if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) { dev_dbg(&usbhid->intf->dev, "resetting device\n"); - usb_queue_reset_device(usbhid->intf); + usb_reset_device(interface_to_usbdev(usbhid->intf)); } } @@ -168,7 +168,7 @@ static void hid_io_error(struct hid_devi /* Retries failed, so do a port reset unless we lack bandwidth*/ if (!test_bit(HID_NO_BANDWIDTH, &usbhid->iofl) && !test_and_set_bit(HID_RESET_PENDING, &usbhid->iofl)) { - + dump_stack(); schedule_work(&usbhid->reset_work); goto done; } @@ -299,6 +299,7 @@ static void hid_irq_in(struct urb *urb) usbhid_mark_busy(usbhid); clear_bit(HID_IN_RUNNING, &usbhid->iofl); set_bit(HID_CLEAR_HALT, &usbhid->iofl); + dump_stack(); schedule_work(&usbhid->reset_work); return; case -ECONNRESET: /* unlink */ @@ -1438,8 +1439,10 @@ static void hid_restart_io(struct hid_de clear_bit(HID_SUSPENDED, &usbhid->iofl); usbhid_mark_busy(usbhid); - if (clear_halt || reset_pending) + if (clear_halt || reset_pending) { + dump_stack(); schedule_work(&usbhid->reset_work); + } usbhid->retry_delay = 0; spin_unlock_irq(&usbhid->lock);
