Re: [PATCH v5 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Clearing DMA base registers to 0 redirects runaway DMA to physical address 0, causing critical memory corruption.
- [High] Unconditional memory leak of DMA descriptor rings during device hot-unplug because -ENODEV is treated as a halt timeout.
- [High] Dangling pointers allow intentionally leaked DMA memory to be accidentally freed on subsequent channel allocation failure, risking hardware memory corruption.

Pre-existing issues:
- [Critical] Client data buffers can be trashed by runaway hardware because pending descriptors are aborted and unmapped despite a halt failure.
--

commit 76ff5cafd116d5c109fd77135de2f45efb9196b7
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

Attempt the register clear regardless of whether the halt was successful,
and have switchtec_dma_chan_stop() return the halt result. In case of failure,
descriptor rings are intentionally leaked to prevent buggy hardware from
trashing freed memory.

Link: https://lore.kernel.org/dmaengine/20260721162531.BA01A1F01560@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index e55fe9ff7e2c5..e2bb65237d2c9 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -543,26 +543,33 @@ switchtec_dma_abort_desc(struct switchtec_dma_chan *swdma_chan, int force)
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  }
>  
> -static void switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
> +static int switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
>  {
> +	struct pci_dev *pdev;
>  	int rc;
>  
>  	rc = halt_channel(swdma_chan);
> -	if (rc)
> -		return;
>  
>  	rcu_read_lock();
> -	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
> +	pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
> +	if (!pdev) {
>  		rcu_read_unlock();
> -		return;
> +		return rc;
>  	}
>  
> +	if (rc)
> +		pci_err(pdev,
> +			"Channel %d halt timed out, clearing DMA base registers anyway\n",
> +			swdma_chan->index);
> +
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_hi);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_hi);

[Severity: High]
If the channel fails to halt (times out), writing 0 to sq_base and cq_base
will point the hardware's queues to physical address 0.

If the active hardware fetches a descriptor or writes a completion, does this
redirect runaway DMA to corrupt physical memory at address 0 instead of writing
to the intentionally leaked descriptor rings?

>  
>  	rcu_read_unlock();
> +
> +	return rc;
>  }

[ ... ]

> @@ -1050,6 +1057,7 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct switchtec_dma_chan *swdma_chan =
>  		container_of(chan, struct switchtec_dma_chan, dma_chan);
> +	int rc;
>  
>  	spin_lock_bh(&swdma_chan->submit_lock);
>  	swdma_chan->ring_active = false;
> @@ -1059,9 +1067,10 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  
> -	switchtec_dma_chan_stop(swdma_chan);
> +	rc = switchtec_dma_chan_stop(swdma_chan);
>  	switchtec_dma_abort_desc(swdma_chan, 0);

[Severity: Critical]
This is a pre-existing issue, but does calling switchtec_dma_abort_desc()
here defeat the purpose of leaking the descriptor memory?

If switchtec_dma_chan_stop() times out due to a hardware hang, this code
still unconditionally calls dma_descriptor_unmap() and invokes client
callbacks with DMA_TRANS_ABORTED:

switchtec_dma_abort_desc()
    ...
    dma_cookie_complete(&desc->txd);
    dma_descriptor_unmap(&desc->txd);
    if (!force)
        dmaengine_desc_get_callback_invoke(&desc->txd, &res);

Could this cause clients to free or reuse their data buffers while the
runaway hardware is still actively writing to or reading from them?

> -	switchtec_dma_free_desc(swdma_chan);
> +	if (!rc)
> +		switchtec_dma_free_desc(swdma_chan);

[Severity: High]
If the PCI device is removed (e.g. hot-unplug), pdev is set to NULL, and
switchtec_dma_chan_stop() detects this and returns -ENODEV.

Does treating -ENODEV as a hardware halt failure here intentionally skip
switchtec_dma_free_desc(), causing a permanent memory leak of the DMA
descriptor rings during normal device unbind?

[Severity: High]
If switchtec_dma_chan_stop() times out, the descriptor memory is
intentionally leaked by skipping switchtec_dma_free_desc(), but the pointers
(hw_sq, hw_cq, desc_ring) are not cleared to NULL in the struct.

If a client subsequently allocates the channel again, and any part of the
new allocation fails:

switchtec_dma_alloc_desc()
    swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
                                           &swdma_chan->dma_addr_sq,
                                           GFP_NOWAIT);
    if (!swdma_chan->hw_sq) {
        rc = -ENOMEM;
        goto free_and_exit;
    }

Would the error path (free_and_exit) call switchtec_dma_free_desc() and
erroneously free the old, leaked memory that the runaway hardware might
still be writing to?

>  
>  	disable_channel(swdma_chan);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=3