Re: [PATCH 5/5] serial: pch_uart: fix TX kfifo underflow on flush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] serial: pch_uart: unlocked sg_tx_p read and TX DMA state overwrite after stop_tx
- [High] serial: pch_uart: lock inversion and self-deadlock in pch_dma_tx_complete()
- [High] serial: pch_uart: active DMA buffer overwrite and duplicate TX after flush
--

commit 41c9c81259479647a63a193b50f8884cd2a2a063
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: pch_uart: fix TX kfifo underflow on flush

This commit adds a flush_buffer() callback to mark in-flight TX DMA
transfers as flushed and takes the port lock in pch_dma_tx_complete() so
completed flushed transfers do not advance the transmit kfifo.

> diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
> index 9505d255f09c5..122025c4a0a66 100644
> --- a/drivers/tty/serial/pch_uart.c
> +++ b/drivers/tty/serial/pch_uart.c

[ ... ]

> @@ -750,18 +751,23 @@ static void pch_dma_tx_complete(void *arg)
>  	struct eg20t_port *priv = arg;
>  	struct uart_port *port = &priv->port;
>  	struct scatterlist *sg = priv->sg_tx_p;
> +	unsigned long flags;
>  	int i;
>  
> -	for (i = 0; i < priv->nent; i++, sg++)
> -		uart_xmit_advance(port, sg_dma_len(sg));
> +	uart_port_lock_irqsave(port, &flags);

[Severity: High]
Can taking port->lock in pch_dma_tx_complete() cause a lock inversion with
pd_chan->lock as well as a single-CPU recursive deadlock on port->lock?

In pch_uart_interrupt(), the interrupt handler holds priv->port.lock while
calling dma_handle_tx(), which calls into drivers/dma/pch_dma.c:

  pch_uart_interrupt()
    uart_port_lock(&priv->port)
    dma_handle_tx()
      dmaengine_prep_slave_sg() -> pd_prep_slave_sg() -> pdc_desc_get()
        spin_lock(&pd_chan->lock)
      dmaengine_submit() -> pd_tx_submit()
        spin_lock(&pd_chan->lock)
      dma_async_issue_pending() -> pd_issue_pending()
        spin_lock(&pd_chan->lock)

This establishes the lock order priv->port.lock -> pd_chan->lock.

In drivers/dma/pch_dma.c, every path that invokes the descriptor callback
pch_dma_tx_complete() holds pd_chan->lock:

  pdc_tasklet() / pd_issue_pending() / pd_device_terminate_all()
    spin_lock(&pd_chan->lock)
    pdc_advance_work() / pdc_complete_all() / pdc_handle_error()
      pdc_chain_complete()
        dmaengine_desc_callback_invoke()
          pch_dma_tx_complete()
            uart_port_lock_irqsave(port, &flags)

On SMP, if one CPU enters pch_uart_interrupt() -> dma_handle_tx() while
another CPU runs pdc_tasklet() -> pch_dma_tx_complete(), can this deadlock
between priv->port.lock and pd_chan->lock?

In addition, in pd_tx_submit(), when pd_chan->active_list is empty,
pdc_dostart() starts the hardware DMA transfer immediately. Two lines later
in dma_handle_tx() (still holding priv->port.lock),
dma_async_issue_pending() calls pd_issue_pending():

  if (pdc_is_idle(pd_chan)) {
          spin_lock(&pd_chan->lock);
          pdc_advance_work(pd_chan);
          spin_unlock(&pd_chan->lock);
  }

If a short transfer into the UART TX FIFO has already finished by the time
pdc_is_idle() is checked, pdc_advance_work() will invoke
pch_dma_tx_complete() synchronously on the same CPU while
pch_uart_interrupt() already holds priv->port.lock.

> +	if (!priv->tx_dma_flushed)
> +		for (i = 0; i < priv->nent; i++, sg++)
> +			uart_xmit_advance(port, sg_dma_len(sg));
>  
>  	async_tx_ack(priv->desc_tx);
>  	dma_unmap_sg(port->dev, priv->sg_tx_p, priv->orig_nent, DMA_TO_DEVICE);
>  	priv->tx_dma_use = 0;
> +	priv->tx_dma_flushed = false;
>  	priv->nent = 0;
>  	priv->orig_nent = 0;
>  	kfree(priv->sg_tx_p);
>  	pch_uart_hal_enable_interrupt(priv, PCH_UART_HAL_TX_INT);
> +	uart_port_unlock_irqrestore(port, flags);
>  }
>  
>  static int handle_rx_to(struct eg20t_port *priv)
> @@ -947,6 +953,7 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
>  	}
>  	priv->orig_nent = num;
>  	priv->nent = nent;
> +	priv->tx_dma_flushed = false;

