USB: gadget: dummy-hcd: Fix "task hung" problem

Commit Message

Alan Stern Oct. 16, 2024, 3:44 p.m. UTC

The syzbot fuzzer has been encountering "task hung" problems ever
since the dummy-hcd driver was changed to use hrtimers instead of
regular timers.  It turns out that the problems are caused by a subtle
difference between the timer_pending() and hrtimer_active() APIs.

The changeover blindly replaced the first by the second.  However,
timer_pending() returns True when the timer is queued but not when its
callback is running, whereas hrtimer_active() returns True when the
hrtimer is queued _or_ its callback is running.  This difference
occasionally caused dummy_urb_enqueue() to think that the callback
routine had not yet started when in fact it was almost finished.  As a
result the hrtimer was not restarted, which made it impossible for the
driver to dequeue later the URB that was just enqueued.  This caused
usb_kill_urb() to hang, and things got worse from there.

Since hrtimers have no API for telling when they are queued and the
callback isn't running, the driver must keep track of this for itself.
That's what this patch does, adding a new "timer_pending" flag and
setting or clearing it at the appropriate times.

Reported-by: [e-mail]
Closes: https://lore.kernel.org/linux-usb/[e-mail]/
Tested-by: [e-mail]
Signed-off-by: Alan Stern <[e-mail]>
Fixes: a7f3813e589f ("usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler")
Cc: Marcello Sylvester Bauer <[e-mail]>
Cc: [e-mail]

---

I expect this will fix a lot of the bugs that syzbot has found in the 
last few months.

 drivers/usb/gadget/udc/dummy_hcd.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Greg KH Oct. 17, 2024, 6:43 a.m. UTC | #1

On Wed, Oct 16, 2024 at 11:44:45AM -0400, Alan Stern wrote:
> The syzbot fuzzer has been encountering "task hung" problems ever
> since the dummy-hcd driver was changed to use hrtimers instead of
> regular timers.  It turns out that the problems are caused by a subtle
> difference between the timer_pending() and hrtimer_active() APIs.
> 
> The changeover blindly replaced the first by the second.  However,
> timer_pending() returns True when the timer is queued but not when its
> callback is running, whereas hrtimer_active() returns True when the
> hrtimer is queued _or_ its callback is running.  This difference
> occasionally caused dummy_urb_enqueue() to think that the callback
> routine had not yet started when in fact it was almost finished.  As a
> result the hrtimer was not restarted, which made it impossible for the
> driver to dequeue later the URB that was just enqueued.  This caused
> usb_kill_urb() to hang, and things got worse from there.
> 
> Since hrtimers have no API for telling when they are queued and the
> callback isn't running, the driver must keep track of this for itself.
> That's what this patch does, adding a new "timer_pending" flag and
> setting or clearing it at the appropriate times.
> 
> Reported-by: [e-mail]
> Closes: https://lore.kernel.org/linux-usb/[e-mail]/
> Tested-by: [e-mail]
> Signed-off-by: Alan Stern <[e-mail]>
> Fixes: a7f3813e589f ("usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler")
> Cc: Marcello Sylvester Bauer <[e-mail]>
> Cc: [e-mail]
> 
> ---
> 
> I expect this will fix a lot of the bugs that syzbot has found in the 
> last few months.

Nice!  Thanks for tracking this down and fixing it.

greg k-h

Patch

Index: usb-devel/drivers/usb/gadget/udc/dummy_hcd.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/dummy_hcd.c
+++ usb-devel/drivers/usb/gadget/udc/dummy_hcd.c
@@ -254,6 +254,7 @@  struct dummy_hcd {
 	u32				stream_en_ep;
 	u8				num_stream[30 / 2];
 
+	unsigned			timer_pending:1;
 	unsigned			active:1;
 	unsigned			old_active:1;
 	unsigned			resuming:1;
@@ -1303,9 +1304,11 @@  static int dummy_urb_enqueue(
 		urb->error_count = 1;		/* mark as a new urb */
 
 	/* kick the scheduler, it'll do the rest */
-	if (!hrtimer_active(&dum_hcd->timer))
+	if (!dum_hcd->timer_pending) {
+		dum_hcd->timer_pending = 1;
 		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
 				HRTIMER_MODE_REL_SOFT);
+	}
 
  done:
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
@@ -1324,9 +1327,10 @@  static int dummy_urb_dequeue(struct usb_
 	spin_lock_irqsave(&dum_hcd->dum->lock, flags);
 
 	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
-	if (!rc && dum_hcd->rh_state != DUMMY_RH_RUNNING &&
-			!list_empty(&dum_hcd->urbp_list))
+	if (rc == 0 && !dum_hcd->timer_pending) {
+		dum_hcd->timer_pending = 1;
 		hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL_SOFT);
+	}
 
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
 	return rc;
@@ -1813,6 +1817,7 @@  static enum hrtimer_restart dummy_timer(
 
 	/* look at each urb queued by the host side driver */
 	spin_lock_irqsave(&dum->lock, flags);
+	dum_hcd->timer_pending = 0;
 
 	if (!dum_hcd->udev) {
 		dev_err(dummy_dev(dum_hcd),
@@ -1994,8 +1999,10 @@  return_urb:
 	if (list_empty(&dum_hcd->urbp_list)) {
 		usb_put_dev(dum_hcd->udev);
 		dum_hcd->udev = NULL;
-	} else if (dum_hcd->rh_state == DUMMY_RH_RUNNING) {
+	} else if (!dum_hcd->timer_pending &&
+			dum_hcd->rh_state == DUMMY_RH_RUNNING) {
 		/* want a 1 msec delay here */
+		dum_hcd->timer_pending = 1;
 		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS),
 				HRTIMER_MODE_REL_SOFT);
 	}
@@ -2390,8 +2397,10 @@  static int dummy_bus_resume(struct usb_h
 	} else {
 		dum_hcd->rh_state = DUMMY_RH_RUNNING;
 		set_link_state(dum_hcd);
-		if (!list_empty(&dum_hcd->urbp_list))
+		if (!list_empty(&dum_hcd->urbp_list)) {
+			dum_hcd->timer_pending = 1;
 			hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL_SOFT);
+		}
 		hcd->state = HC_STATE_RUNNING;
 	}
 	spin_unlock_irq(&dum_hcd->dum->lock);
@@ -2522,6 +2531,7 @@  static void dummy_stop(struct usb_hcd *h
 	struct dummy_hcd	*dum_hcd = hcd_to_dummy_hcd(hcd);
 
 	hrtimer_cancel(&dum_hcd->timer);
+	dum_hcd->timer_pending = 0;
 	device_remove_file(dummy_dev(dum_hcd), &dev_attr_urbs);
 	dev_info(dummy_dev(dum_hcd), "stopped\n");
 }