Re: [PATCH v4 06/12] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

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

New issues:
- [High] Leaking DMA descriptors and ring buffers when channel stop fails (e.g., during device hot-unplug or module unload).

Pre-existing issues:
- [High] Hardware resources (IRQs and tasklets) are released before the DMA device is unregistered, allowing clients to request or use unbacked channels.
--

--- Patch [6]: [PATCH v4 06/12] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void ---
commit 4ae99d5104edb85994c22547e64122fc62df38cb
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

switchtec_dma_chans_release() always returned 0, and no caller
checked it.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 48da269198f3f..63e60de9f86dc 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1177,15 +1177,13 @@ static int switchtec_dma_chan_free(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> -static int switchtec_dma_chans_release(struct pci_dev *pdev,
> -				       struct switchtec_dma_dev *swdma_dev)
> +static void switchtec_dma_chans_release(struct pci_dev *pdev,
> +					struct switchtec_dma_dev *swdma_dev)
>  {
>  	int i;
>  
>  	for (i = 0; i < swdma_dev->chan_cnt; i++)
>  		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
> -
> -	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but hardware resources (IRQs and tasklets) are
released before the DMA device is unregistered, allowing clients to request or
use unbacked channels.

In switchtec_dma_remove(), switchtec_dma_chans_release() is called first, which
frees all per-channel IRQs and kills their descriptor tasklets:

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
	...
	switchtec_dma_chans_release(pdev, swdma_dev);
	rcu_assign_pointer(swdma_dev->pdev, NULL);
	synchronize_rcu();
	pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
	pci_free_irq_vectors(pdev);
	dma_async_device_unregister(&swdma_dev->dma_dev);
}

Could this cause active DMA transfers to hang forever without completion, since
the DMA device is not unregistered from the core until much later?

Because the device is still in the DMA engine channel table, could a client
theoretically request a new channel in this window?

[Severity: High]
Does this code leak DMA descriptors and ring buffers when channel stop fails?

In switchtec_dma_free_chan_resources(), if switchtec_dma_chan_stop() returns a
non-zero error (like -ENODEV during hot-unplug), the cleanup of descriptor
memory is completely skipped:

drivers/dma/switchtec_dma.c:switchtec_dma_free_chan_resources() {
	...
	rc = switchtec_dma_chan_stop(swdma_chan);
	switchtec_dma_abort_desc(swdma_chan, 0);
	if (!rc)
		switchtec_dma_free_desc(swdma_chan);
}

During switchtec_dma_remove(), swdma_dev->pdev is set to NULL before
dma_async_device_unregister() is called. When dma_async_device_unregister()
subsequently cleans up active channels, it invokes
switchtec_dma_free_chan_resources().

Because pdev is NULL, switchtec_dma_chan_stop() returns -ENODEV. The code checks
if (!rc) and skips switchtec_dma_free_desc(), leaking the resources.

A similar issue appears to exist in the error path of
switchtec_dma_alloc_chan_resources() if the channel stop returns an error:

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_chan_resources() {
	...
err_disable_channel:
	if (switchtec_dma_chan_stop(swdma_chan)) {
		disable_channel(swdma_chan);
		return rc;
	}
	disable_channel(swdma_chan);
err_free_desc:
	switchtec_dma_free_desc(swdma_chan);
}

Can this leak memory for every active channel on standard device teardown or
hot-unplug?

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