Re: [syzbot] [usb?] general protection fault in usb_gadget_udc_reset (4)

[Alan Stern]

On Thu, Mar 12, 2026 at 07:42:03PM -0700, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> general protection fault in usb_gadget_udc_reset
> 
> raw-gadget.1 gadget.0: Dummy disconnect 0 reset 16, ints_enabled 1
> raw-gadget.1 gadget.0: Inc usage: 1 X
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000008: 0000 [#1] SMP KASAN PTI

I believe I have found the problem.  The code in question in dummy_hcd.c 
does this:

		dev_info(&dum->gadget.dev, "Dummy disconnect %d reset %d, ints_enabled %d\n", disconnect, reset, dum->ints_enabled);

		/* Report reset and disconnect events to the driver */
		if (dum->ints_enabled && (disconnect || reset)) {
			stop_activity(dum);
			++dum->callback_usage;
			dev_info(&dum->gadget.dev, "Inc usage: %d X\n", dum->callback_usage);
			spin_unlock(&dum->lock);

The issue is that dum->callback_usage needs to be incremented after the 
dum->ints_enabled check and before the dum->lock spinlock is released, 
but stop_activity() drops the spinlock temporarily while giving back 
cancelled requests.

Let's interchange the increment and the stop_activity() call.  That's 
the only change here from the prior test patch.

Alan Stern

#syz test: upstream 651690480a96

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
@@ -460,21 +460,27 @@ static void set_link_state(struct dummy_
 		unsigned int reset = USB_PORT_STAT_RESET &
 				(~dum_hcd->old_status) & dum_hcd->port_status;
 
+		dev_info(&dum->gadget.dev, "Dummy disconnect %d reset %d, ints_enabled %d\n", disconnect, reset, dum->ints_enabled);
+
 		/* Report reset and disconnect events to the driver */
 		if (dum->ints_enabled && (disconnect || reset)) {
-			stop_activity(dum);
 			++dum->callback_usage;
+			dev_info(&dum->gadget.dev, "Inc usage: %d X\n", dum->callback_usage);
+			stop_activity(dum);
 			spin_unlock(&dum->lock);
+			udelay(1000);
 			if (reset)
 				usb_gadget_udc_reset(&dum->gadget, dum->driver);
 			else
 				dum->driver->disconnect(&dum->gadget);
 			spin_lock(&dum->lock);
 			--dum->callback_usage;
+			dev_info(&dum->gadget.dev, "Dec usage: %d X\n", dum->callback_usage);
 		}
 	} else if (dum_hcd->active != dum_hcd->old_active &&
 			dum->ints_enabled) {
 		++dum->callback_usage;
+		dev_info(&dum->gadget.dev, "Inc usage: %d Y\n", dum->callback_usage);
 		spin_unlock(&dum->lock);
 		if (dum_hcd->old_active && dum->driver->suspend)
 			dum->driver->suspend(&dum->gadget);
@@ -482,6 +488,7 @@ static void set_link_state(struct dummy_
 			dum->driver->resume(&dum->gadget);
 		spin_lock(&dum->lock);
 		--dum->callback_usage;
+		dev_info(&dum->gadget.dev, "Dec usage: %d Y\n", dum->callback_usage);
 	}
 
 	dum_hcd->old_status = dum_hcd->port_status;
@@ -908,21 +915,6 @@ static int dummy_pullup(struct usb_gadge
 	spin_lock_irqsave(&dum->lock, flags);
 	dum->pullup = (value != 0);
 	set_link_state(dum_hcd);
-	if (value == 0) {
-		/*
-		 * Emulate synchronize_irq(): wait for callbacks to finish.
-		 * This seems to be the best place to emulate the call to
-		 * synchronize_irq() that's in usb_gadget_remove_driver().
-		 * Doing it in dummy_udc_stop() would be too late since it
-		 * is called after the unbind callback and unbind shouldn't
-		 * be invoked until all the other callbacks are finished.
-		 */
-		while (dum->callback_usage > 0) {
-			spin_unlock_irqrestore(&dum->lock, flags);
-			usleep_range(1000, 2000);
-			spin_lock_irqsave(&dum->lock, flags);
-		}
-	}
 	spin_unlock_irqrestore(&dum->lock, flags);
 
 	usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd));
@@ -945,6 +937,27 @@ static void dummy_udc_async_callbacks(st
 
 	spin_lock_irq(&dum->lock);
 	dum->ints_enabled = enable;
+	dev_info(&_gadget->dev, "Dummy ints_enabled <- %d A\n", dum->ints_enabled);
+	if (!enable) {
+		dev_info(&dum->gadget.dev, "Wait usage %d\n", dum->callback_usage);
+		/*
+		 * Emulate synchronize_irq(): wait for callbacks to finish.
+		 * This seems to be the best place to emulate the call to
+		 * synchronize_irq() that's in usb_gadget_remove_driver().
+		 * It has to come after dum->ints_enabled is clear.  But
+		 * doing it in dummy_udc_stop() would be too late since that
+		 * routine is called after the unbind callback, and unbind
+		 * shouldn't be invoked until all the other callbacks are
+		 * finished.
+		 */
+		while (dum->callback_usage > 0) {
+			spin_unlock_irq(&dum->lock);
+			usleep_range(1000, 2000);
+			spin_lock_irq(&dum->lock);
+		}
+		if (dum->ints_enabled)
+			dev_info(&_gadget->dev, "Dummy ints_enabled = %d !\n", dum->ints_enabled);
+	}
 	spin_unlock_irq(&dum->lock);
 }
 
@@ -1020,6 +1033,8 @@ static int dummy_udc_start(struct usb_ga
 	spin_lock_irq(&dum->lock);
 	dum->devstatus = 0;
 	dum->driver = driver;
+	dev_info(&dum->gadget.dev, "Start on bus %d\n",
+			dummy_hcd_to_hcd(dum_hcd)->self.busnum);
 	spin_unlock_irq(&dum->lock);
 
 	return 0;
@@ -1032,6 +1047,7 @@ static int dummy_udc_stop(struct usb_gad
 
 	spin_lock_irq(&dum->lock);
 	dum->ints_enabled = 0;
+	dev_info(&g->dev, "Dummy ints_enabled <- %d B\n", dum->ints_enabled);
 	stop_activity(dum);
 	dum->driver = NULL;
 	spin_unlock_irq(&dum->lock);
@@ -1923,11 +1939,13 @@ restart:
 			 */
 			if (value > 0) {
 				++dum->callback_usage;
+				dev_info(&dum->gadget.dev, "Inc usage: %d Z\n", dum->callback_usage);
 				spin_unlock(&dum->lock);
 				value = dum->driver->setup(&dum->gadget,
 						&setup);
 				spin_lock(&dum->lock);
 				--dum->callback_usage;
+				dev_info(&dum->gadget.dev, "Dec usage: %d Z\n", dum->callback_usage);
 
 				if (value >= 0) {
 					/* no delays (max 64KB data stage) */