usb: hub: Fix usb enumeration issue due to address0 race

xHC hardware can only have one slot in default state with address 0 waiting for a unique address at a time, otherwise "undefined behavior may occur" according to xhci spec 5.4.3.4

The address0_mutex exists to prevent this across both xhci roothubs.

If hub_port_init() fails, it may unlock the mutex and exit with a xhci slot in default state. If the other xhci roothub calls hub_port_init() at this point we end up with two slots in default state.

Make sure the address0_mutex protects the slot default state across hub_port_init() retries, until slot is addressed or disabled.

Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init() retries in usb_reset_and_verify_device(), then it's possible the slot is still left in default state when address0_mutex is unlocked.

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

Quoted 18 lines · Mathias Nyman

xHC hardware can only have one slot in default state with address 0 waiting for a unique address at a time, otherwise "undefined behavior may occur" according to xhci spec 5.4.3.4

The address0_mutex exists to prevent this across both xhci roothubs.

If hub_port_init() fails, it may unlock the mutex and exit with a xhci slot in default state. If the other xhci roothub calls hub_port_init() at this point we end up with two slots in default state.

Make sure the address0_mutex protects the slot default state across hub_port_init() retries, until slot is addressed or disabled.

Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init() retries in usb_reset_and_verify_device(), then it's possible the slot is still left in default state when address0_mutex is unlocked.

Quoted 2 lines

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

What commit id does this "fix"?

xHC hardware can only have one slot in default state with address 0 waiting for a unique address at a time, otherwise "undefined behavior may occur" according to xhci spec 5.4.3.4

The address0_mutex exists to prevent this across both xhci roothubs.

If hub_port_init() fails, it may unlock the mutex and exit with a xhci slot in default state. If the other xhci roothub calls hub_port_init() at this point we end up with two slots in default state.

Make sure the address0_mutex protects the slot default state across hub_port_init() retries, until slot is addressed or disabled.

Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init() retries in usb_reset_and_verify_device(), then it's possible the slot is still left in default state when address0_mutex is unlocked.

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
  drivers/usb/core/hub.c | 14 +++++++++++---
  1 file changed, 11 insertions(+), 3 deletions(-)

drivers/usb/core/hub.c +10 -3

index 86658a81d284..00c3506324e4 100644
@@ -4700,8 +4700,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
  	if (oldspeed == USB_SPEED_LOW)
  		delay = HUB_LONG_RESET_TIME;
 
-	mutex_lock(hcd->address0_mutex);
-
  	/* Reset the device; full speed may morph to high speed */
  	/* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
  	retval = hub_port_reset(hub, port1, udev, delay, false);

@@ -5016,7 +5014,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
  		hub_port_disable(hub, port1, 0);
  		update_devnum(udev, devnum);	/* for disconnect processing */
  	}
-	mutex_unlock(hcd->address0_mutex);
  	return retval;
  }
 

@@ -5246,6 +5243,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  		unit_load = 100;
 
  	status = 0;
