Re: [PATCH] tty: serial: samsung: fix TX fifo underflow on flush race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: serial: samsung: race in s3c24xx_serial_flush_buffer() underflows TX fifo
--

commit f08a63c8494169298262a17372c5a89a1b95c795
Author: tianci xie <xiex16070@gmail.com>

tty: serial: samsung: fix TX fifo underflow on flush race

This commit adds a flush_buffer callback to s3c64xx_serial_ops and
apple_s5l_serial_ops to terminate active TX DMA transfers and clear
tx_bytes_requested when the transmit buffer is flushed, while moving the
tx_bytes_requested check in s3c24xx_serial_tx_dma_complete() under the port
lock.

Link: https://lore.kernel.org/all/20261031004733.3345841-1-samarth.colleges@gmail.com/

> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 640f85e2d636b..e955b7eab5f27 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -330,7 +330,6 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>  	int count;
>  
>  	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> -	count = dma->tx_bytes_requested - state.residue;
>  	async_tx_ack(dma->tx_desc);
>  
>  	dma_sync_single_for_cpu(dmaengine_get_dma_device(dma->tx_chan),
> @@ -339,6 +338,17 @@ static void s3c24xx_serial_tx_dma_complete(void *args)
>  
>  	uart_port_lock_irqsave(port, &flags);
>  
> +	/*
> +	 * uart_flush_buffer() resets the xmit fifo and then calls
> +	 * ->flush_buffer(), which clears tx_bytes_requested.  Sample the byte
> +	 * count under the port lock so that a completion which raced with a
> +	 * flush cannot advance the already reset fifo past its write index.
> +	 */
> +	if (dma->tx_bytes_requested)
> +		count = dma->tx_bytes_requested - state.residue;
> +	else
> +		count = 0;
> +
>  	uart_xmit_advance(port, count);
>  	ourport->tx_in_progress = 0;
>  
> @@ -1162,6 +1172,32 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
>  	}
>  }
>  
> +/*
> + * Called from uart_flush_buffer() with the port lock held, after the xmit fifo
> + * has already been reset.  A TX DMA transfer still in flight carries data that
> + * no longer exists, so drop its byte count before terminating it: the
> + * completion callback may still run and must not advance the reset fifo.
> + */
> +static void s3c24xx_serial_flush_buffer(struct uart_port *port)
> +{
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +	struct s3c24xx_uart_dma *dma = ourport->dma;
> +
> +	if (!dma || !dma->tx_chan || ourport->tx_in_progress != S3C24XX_TX_DMA)
> +		return;
> +
> +	dma->tx_bytes_requested = 0;
> +
> +	/*
> +	 * dmaengine_terminate_sync() cannot be used here because the port
> +	 * lock is held.  The callback may or may not run afterwards; either
> +	 * way it is a no-op for the fifo because tx_bytes_requested is zero.
> +	 */
> +	dmaengine_terminate_async(dma->tx_chan);
> +
> +	ourport->tx_in_progress = 0;
> +}

[Severity: High]
Can clearing ourport->tx_in_progress = 0 in s3c24xx_serial_flush_buffer()
allow a subsequent write to start a new transfer before an already-dispatched
s3c24xx_serial_tx_dma_complete() callback from the flushed transfer acquires
port->lock?

When a DMA transfer completes just as uart_flush_buffer() runs, the DMA
engine tasklet (such as pl330_tasklet()) can invoke
s3c24xx_serial_tx_dma_complete() on CPU 0 while CPU 1 holds port->lock in
uart_flush_buffer():

CPU 0                                   CPU 1
s3c24xx_serial_tx_dma_complete()        uart_flush_buffer()
  dmaengine_tx_status(...)                uart_port_lock_irqsave()
  async_tx_ack(...)                       kfifo_reset(&tport->xmit_fifo)
  dma_sync_single_for_cpu(...)            s3c24xx_serial_flush_buffer()
                                            dma->tx_bytes_requested = 0;
                                            dmaengine_terminate_async();
                                            ourport->tx_in_progress = 0;
                                          uart_port_unlock_irqrestore()

Because ourport->tx_in_progress is now 0, a new write via
uart_write() -> s3c24xx_serial_start_tx() -> s3c24xx_serial_start_next_tx()
can acquire port->lock before CPU 0's s3c24xx_serial_tx_dma_complete() does:

drivers/tty/serial/samsung_tty.c:s3c24xx_serial_start_tx() {
	...
	if (ourport->dma && ourport->dma->tx_chan) {
		if (!kfifo_is_empty(&tport->xmit_fifo) &&
				!ourport->tx_in_progress)
			s3c24xx_serial_start_next_tx(ourport);
	}
}

If the new write starts a second DMA transfer via
s3c24xx_serial_start_tx_dma(), it overwrites dma->tx_size,
dma->tx_transfer_addr, dma->tx_desc, and dma->tx_cookie (which also races
with the unlocked reads at the top of s3c24xx_serial_tx_dma_complete() if
CPU 0 has not passed them yet), sets dma->tx_bytes_requested to the new
transfer's size, and sets ourport->tx_in_progress = S3C24XX_TX_DMA.

When CPU 0's stale callback from the first transfer then acquires port->lock,
wouldn't it observe the second transfer's non-zero dma->tx_bytes_requested,
advance xmit_fifo prematurely, clear ourport->tx_in_progress = 0 while the
second DMA transfer is still in flight, and then underflow xmit_fifo when the
second transfer's callback later runs and advances xmit_fifo again?

Similarly, if the new write is smaller than ourport->min_dma_size and starts
a PIO transfer via s3c24xx_serial_start_tx_pio() -> enable_tx_pio(), setting
ourport->tx_in_progress = S3C24XX_TX_PIO and ourport->tx_mode =
S3C24XX_TX_PIO, wouldn't the stale DMA callback subsequently clear
ourport->tx_in_progress = 0 and call s3c24xx_serial_start_next_tx() ->
s3c24xx_serial_start_tx_pio(), which skips enable_tx_pio() because
ourport->tx_mode is already S3C24XX_TX_PIO, leaving ourport->tx_in_progress
as 0 while PIO is active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031152656.1422-1-xiex16070@gmail.com?part=1