[PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails

[James Hilliard]

On Thu, Oct 29, 2026 at 4:27 PM Raul E Rangel <rrangel at chromium.org> wrote:
>
> On Thu, Oct 29, 2026 at 11:19 AM James Hilliard <james.hilliard1 at gmail.com> wrote:
> > On Thu, Oct 29, 2026 at 8:24 AM Andy Shevchenko
> > <andriy.shevchenko at linux.intel.com> wrote:
> > > > + while (rx_ret || tx_ret) {
> > > > +         dev_err_ratelimited(p->port.dev,
> > > > > +                                   "DMA termination failed: RX %d, TX %d; retrying\n",
> > > > > +                                   rx_ret, tx_ret);
> > > > > +               msleep(100);
> > > > > +               if (rx_ret)
> > > > > +                       rx_ret = dmaengine_terminate_sync(dma->rxchan);
> > > > > +               if (tx_ret)
> > > > > +                       tx_ret = dmaengine_terminate_sync(dma->txchan);
> > > > > +       }
> > > >
> > > > Huh?!
> > > >
> > > > This is unacceptable. Sleeping is okay, but value should really match the
> > > > baudspeed and other parameters of the transfer.
> > ...
> > Regardless of the retry interval, shutdown cannot release the buffers
> > until the provider establishes that DMA and its callbacks have stopped.
>
> Agreed that if dmaengine_terminate_sync() fails, shutdown must not free
> dma->rx_buf, unmap dma->tx_addr, or kfree(port->xmit_buf) while hardware
> or a detached callback may still touch them.
>
> However, because serial8250_release_dma() runs under port->mutex in
> uart_shutdown(), uart_tty_port_shutdown(), and uart_suspend_port(), an
> unbounded `while (rx_ret || tx_ret) { msleep(100); ... }` loop means that
> if a DMA provider returns a persistent error from dmaengine_terminate_sync()
> (-ENOSYS, -EINVAL, -ENODEV, -ENXIO, -ETIMEDOUT, or a wedged controller),
> close(), hangup(), and system suspend will hang forever in
> TASK_UNINTERRUPTIBLE while holding port->mutex (triggering hung_task_panic
> after 120 seconds).

That is the current behavior. Returning from shutdown allows serial
core to free the TX buffer and removal to release the callback
context. The retry loop prevents those releases, but a permanent
termination failure blocks teardown indefinitely

>
> Would it make sense to satisfy both constraints (no UAF on buffers/callbacks,
> and no infinite sleep in close()/suspend()) by quarantining/leaking only the
> failed channel's resources instead of looping forever?

Retaining the buffers and channels does not preserve the complete
callback context.

The callbacks carry a uart_8250_port pointer. For DW, the DMA state is
embedded in devm-managed private data, which can be freed after
unbind. The registered 8250 port slot can also be reused. Retaining
the DMA channel does not prevent either operation.

> Specifically:
> 1. Bound the retry loop in serial8250_terminate_dma() (or do not loop on
>    non-transient errors, checking `if (dma->rxchan)` / `if (dma->txchan)`).
> 2. If rx_ret or tx_ret is still non-zero, warn once (`dev_err` / `WARN_ON_ONCE`)
>    and intentionally quarantine/leak only the failed channel's resources:
>    - If rx_ret != 0, leave dma->rxchan non-NULL, keep dma->shutting_down = true,
>      and skip dma_free_coherent() / dma_release_channel(dma->rxchan).
>    - If tx_ret != 0, leave dma->txchan non-NULL, keep dma->shutting_down = true
>      and state->xmit_blocked = true, skip dma_unmap_single() /
>      uart_xmit_unblock() / dma_release_channel(dma->txchan), and in
>      serial_core.c:uart_free_xmit_buf(), skip `kfree(xmit_buf)` if
>      `state->xmit_blocked` is still set.
> 3. For a subsequent port reopen after a quarantined failure:
>    - Remove the redundant `up->dma->shutting_down = false;` in
>      serial8250_do_startup() before serial8250_request_dma(up) (which already
>      clears shutting_down under port->lock), and have serial8250_request_dma()
>      return -EBUSY if `dma->rxchan || dma->txchan` is still non-NULL so
>      reopen falls back cleanly to PIO (`up->dma = NULL`).

Clearing up->dma does not stop hardware accesses. After unsuccessful
termination, the channel may still access the UART FIFO, so reopening
with PIO can introduce concurrent FIFO access.

A hardware-specific stop/reset sequence could establish quiescence,
but its implementation and effects on other channels depend on the
controller. If the channel cannot be stopped, quarantine also needs
to preserve callback-context lifetimes and prevent unsafe port reuse.

>    - Guard the DMA completion callbacks (`__dma_tx_complete`, `dma_rx_complete`,
>      `omap_8250_dma_tx_complete`, `__dma_rx_complete`) with `if (!dma || ...)`
>      so a late callback after `up->dma = NULL` is ignored safely.

The current callbacks fetch p->dma before acquiring the port lock.
A callback can therefore retain the old non-NULL pointer across the
assignment of NULL. Checking that cached pointer does not protect
its backing allocation from being freed.

When termination returns an error, dmaengine_terminate_sync() also
returns without calling dmaengine_synchronize(), so callback draining
has not been established.

>
> That way, neither hardware nor a detached callback can ever UAF rx_buf or
> xmit_buf, while close(), system suspend, and subsequent PIO reopen all
> complete cleanly without an unbounded msleep() loop.
>
> Thanks,
> Raul