[Severity: Critical]
What happens if pch_uart_stop_tx() is called while a TX DMA transfer is in
flight, followed by a flush and a subsequent TX restart before that first
transfer completes?

In pch_dma_tx_complete(), sg is initialized from priv->sg_tx_p before
uart_port_lock_irqsave() is called:

  struct scatterlist *sg = priv->sg_tx_p;
  ...
  uart_port_lock_irqsave(port, &flags);
  if (!priv->tx_dma_flushed)
          for (i = 0; i < priv->nent; i++, sg++)
                  uart_xmit_advance(port, sg_dma_len(sg));

Meanwhile, pch_uart_stop_tx() clears priv->tx_dma_use = 0 without stopping
the in-flight DMA transfer. If TX is restarted via pch_uart_start_tx(),
dma_handle_tx() sees priv->tx_dma_use == 0 and starts a second TX DMA
transfer while the first transfer is still running:

  priv->sg_tx_p = kmalloc_objs(struct scatterlist, num, GFP_ATOMIC);
  ...
  priv->orig_nent = num;
  priv->nent = nent;
  priv->tx_dma_flushed = false;

When the first transfer's pch_dma_tx_complete() runs:

1. Because dma_handle_tx() reset priv->tx_dma_flushed to false, the flushed
first transfer still calls uart_xmit_advance() and underflows the kfifo.

2. If pch_dma_tx_complete() read sg = priv->sg_tx_p before dma_handle_tx()
replaced priv->sg_tx_p under port->lock, and the second transfer has a
larger priv->nent than the first, the loop over priv->nent reads past the
end of the first scatterlist allocation.

3. dma_unmap_sg() and kfree(priv->sg_tx_p) in the first completion will
unmap and free the second transfer's active scatterlist while the first
scatterlist is leaked, and the second completion will then unmap and kfree()
priv->sg_tx_p a second time.

>  
>  	for (i = 0; i < nent; i++, sg++) {
>  		sg->offset = tail + fifo_size * i;
> @@ -1150,6 +1157,21 @@ static void pch_uart_stop_tx(struct uart_port *port)
>  	priv->tx_dma_use = 0;
>  }
>  
> +static void pch_uart_flush_buffer(struct uart_port *port)
> +{
> +	struct eg20t_port *priv;
> +
> +	priv = container_of(port, struct eg20t_port, port);
> +
> +	/*
> +	 * The kfifo has been reset. Let the running transfer finish, so that
> +	 * its buffer is unmapped only once it has stopped, but do not advance
> +	 * the kfifo for it.
> +	 */
> +	if (priv->nent)
> +		priv->tx_dma_flushed = true;
> +}

[Severity: High]
Could leaving the TX DMA transfer running after uart_flush_buffer() resets
the kfifo cause post-flush writes to overwrite the active DMA buffer and be
transmitted twice?

When uart_flush_buffer() runs during an active TX DMA transfer:

  uart_flush_buffer()
    uart_port_lock_irqsave()
    kfifo_reset(&state->port.xmit_fifo)
    pch_uart_flush_buffer()
      priv->tx_dma_flushed = true
    uart_port_unlock_irqrestore()
    tty_port_tty_wakeup(&state->port)

Because kfifo_reset() resets xmit_fifo.in and xmit_fifo.out to offset 0 of
state->port.xmit_buf while the in-flight DMA transfer still has
state->port.xmit_buf mapped DMA_TO_DEVICE via priv->sg_tx_p, any new data
written through uart_write() or uart_put_char() before the DMA transfer
completes will write into state->port.xmit_buf starting at offset 0 while
DMA is actively reading from that buffer.

Then, if the in-flight DMA transfer reads those newly written bytes from
offset 0 and finishes, pch_dma_tx_complete() skips uart_xmit_advance()
because priv->tx_dma_flushed is set and enables PCH_UART_HAL_TX_INT, causing
dma_handle_tx() to transmit the same post-flush bytes from xmit_fifo a
second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=5