Re: [PATCH 2/2] tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] tty: serial: fsl_lpuart: race scheduling dma_tx_flush_work after flush_work()
- [High] tty: serial: fsl_lpuart: lpuart_dma_tx_flush_work() restarts DMA on teardown
--

commit 45f7dcf3d930bb1e96c498ca260fc93b635e30f0
Author: Samarth Narang <samarth.colleges@gmail.com>

tty: serial: fsl_lpuart: don't unmap TX buffer before DMA has stopped

This commit defers TX DMA scatterlist unmapping from lpuart_flush_buffer() to
a work item that runs dmaengine_synchronize() before releasing the mapping,
and flushes that work item in lpuart_dma_shutdown() and lpuart_suspend().

> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index c8575c9652035..8343351199eb5 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c

[ ... ]

> @@ -558,6 +561,36 @@ static void lpuart_dma_tx_complete(void *arg)
>  	uart_port_unlock_irqrestore(&sport->port, flags);
>  }
>  
> +/*
> + * flush_buffer runs under the port lock and can only request termination.
> + * Unmap the TX scatterlist here, once the terminated transfer and its
> + * completion callback are known to have finished.
> + */
> +static void lpuart_dma_tx_flush_work(struct work_struct *work)
> +{
> +	struct lpuart_port *sport = container_of(work, struct lpuart_port,
> +						 dma_tx_flush_work);
> +	struct dma_chan *chan = sport->dma_tx_chan;
> +	unsigned long flags;
> +
> +	dmaengine_synchronize(chan);
> +
> +	uart_port_lock_irqsave(&sport->port, &flags);
> +	if (sport->dma_tx_flushing) {
> +		dma_unmap_sg(dmaengine_get_dma_device(chan), &sport->tx_sgl[0],
> +			     sport->dma_tx_nents, DMA_TO_DEVICE);
> +		sport->dma_tx_flushing = false;
> +		sport->dma_tx_in_progress = false;
> +	}
> +	/* As in the TX callback: no restart while shutdown waits on dma_wait */
> +	if (!waitqueue_active(&sport->dma_wait) &&
> +	    !lpuart_stopped_or_empty(&sport->port))
> +		lpuart_dma_tx(sport);

[Severity: High]
Can lpuart_dma_tx_flush_work() still restart TX DMA and leak the scatterlist
mapping on sport->tx_sgl during shutdown or suspend?

In lpuart_dma_shutdown(), flush_work(&sport->dma_tx_flush_work) is called
before wait_event_interruptible_timeout() adds a waiter to sport->dma_wait:

	if (sport->lpuart_dma_tx_use) {
		flush_work(&sport->dma_tx_flush_work);
		if (wait_event_interruptible_timeout(sport->dma_wait,
			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
			sport->dma_tx_in_progress = false;
			dmaengine_terminate_sync(sport->dma_tx_chan);
		}
		sport->lpuart_dma_tx_use = false;
	}

In lpuart_suspend(), flush_work(&sport->dma_tx_flush_work) is also called
with no waiter on sport->dma_wait:

		if (sport->lpuart_dma_tx_use) {
			...
			uart_port_unlock_irqrestore(&sport->port, flags);
			flush_work(&sport->dma_tx_flush_work);
			sport->dma_tx_in_progress = false;
			dmaengine_terminate_sync(sport->dma_tx_chan);
		}

Because waitqueue_active(&sport->dma_wait) is false while flush_work() runs
in both paths, if xmit_fifo is non-empty (for example, if bytes were queued
after uart_flush_buffer() called tty_port_tty_wakeup() while
dma_tx_in_progress was held true by a pending flush, or when suspending an
active wakeup-enabled port where uart_suspend_port() returns early),
lpuart_dma_tx_flush_work() will call lpuart_dma_tx(sport) and map
sport->tx_sgl after the UART transmitter and TX DMA request bits have already
been disabled in hardware.

Since hardware TX DMA is already disabled, that newly submitted transfer
cannot complete: lpuart_dma_shutdown() will wait for the 300ms timeout (or
return immediately on a pending signal) and then call
dmaengine_terminate_sync(sport->dma_tx_chan) without unmapping sport->tx_sgl
before uart_shutdown() frees xmit_buf, and lpuart_suspend() will likewise
clear sport->dma_tx_in_progress and call dmaengine_terminate_sync() without
unmapping sport->tx_sgl.

> +	uart_port_unlock_irqrestore(&sport->port, flags);
> +
> +	wake_up(&sport->dma_wait);
> +}

[ ... ]

> @@ -1760,6 +1797,8 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>  		goto err;
>  
>  	init_waitqueue_head(&sport->dma_wait);
> +	INIT_WORK(&sport->dma_tx_flush_work, lpuart_dma_tx_flush_work);
> +	sport->dma_tx_flushing = false;
>  	sport->lpuart_dma_tx_use = true;
>  	if (lpuart_is_32(sport)) {
>  		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> @@ -1926,6 +1965,7 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
>  	}
>  
>  	if (sport->lpuart_dma_tx_use) {
> +		flush_work(&sport->dma_tx_flush_work);
>  		if (wait_event_interruptible_timeout(sport->dma_wait,
>  			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
>  			sport->dma_tx_in_progress = false;
> @@ -3179,6 +3219,7 @@ static int lpuart_suspend(struct device *dev)
>  				writeb(temp, sport->port.membase + UARTCR5);
>  			}
>  			uart_port_unlock_irqrestore(&sport->port, flags);
> +			flush_work(&sport->dma_tx_flush_work);
>  			sport->dma_tx_in_progress = false;
>  			dmaengine_terminate_sync(sport->dma_tx_chan);
>  		}

[Severity: Critical]
What prevents lpuart_flush_buffer() from scheduling sport->dma_tx_flush_work
again after flush_work(&sport->dma_tx_flush_work) has already returned in
lpuart_dma_shutdown() or lpuart_suspend()?

uart_flush_buffer() only takes sport->port.lock and does not hold
state->port.mutex or check tty_port_initialized(), so it can run concurrently
via tty_ioctl(TCFLSH) -> tty_perform_flush() or line discipline signal
handling (isig() -> tty_driver_flush_buffer()):

	if (sport->lpuart_dma_tx_use) {
		dmaengine_terminate_async(chan);
		if (sport->dma_tx_in_progress && !sport->dma_tx_flushing) {
			sport->dma_tx_flushing = true;
			schedule_work(&sport->dma_tx_flush_work);
		}
	}

In lpuart_dma_shutdown(), after flush_work(&sport->dma_tx_flush_work)
returns, the task sleeps in wait_event_interruptible_timeout() without
holding sport->port.lock while sport->lpuart_dma_tx_use is still true and
(if an un-flushed TX DMA transfer was active when lpuart_shutdown() or
lpuart32_shutdown() disabled hardware TX) sport->dma_tx_in_progress is true
and sport->dma_tx_flushing is false.

If lpuart_flush_buffer() runs during that wait and schedules
sport->dma_tx_flush_work, and wait_event_interruptible_timeout() is
interrupted by a signal or times out before the work runs,
lpuart_dma_shutdown() proceeds to call
dma_release_channel(sport->dma_tx_chan) without flushing
sport->dma_tx_flush_work again, allowing lpuart_dma_tx_flush_work() to
dereference the released sport->dma_tx_chan.

Similarly, in lpuart_suspend() on an active wakeup-enabled port,
sport->lpuart_dma_tx_use remains true across suspend and
sport->dma_tx_in_progress is cleared outside sport->port.lock after
flush_work(). If sport->dma_tx_flush_work is queued after flush_work() in
either path, a subsequent open or resume calling lpuart_tx_dma_startup()
will also execute INIT_WORK(&sport->dma_tx_flush_work, ...) while the work
item is still pending or running.

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