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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unintentional memory leak of DMA descriptor rings during device hot-unplug or driver unbind.
- [High] Zeroing DMA base registers on an unhalted channel directs ongoing DMA to physical address 0, causing memory corruption.
--

--- Patch [3]: [PATCH v4 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop() ---
commit ba435d6b93a5b911f612d1880a59068a9e918427
Author: Logan Gunthorpe <[email protected]>

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

This patch modifies switchtec_dma_chan_stop() to clear DMA base registers
even if the hardware halt times out. It also updates callers to check the
halt result and skip freeing descriptor rings if the halt was unconfirmed.

Link: https://lore.kernel.org/dmaengine/[email protected]

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..cbbc9ad082471 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);
>  
>  	rcu_read_unlock();
> +
> +	return rc;
>  }

[Severity: High]
If the channel failed to halt and is still actively processing transactions,
could writing 0 to the base address registers redirect ongoing DMA to physical 
address 0x0?

The commit message states the descriptor rings are intentionally leaked so
the hardware can safely continue to write into them. However, by clearing
the base addresses here while the hardware is active, it appears the active
DMA is instead pointed at address 0x0, which might lead to memory corruption.

[ ... ]

> @@ -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);
> -	switchtec_dma_free_desc(swdma_chan);
> +	if (!rc)
> +		switchtec_dma_free_desc(swdma_chan);
>  
>  	disable_channel(swdma_chan);
>  }

[Severity: High]
Does this logic skip freeing the descriptor rings during normal device unplug?

When a device is hot-unplugged and the driver unbinds, swdma_dev->pdev is set
to NULL. This causes halt_channel() and switchtec_dma_chan_stop() to return
-ENODEV.

Because rc is -ENODEV (non-zero) on a standard unplug, the condition if (!rc)
evaluates to false. This seems to unintentionally bypass
switchtec_dma_free_desc() and permanently leak the descriptor rings and
coherent memory. A similar bypass happens in the error paths of
switchtec_dma_alloc_chan_resources().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3