[PATCH 5/6] USB: serial: ftdi_sio: serialise low_latency toggle against read_bulk_callback

[Chinna Mopurigari Naveen Kumar Reddy]

Toggling the per-port low_latency sysfs attribute while the port is
actively reading or writing was found to occasionally leave the port
wedged.  The application would block on read() until restarted.

The race is between low_latency_store() and ftdi_read_bulk_callback():

  1. ftdi_read_bulk_callback() passes the top-of-function fast-path
     low_latency check (sees false) and proceeds towards the
     throttle/start-timer block.
  2. low_latency_store() runs on another CPU and sets the flag.
     Previously it only cancelled the hrtimer and cleared
     timer_active -- it did not clear USB_SERIAL_THROTTLED.
  3. ftdi_read_bulk_callback() continues past the lock, calls
     usb_serial_generic_throttle() which sets USB_SERIAL_THROTTLED,
     and arms the (now-cancelled-and-restarted) hrtimer.
  4. Every subsequent URB completion now takes the low_latency fast
     path at the top of the callback, delegates to the upstream
     generic callback which marks the URB free, sees
     USB_SERIAL_THROTTLED set, and refuses to resubmit.  The data
     stream stalls.

Close the race:

  a) low_latency_store() now publishes the new value under
     urb_defer_lock so the callback's locked re-check observes it
     with proper ordering, then cancels the hrtimer synchronously,
     then calls usb_serial_generic_unthrottle() to clear
     USB_SERIAL_THROTTLED and resubmit any free URBs.  This is done
     in both directions of the toggle -- when nothing is parked it
     is a cheap no-op submit.

  b) ftdi_read_bulk_callback() re-checks low_latency under the same
     urb_defer_lock immediately before the throttle/start-timer
     block.  On bail it calls usb_serial_generic_submit_read_urbs()
     to resubmit the URB itself, closing the brief window where the
     URB is marked free but neither the store nor anyone else has
     resubmitted.

Signed-off-by: Chinna Mopurigari Naveen Kumar Reddy <naveen.reddy@xxxxxxxxxxxx>
---
 drivers/usb/serial/ftdi_sio.c | 57 ++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 8faa073f1383..c631c6d0a1a5 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1764,20 +1764,54 @@ static ssize_t low_latency_store(struct device *dev,
 {
 	struct usb_serial_port *port = to_usb_serial_port(dev);
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	struct tty_struct *tty;
 	unsigned long flags;
 	u8 v;
 
 	if (kstrtou8(valbuf, 10, &v))
 		return -EINVAL;
 
+	/*
+	 * Toggling low_latency while the port is actively reading or
+	 * writing is racy unless we serialise carefully.  Between the
+	 * moment ftdi_read_bulk_callback() passes its top-of-function
+	 * low_latency check and the moment it sets USB_SERIAL_THROTTLED
+	 * + starts the hrtimer, this store may set low_latency = true.
+	 * If we then merely cancelled the timer (without clearing
+	 * USB_SERIAL_THROTTLED), every subsequent URB completion would
+	 * take the generic callback path, mark the URB free, observe
+	 * the throttled bit, refuse to resubmit, and silently stall
+	 * the data stream -- only an application restart would recover.
+	 *
+	 * Three things, in order, close the race:
+	 *   1. Publish low_latency under urb_defer_lock so the
+	 *      callback's re-check (in the throttle block below)
+	 *      observes the new value with proper ordering.
+	 *   2. Cancel the defer hrtimer synchronously so no late
+	 *      timer callback can race the unthrottle.
+	 *   3. Force-unthrottle the port and resubmit any free URBs.
+	 *      Done in both directions of the toggle -- when nothing is
+	 *      parked this is a cheap no-op submit; it eliminates the
+	 *      wedge case either way.
+	 */
+	spin_lock_irqsave(&priv->urb_defer_lock, flags);
 	priv->low_latency = !!v;
+	tty = priv->urb_defer_tty;
+	if (tty)
+		tty_kref_get(tty);
+	spin_unlock_irqrestore(&priv->urb_defer_lock, flags);
 
-	if (priv->low_latency) {
-		hrtimer_cancel(&priv->urb_defer_timer);
-		spin_lock_irqsave(&priv->urb_defer_lock, flags);
-		priv->urb_defer_timer_active = false;
-		spin_unlock_irqrestore(&priv->urb_defer_lock, flags);
+	hrtimer_cancel(&priv->urb_defer_timer);
+
+	spin_lock_irqsave(&priv->urb_defer_lock, flags);
+	priv->urb_defer_timer_active = false;
+	spin_unlock_irqrestore(&priv->urb_defer_lock, flags);
+
+	if (tty) {
+		usb_serial_generic_unthrottle(tty);
+		tty_kref_put(tty);
 	}
+
 	return count;
 }
 static DEVICE_ATTR_RW(low_latency);
@@ -2873,6 +2907,19 @@ static void ftdi_read_bulk_callback(struct urb *urb)
 		return;
 
 	spin_lock_irqsave(&priv->urb_defer_lock, flags);
+	/*
+	 * Re-check low_latency under the lock.  Pairs with the locked
+	 * publish in low_latency_store().  If the toggle landed
+	 * between the top-of-function fast-path check and here, do
+	 * not throttle or arm the timer; resubmit the URB ourselves
+	 * so the data stream does not stall in the narrow window
+	 * before the store's unthrottle runs.
+	 */
+	if (priv->low_latency) {
+		spin_unlock_irqrestore(&priv->urb_defer_lock, flags);
+		ftdi_atomic_submit_read_urbs(port);
+		return;
+	}
 	tty = priv->urb_defer_tty;
 	if (tty) {
 		tty_kref_get(tty);
-- 
2.43.0