Re: [PATCH v5 02/12] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] switchtec_dma_remove() tears down hardware resources before unregistering the DMA device, exposing a race window where new clients can request channels on a dying device.
- [High] The driver holds a spinlock between dmaengine prep_desc and tx_submit, violating dmaengine semantics and risking deadlocks.
- [High] The driver performs massive allocations using GFP_NOWAIT in process context, which is highly likely to fail under memory pressure.
--

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

dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

This commit fixes a resource leak in switchtec_dma_alloc_chan_resources()
where descriptor rings and coherent DMA memory were not freed on failure.
It adds an error-unwind path to properly disable the channel and free the
resources since the dmaengine core does not handle this automatically.

Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index a10818efba4e5..e55fe9ff7e2c5 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

While reviewing this code, I noticed a few pre-existing issues in the driver
that might need attention:

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);

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown order expose
a race window where new clients can request channels on a dying device?

If a concurrent client requests a channel via dma_request_chan() during
switchtec_dma_remove(), the DMA core will route it to this device because it
has not yet been unregistered.

The client will then trigger switchtec_dma_alloc_chan_resources(), which
re-activates a channel whose IRQs and tasklets were already permanently
destroyed by switchtec_dma_chans_release().

Should dma_async_device_unregister() be called first to safely detach the
device from the subsystem before destroying local hardware resources?


drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
...
> 	spin_lock_bh(&swdma_chan->submit_lock);
> 
> 	if (!swdma_chan->ring_active)
> 		goto err_unlock;

and in switchtec_dma_tx_submit():

> 	spin_unlock_bh(&swdma_chan->submit_lock);
> 
> 	return cookie;

[Severity: High]
This is a pre-existing issue, but could holding submit_lock across dmaengine
API boundaries risk deadlocks?

If a client attempts to prepare multiple descriptors sequentially before
submitting them (batching prep operations), wouldn't the second call to
dmaengine_prep_dma_memcpy() attempt to acquire the exact same lock and
deadlock?

Furthermore, this leaves the caller's CPU permanently locked with softirqs
disabled if the client prepares a descriptor but aborts before submitting it.
The dmaengine API explicitly allows batching of prep operations.


drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
...
> 	swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
> 					       &swdma_chan->dma_addr_sq,
> 					       GFP_NOWAIT);
...
> 	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
> 		desc = kzalloc_obj(*desc, GFP_NOWAIT);
> 		if (!desc) {
> 			rc = -ENOMEM;
> 			goto free_and_exit;
> 		}

[Severity: High]
This isn't a bug introduced by this patch, but could these allocations use
GFP_KERNEL instead?

Because switchtec_dma_alloc_chan_resources() is called in process context and
is allowed to sleep, using GFP_NOWAIT here performs massive allocations
without direct reclaim.

The driver attempts to allocate 1MB of coherent DMA memory and performs 32,768
consecutive atomic allocations, which seems highly likely to fail on
fragmented or loaded systems under memory pressure.

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