[PATCH v3 1/2] usb: gadget: u_serial: serialise line setup against line teardown

[Varun Singh <varun.singh10011@gmail.com>]

Creating a u_serial gadget function can fail with -EEXIST for no reason:

  sysfs: cannot create duplicate filename '/devices/virtual/tty/ttyGS0'
  gserial_alloc_line_no_console: failed to register tty for port 0, err -17

ports[n].lock protects the ports[n].port slot, but the line's tty device
is registered and unregistered outside it, so the two can change
independently:

  gserial_alloc_line_no_console()        gserial_free_line()
    gs_port_alloc()
      mutex_lock(ports[n].lock)
                                           mutex_lock(ports[n].lock)
                                           ports[n].port = NULL
                                           mutex_unlock(ports[n].lock)
      ports[n].port = new
      mutex_unlock(ports[n].lock)
    tty_port_register_device(n)            <- device for n still there
                                           tty_unregister_device(n)

The allocator claims a slot whose old device is still registered, which
is the -EEXIST above. The reverse order leaves a slot with no device
node.

Hold ports[n].lock across slot assignment and device registration, and
across slot clearing and unregistration, so a concurrent caller sees
either both or neither. gs_port_alloc() is now called with the lock
held. Neither tty_port_register_device() nor tty_unregister_device()
takes tty_lock or ports[n].lock, and gserial_free_line() already slept
under this mutex, so this adds no new lock dependency.

With two gadgets racing for line 0, 25 of 383 function creations failed
on an unpatched kernel; 0 of 530 with this patch.

Found with the assistance of Claude Opus 5.

Signed-off-by: Varun Singh <varun.singh10011@gmail.com>
---
 drivers/usb/gadget/function/u_serial.c | 54 ++++++++++++++------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index cdd1dfc666c4..e172050f1392 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1203,23 +1203,18 @@ static void gs_console_exit(struct gs_port *port)
 
 #endif
 
+/* Caller must hold ports[port_num].lock. */
 static int
 gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 {
 	struct gs_port	*port;
-	int		ret = 0;
 
-	mutex_lock(&ports[port_num].lock);
-	if (ports[port_num].port) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if (ports[port_num].port)
+		return -EBUSY;
 
 	port = kzalloc_obj(struct gs_port);
-	if (port == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!port)
+		return -ENOMEM;
 
 	tty_port_init(&port->port);
 	spin_lock_init(&port->port_lock);
@@ -1236,9 +1231,8 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
 	port->port_line_coding = *coding;
 
 	ports[port_num].port = port;
-out:
-	mutex_unlock(&ports[port_num].lock);
-	return ret;
+
+	return 0;
 }
 
 static int gs_closed(struct gs_port *port)
@@ -1267,17 +1261,23 @@ void gserial_free_line(unsigned char port_num)
 	struct gs_port	*port;
 
 	mutex_lock(&ports[port_num].lock);
-	if (!ports[port_num].port) {
+	port = ports[port_num].port;
+	if (!port) {
 		mutex_unlock(&ports[port_num].lock);
 		return;
 	}
-	port = ports[port_num].port;
 	gs_console_exit(port);
 	ports[port_num].port = NULL;
+
+	/*
+	 * Unregister under the lock, so the device cannot outlive its slot:
+	 * otherwise a concurrent gserial_alloc_line() could claim the line
+	 * and register a device that this unregister would tear down.
+	 */
+	tty_unregister_device(gs_tty_driver, port_num);
 	mutex_unlock(&ports[port_num].lock);
 
 	gserial_free_port(port);
-	tty_unregister_device(gs_tty_driver, port_num);
 }
 EXPORT_SYMBOL_GPL(gserial_free_line);
 
@@ -1294,13 +1294,18 @@ int gserial_alloc_line_no_console(unsigned char *line_num)
 	coding.bParityType = USB_CDC_NO_PARITY;
 	coding.bDataBits = USB_CDC_1_STOP_BITS;
 
+	/*
+	 * Hold the line's lock across both, so a concurrent
+	 * gserial_free_line() sees the slot and the device together.
+	 */
 	for (port_num = 0; port_num < MAX_U_SERIAL_PORTS; port_num++) {
+		mutex_lock(&ports[port_num].lock);
 		ret = gs_port_alloc(port_num, &coding);
-		if (ret == -EBUSY)
-			continue;
-		if (ret)
+		if (!ret)
+			break;
+		mutex_unlock(&ports[port_num].lock);
+		if (ret != -EBUSY)
 			return ret;
-		break;
 	}
 	if (ret)
 		return ret;
@@ -1315,15 +1320,16 @@ int gserial_alloc_line_no_console(unsigned char *line_num)
 				__func__, port_num, PTR_ERR(tty_dev));
 
 		ret = PTR_ERR(tty_dev);
-		mutex_lock(&ports[port_num].lock);
 		ports[port_num].port = NULL;
 		mutex_unlock(&ports[port_num].lock);
 		gserial_free_port(port);
-		goto err;
+		return ret;
 	}
+	mutex_unlock(&ports[port_num].lock);
+
 	*line_num = port_num;
-err:
-	return ret;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(gserial_alloc_line_no_console);
 
-- 
2.34.1