+
+	mutex_lock(hcd->address0_mutex);
+
  	for (i = 0; i < PORT_INIT_TRIES; i++) {
 
  		/* reallocate for each attempt, since references

@@ -5282,6 +5282,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  		if (status < 0)
  			goto loop;
 
+		mutex_unlock(hcd->address0_mutex);
+
  		if (udev->quirks & USB_QUIRK_DELAY_INIT)
  			msleep(2000);
 

@@ -5370,6 +5372,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 
  loop_disable:
  		hub_port_disable(hub, port1, 1);
+		mutex_lock(hcd->address0_mutex);
  loop:
  		usb_ep0_reinit(udev);
  		release_devnum(udev);

@@ -5396,6 +5399,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  	}
 
  done:
+	mutex_unlock(hcd->address0_mutex);
+
  	hub_port_disable(hub, port1, 1);
  	if (hcd->driver->relinquish_port && !hub->hdev->parent) {
  		if (status != -ENOTCONN && status != -ENODEV)

@@ -5915,6 +5920,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
  	bos = udev->bos;
  	udev->bos = NULL;
 
+	mutex_lock(hcd->address0_mutex);
+
  	for (i = 0; i < PORT_INIT_TRIES; ++i) {
 
  		/* ep0 maxpacket size may change; let the HCD know about it.

Hi,

Quoted 13 lines · Mathias Nyman

xHC hardware can only have one slot in default state with address 0 waiting for a unique address at a time, otherwise "undefined behavior may occur" according to xhci spec 5.4.3.4

The address0_mutex exists to prevent this across both xhci roothubs.

If hub_port_init() fails, it may unlock the mutex and exit with a xhci slot in default state. If the other xhci roothub calls hub_port_init() at this point we end up with two slots in default state.

Make sure the address0_mutex protects the slot default state across hub_port_init() retries, until slot is addressed or disabled.

Quoted 7 lines

Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init() retries in usb_reset_and_verify_device(), then it's possible the slot is still left in default state when address0_mutex is unlocked.

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb:
hub: Fix usb enumeration issue due to address0 race"). On my test systems it triggers the following deplock warning during system suspend/resume cycle:

======================================================
WARNING: possible circular locking dependency detected 5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
------------------------------------------------------
kworker/u16:8/738 is trying to acquire lock:
cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at:
usb_reset_and_verify_device+0xe8/0x3e4

but task is already holding lock:
cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
usb_port_resume+0xa0/0x7e8

Quoted 37 lines · Marek Szyprowski

Hi,

Quoted 13 lines · Mathias Nyman

xHC hardware can only have one slot in default state with address 0 waiting for a unique address at a time, otherwise "undefined behavior may occur" according to xhci spec 5.4.3.4

The address0_mutex exists to prevent this across both xhci roothubs.

If hub_port_init() fails, it may unlock the mutex and exit with a xhci slot in default state. If the other xhci roothub calls hub_port_init() at this point we end up with two slots in default state.

Make sure the address0_mutex protects the slot default state across hub_port_init() retries, until slot is addressed or disabled.

Quoted 7 lines

Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init() retries in usb_reset_and_verify_device(), then it's possible the slot is still left in default state when address0_mutex is unlocked.

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb:
hub: Fix usb enumeration issue due to address0 race"). On my test systems it triggers the following deplock warning during system suspend/resume cycle:

======================================================
WARNING: possible circular locking dependency detected 5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
------------------------------------------------------
kworker/u16:8/738 is trying to acquire lock:
cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at:
usb_reset_and_verify_device+0xe8/0x3e4

Quoted 4 lines

but task is already holding lock:
cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
usb_port_resume+0xa0/0x7e8

Thanks, I see it now.

Lock order is:
mutex_lock(&port_dev->status_lock)

    mutex_lock(hcd->address0_mutex)
    mutex_unlock(hcd->address0_mutex)

mutex_unlock(&port_dev->status_lock)
in hub_port_connect(), usb_port_resume() and usb_reset_device()

But patch changed the status_lock and address0_mutex lock order in hub_port_connect(). Lets see if we can take the status_lock a bit earlier in hub_port_connect() to solve this.

Fix the circular lock dependency and unbalanced unlock of addess0_mutex introduced when fixing an address0_mutex enumeration retry race in commit ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Make sure locking order between port_dev->status_lock and address0_mutex is correct, and that address0_mutex is not unlocked in hub_port_connect "done:" codepath which may be reached without locking address0_mutex

Fixes 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
  drivers/usb/core/hub.c | 20 ++++++++++++--------
  1 file changed, 12 insertions(+), 8 deletions(-)

drivers/usb/core/hub.c +12 -6

index 00c3506324e4..00070a8a6507 100644
@@ -5188,6 +5188,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  	struct usb_port *port_dev = hub->ports[port1 - 1];
  	struct usb_device *udev = port_dev->child;
  	static int unreliable_port = -1;
+	bool retry_locked;
 
  	/* Disconnect any existing devices under this port */
  	if (udev) {

@@ -5244,10 +5245,10 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 
  	status = 0;
 
-	mutex_lock(hcd->address0_mutex);
-
  	for (i = 0; i < PORT_INIT_TRIES; i++) {
-
+		usb_lock_port(port_dev);
+		mutex_lock(hcd->address0_mutex);
+		retry_locked = true;
  		/* reallocate for each attempt, since references
  		 * to the previous one can escape in various ways
  		 */

@@ -5255,6 +5256,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  		if (!udev) {
  			dev_err(&port_dev->dev,
  					"couldn't allocate usb_device\n");
+			mutex_unlock(hcd->address0_mutex);
+			usb_unlock_port(port_dev);
  			goto done;
  		}
 

@@ -5276,13 +5279,13 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
  		}
 
  		/* reset (non-USB 3.0 devices) and get descriptor */
-		usb_lock_port(port_dev);
  		status = hub_port_init(hub, udev, port1, i);
-		usb_unlock_port(port_dev);
  		if (status < 0)
  			goto loop;
 
  		mutex_unlock(hcd->address0_mutex);
+		usb_unlock_port(port_dev);
+		retry_locked = false;
 
  		if (udev->quirks & USB_QUIRK_DELAY_INIT)
  			msleep(2000);

@@ -5372,11 +5375,14 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 
  loop_disable:
  		hub_port_disable(hub, port1, 1);
-		mutex_lock(hcd->address0_mutex);
  loop:
  		usb_ep0_reinit(udev);
  		release_devnum(udev);
  		hub_free_dev(udev);
+		if (retry_locked) {
+			mutex_unlock(hcd->address0_mutex);
+			usb_unlock_port(port_dev);
+		}
  		usb_put_dev(udev);
  		if ((status == -ENOTCONN) || (status == -ENOTSUPP))
  			break;

Hi,

Quoted 8 lines · Mathias Nyman

Fix the circular lock dependency and unbalanced unlock of addess0_mutex introduced when fixing an address0_mutex enumeration retry race in commit ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Make sure locking order between port_dev->status_lock and address0_mutex is correct, and that address0_mutex is not unlocked in hub_port_connect "done:" codepath which may be reached without locking address0_mutex

Quoted 3 lines

Fixes 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

This fixes the issue I've reported here:
https://lore.kernel.org/all/[email protected]/

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Fix the circular lock dependency and unbalanced unlock of addess0_mutex introduced when fixing an address0_mutex enumeration retry race in commit ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Make sure locking order between port_dev->status_lock and address0_mutex is correct, and that address0_mutex is not unlocked in hub_port_connect "done:" codepath which may be reached without locking address0_mutex

Fixes 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/core/hub.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

drivers/usb/core/hub.c +12 -8

index 00c3506324e4..00070a8a6507 100644
@@ -5188,6 +5188,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 	struct usb_port *port_dev = hub->ports[port1 - 1];
 	struct usb_device *udev = port_dev->child;
 	static int unreliable_port = -1;
+	bool retry_locked;
 
 	/* Disconnect any existing devices under this port */
 	if (udev) {

@@ -5244,10 +5245,10 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 
 	status = 0;
 
-	mutex_lock(hcd->address0_mutex);
-
 	for (i = 0; i < PORT_INIT_TRIES; i++) {
-
+		usb_lock_port(port_dev);
+		mutex_lock(hcd->address0_mutex);
+		retry_locked = true;
 		/* reallocate for each attempt, since references
 		 * to the previous one can escape in various ways
 		 */

@@ -5255,6 +5256,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 		if (!udev) {
 			dev_err(&port_dev->dev,
 					"couldn't allocate usb_device\n");
+			mutex_unlock(hcd->address0_mutex);
+			usb_unlock_port(port_dev);
 			goto done;
 		}
 

@@ -5276,13 +5279,13 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 		}
 
 		/* reset (non-USB 3.0 devices) and get descriptor */
-		usb_lock_port(port_dev);
 		status = hub_port_init(hub, udev, port1, i);
-		usb_unlock_port(port_dev);
 		if (status < 0)
 			goto loop;
 
 		mutex_unlock(hcd->address0_mutex);
+		usb_unlock_port(port_dev);
+		retry_locked = false;
 
 		if (udev->quirks & USB_QUIRK_DELAY_INIT)
 			msleep(2000);

@@ -5372,11 +5375,14 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 
 loop_disable:
 		hub_port_disable(hub, port1, 1);
-		mutex_lock(hcd->address0_mutex);
 loop:
 		usb_ep0_reinit(udev);
 		release_devnum(udev);
 		hub_free_dev(udev);
+		if (retry_locked) {
+			mutex_unlock(hcd->address0_mutex);
+			usb_unlock_port(port_dev);
+		}
 		usb_put_dev(udev);
 		if ((status == -ENOTCONN) || (status == -ENOTSUPP))
 			break;

@@ -5399,8 +5405,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
 	}
 
 done:
-	mutex_unlock(hcd->address0_mutex);
-
 	hub_port_disable(hub, port1, 1);
 	if (hcd->driver->relinquish_port && !hub->hdev->parent) {
 		if (status != -ENOTCONN && status != -ENODEV)

Hi,

Quoted 11 lines · Mathias Nyman

Fix the circular lock dependency and unbalanced unlock of addess0_mutex introduced when fixing an address0_mutex enumeration retry race in commit ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Make sure locking order between port_dev->status_lock and address0_mutex is correct, and that address0_mutex is not unlocked in hub_port_connect "done:" codepath which may be reached without locking address0_mutex

Fixes 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")

Cc <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

Oh, this is great, with this patch I can finally hot-plug my thunderbolt dock (and thus a XHCI controller) without the XHCI controller given a whole bunch of weird errors (and some USB devices not working), which it does not when already connected at boot.

I also tried the hotplug thingy with the previous fix without this locking fix and then I actually hit the deadlock and things like lsusb would hang.

If we can get these 2 fixes together merged soon and also backported to the stable series that would be great:

Acked-by Hans de Goede <[email protected]>

Tested-by Hans de Goede <[email protected]>

Regards,

Hans