Re: [PATCH v3 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

On Mon, Jul 27, 2026 at 06:51:00PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [High] Incomplete fix / DMA Use-After-Free due to early return in switchtec_dma_chan_stop() on halt timeout.
>

Is it true?

Frank
> Pre-existing issues:
> - [High] Soft lockup and severe latency spikes due to 100ms atomic busy-waiting under a spinlock.
> - [High] Extreme allocation brittleness due to requesting 32,768 descriptors with GFP_NOWAIT.
> - [High] DMA Use-After-Free in switchtec_dma_free_chan_resources() due to incorrect teardown ordering.
> - [High] Massive Use-After-Free of swdma_dev during driver removal.
> --
>
> --- Patch [3]: [PATCH v3 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error ---
> commit 647e1927ea58080fcbb88ef03dd8a74aa7555178
> Author: Logan Gunthorpe <[email protected]>
>
> dmaengine: switchtec-dma: halt channel on alloc_chan_resources error
>
> The commit adds a call to switchtec_dma_chan_stop() on the error-unwind
> path of switchtec_dma_alloc_chan_resources(). This aims to synchronously
> halt the channel and clear DMA base registers before freeing the
> descriptors, preventing a potential use-after-free if the hardware
> continues accessing the rings.
>
> Link: https://lore.kernel.org/dmaengine/[email protected]
>
> > diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> > index f77da31aeb65e..107769cca772f 100644
> > --- a/drivers/dma/switchtec_dma.c
> > +++ b/drivers/dma/switchtec_dma.c
> > @@ -1040,6 +1040,7 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
> >  	swdma_chan->comp_ring_active = false;
> >  	spin_unlock_bh(&swdma_chan->complete_lock);
> >  err_disable_channel:
> > +	switchtec_dma_chan_stop(swdma_chan);
> >  	disable_channel(swdma_chan);
> >  err_free_desc:
> >  	switchtec_dma_free_desc(swdma_chan);
>
> [Severity: High]
> Does this completely resolve the use-after-free if the hardware is
> unresponsive?
>
> Looking at switchtec_dma_chan_stop(), if halt_channel() times out, the
> function returns early without clearing the DMA base registers:
>
> drivers/dma/switchtec_dma.c:switchtec_dma_chan_stop() {
>         rc = halt_channel(swdma_chan);
>         if (rc)
>                 return;
>         ...
>         writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
>         ...
> }
>
> If that happens, execution continues to disable_channel() which does an
> unflushed write, and then switchtec_dma_free_desc() unconditionally frees
> the descriptors while the hardware might still be accessing them.
>
>
> [Severity: High]
> This is a pre-existing issue, but can this lead to soft lockups or severe
> latency spikes?
>
> In halt_channel(), wait_for_chan_status() is called while holding a spinlock:
>
> drivers/dma/switchtec_dma.c:halt_channel() {
>         spin_lock(&swdma_chan->hw_ctrl_lock);
>         writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
>         ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
>         spin_unlock(&swdma_chan->hw_ctrl_lock);
>         ...
> }
>
> wait_for_chan_status() uses readl_poll_timeout_atomic() with a 100ms timeout.
> Is it safe to block preemption for up to 100 milliseconds if the hardware
> is slow to respond?
>
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this allocation strategy
> cause issues under memory pressure?
>
> drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
>         for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
>                 desc = kzalloc_obj(*desc, GFP_NOWAIT);
>                 if (!desc) {
>         ...
> }
>
> SWITCHTEC_DMA_RING_SIZE is 32,768. Iteratively requesting 32,768 allocations
> with GFP_NOWAIT forbids sleeping and memory reclaim. Could this rapidly
> drain atomic memory pools and fail on slightly fragmented systems?
>
>
> [Severity: High]
> This is a pre-existing issue, but could this teardown ordering result in
> a use-after-free?
>
> drivers/dma/switchtec_dma.c:switchtec_dma_free_chan_resources() {
>         switchtec_dma_chan_stop(swdma_chan);
>         switchtec_dma_abort_desc(swdma_chan, 0);
>         switchtec_dma_free_desc(swdma_chan);
>
>         disable_channel(swdma_chan);
>         ...
> }
>
> If switchtec_dma_chan_stop() times out and fails to halt the channel,
> the descriptors are freed by switchtec_dma_free_desc() before the
> channel enable bit is cleared by disable_channel(). Could the active
> hardware continue executing and corrupt the freed descriptor memory?
>
>
> [Severity: High]
> This is a pre-existing issue, but does this sequence cause a use-after-free
> of swdma_dev?
>
> drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
>         ...
>         dma_async_device_unregister(&swdma_dev->dma_dev);
>
>         iounmap(swdma_dev->bar);
>         ...
> }
>
> If dma_async_device_unregister() drops the last reference to the DMA device,
> it synchronously invokes the release callback, switchtec_dma_release(),
> which frees swdma_dev. Does this mean iounmap(swdma_dev->bar) will
> dereference the freed swdma_dev pointer?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3