Re: [PATCH v3 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

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

Pre-existing issues:
- [High] Memory leak of DMA channel resources on error paths in switchtec_dma_alloc_chan_resources()
- [High] Memory leak of channel structures (swdma_chan) and pointer array on driver probe error
- [High] Incorrect IRQ teardown using free_irq() with a raw MSI-X vector index instead of pci_free_irq()
--

--- Patch [1]: [PATCH v3 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() ---
commit e34f78c4f91061f06369e0d37a6d5d75685edb26
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

switchtec_dma_free_desc() frees swdma_chan->hw_sq, hw_cq, and every
desc_ring[] entry without clearing the pointers afterward. This patch
NULLs out each pointer as it's freed so a subsequent call is a no-op
for anything already released.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..a4a7d66d042de 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
>  	if (swdma_chan->hw_sq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
> +	swdma_chan->hw_sq = NULL;
>  
>  	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
>  	if (swdma_chan->hw_cq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_cq, swdma_chan->dma_addr_cq);
> +	swdma_chan->hw_cq = NULL;
>  
> -	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++)
> +	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
>  		kfree(swdma_chan->desc_ring[i]);
> +		swdma_chan->desc_ring[i] = NULL;
> +	}
>  }
>  
>  static int switchtec_dma_alloc_desc(struct switchtec_dma_chan *swdma_chan)

[Severity: High]
This isn't a bug introduced by this patch, but does
switchtec_dma_alloc_chan_resources() leak DMA channel resources on error
paths?

If switchtec_dma_alloc_desc() succeeds but a subsequent step like
enable_channel() fails, the driver returns the error code immediately:

switchtec_dma_alloc_chan_resources() {
	...
	rc = enable_channel(swdma_chan);
	if (rc)
		return rc;
	...
}

Does this fail to call switchtec_dma_free_desc() to free the resources
successfully allocated earlier in the function? Since the dmaengine core
does not automatically invoke the cleanup routine for partial failures,
could these resources be permanently leaked?

[Severity: High]
This is a pre-existing issue, but I noticed a potential memory leak of
channel structures on driver probe error.

If switchtec_dma_chans_enumerate() fails partway through, the error path
iterates over initialized channels and calls switchtec_dma_chan_free(),
but it doesn't seem to free the individual channel elements:

switchtec_dma_chans_enumerate() {
	...
err_exit:
	for (i = 0; i < chan_cnt; i++)
		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);

	kfree(swdma_dev->swdma_chans);
	...
}

Furthermore, if dma_async_device_register() fails in switchtec_dma_create(),
does the error path forget to free the channel pointer array or the
structures it points to?

switchtec_dma_create() {
	...
err_chans_release_exit:
	switchtec_dma_chans_release(pdev, swdma_dev);
	...
}

[Severity: High]
This is a pre-existing issue, but is the driver using free_irq() incorrectly
with a raw vector index instead of pci_free_irq() during teardown?

If switchtec_dma_create() encounters an error after successfully requesting
the channel status IRQ, it branches to err_exit and calls free_irq():

switchtec_dma_create() {
	...
err_exit:
	if (swdma_dev->chan_status_irq)
		free_irq(swdma_dev->chan_status_irq, swdma_dev);
	...
}

Is swdma_dev->chan_status_irq holding the raw hardware index rather than
the Linux IRQ number? Could this incorrectly free an unrelated system
interrupt and leak the originally requested IRQ?

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