Re: [PATCH 6/6] USB: serial: mxuport: wait for UART transmit completion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb: serial: mxuport: missing tty_io_error check after generic wait
- [Low] usb: serial: mxuport: TOCTOU race on jiffies causes integer underflow
--

commit 4adf9d00bbc773d24e34eebaf0a7427eedbc5bef
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>
USB: serial: mxuport: wait for UART transmit completion

This patch introduces a device-specific wait_until_sent callback for the
mxuport driver. It supplements the generic queue polling with an additional
wait for remaining UART transmit stages based on the active termios baud
rate and frame size.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 9d304fb3aa02f..53ab284f82aad 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c

[ ... ]

> @@ -716,6 +722,64 @@ static bool mxuport_tx_empty(struct usb_serial_port *port)
>  	return txlen == 0;
>  }
>  
> +static void mxuport_wait_until_sent(struct tty_struct *tty, long timeout)
> +{
> +	struct usb_serial_port *port = tty->driver_data;

[ ... ]

> +	frame_bits = tty_get_frame_size(tty->termios.c_cflag);
> +	if (timeout)
> +		deadline = jiffies + timeout;
> +
> +	usb_serial_generic_wait_until_sent(tty, timeout);
> +	if (signal_pending(current))
> +		return;

[Severity: Medium]
The generic wait function usb_serial_generic_wait_until_sent() can break out
early when tty_io_error(tty) is true, such as during a device disconnect.

Because mxuport_wait_until_sent() does not check tty_io_error() or device
disconnection after the generic wait returns, does it proceed unconditionally
to sleep for the hardware drain delay?

If the device is unplugged, this could cause close() or tcsetattr() to block
pointlessly for a long duration, up to around 25 seconds per port at 50 baud.

> +
> +	if (timeout && time_after_eq(jiffies, deadline))
> +		return;
> +
> +	delay_ns = DIV_ROUND_UP_ULL((u64)frames * frame_bits * NSEC_PER_SEC,
> +				    baud);
> +	if (timeout)
> +		delay_ns = min_t(u64, delay_ns,
> +				 jiffies_to_nsecs(deadline - jiffies));

[Severity: Low]
Can the unprotected double-read of jiffies cause an integer underflow here?

If the timer tick advances jiffies past deadline exactly between the
time_after_eq(jiffies, deadline) check and the deadline - jiffies subtraction
below it, the result of the subtraction will be negative. This negative value
will underflow to a massive unsigned value.

Consequently, min_t() would fail to bound the wait to the remaining timeout,
instead selecting the full delay_ns and artificially extending the sleep
duration. Would capturing jiffies into a local variable before the check
prevent this race?

> +
> +	dev_dbg(&port->dev,
> +		"%s - waiting %llu us after transmit queue drained\n",
> +		__func__, DIV_ROUND_UP_ULL(delay_ns, NSEC_PER_USEC));

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=6