| Message ID | 20260902062153.31048-4-logang@deltatee.com |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] The channel status ISR iterates over `dma_dev->channels` before the list head is initialized, causing a NULL pointer dereference if the interrupt fires early. -- commit b0c8d064063987d7e3518dbe0fb57de31b776bde Author: Logan Gunthorpe <logang@deltatee.com> Date: Wed Sep 2 06:21:45 2026 +0000 dmaengine: switchtec-dma: fix channel leak on registration failure This patch addresses a memory leak during initialization failure by introducing a switchtec_dma_chans_free() helper. It ensures channel memory is cleanly freed and removes channels from the DMA device list to prevent the ISR from dereferencing freed memory. Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org Link: https://lore.kernel.org/dmaengine/20260727190604.397DB1F000E9@smtp.kernel.org > diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c > index e55fe9ff7e2c5..02d7c38363d68 100644 > --- a/drivers/dma/switchtec_dma.c > +++ b/drivers/dma/switchtec_dma.c [ ... ] > @@ -1316,6 +1322,7 @@ static int switchtec_dma_create(struct pci_dev *pdev) > > err_chans_release_exit: > switchtec_dma_chans_release(pdev, swdma_dev); > + switchtec_dma_chans_free(swdma_dev); > > err_exit: > if (swdma_dev->chan_status_irq) [Severity: High] This isn't a bug introduced by this patch, but since we are modifying the cleanup and setup paths here, could this code be vulnerable to a race condition with the interrupt handler? In switchtec_dma_create(), we request and unmask the interrupt before the channels are enumerated: rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr, swdma_dev, KBUILD_MODNAME); However, the dma->channels list head isn't initialized until later when switchtec_dma_chans_enumerate() is called: INIT_LIST_HEAD(&dma->channels); If a pending interrupt triggers immediately upon registration, the ISR will execute and iterate over the uninitialized list: list_for_each_entry(chan, &dma_dev->channels, device_node) { Because swdma_dev was allocated with kzalloc_obj(), dma->channels.next is NULL at this point. Will this result in a NULL pointer dereference in the ISR during device probe? Should INIT_LIST_HEAD(&dma->channels) be called before the interrupt is requested?
diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c index e55fe9ff7e2c..02d7c38363d6 100644 --- a/drivers/dma/switchtec_dma.c +++ b/drivers/dma/switchtec_dma.c @@ -1175,6 +1175,18 @@ static int switchtec_dma_chans_release(struct pci_dev *pdev, return 0; } +static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev) +{ + int i; + + for (i = 0; i < swdma_dev->chan_cnt; i++) { + list_del(&swdma_dev->swdma_chans[i]->dma_chan.device_node); + kfree(swdma_dev->swdma_chans[i]); + } + + kfree(swdma_dev->swdma_chans); +} + static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev, struct pci_dev *pdev, int chan_cnt) { @@ -1200,7 +1212,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev, if (rc) { dev_err(&pdev->dev, "Channel %d: init channel failed\n", i); - chan_cnt = i; + swdma_dev->chan_cnt = i; goto err_exit; } } @@ -1208,10 +1220,8 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev, return chan_cnt; err_exit: - for (i = 0; i < chan_cnt; i++) - switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]); - - kfree(swdma_dev->swdma_chans); + switchtec_dma_chans_release(pdev, swdma_dev); + switchtec_dma_chans_free(swdma_dev); return rc; } @@ -1220,12 +1230,8 @@ static void switchtec_dma_release(struct dma_device *dma_dev) { struct switchtec_dma_dev *swdma_dev = container_of(dma_dev, struct switchtec_dma_dev, dma_dev); - int i; - for (i = 0; i < swdma_dev->chan_cnt; i++) - kfree(swdma_dev->swdma_chans[i]); - - kfree(swdma_dev->swdma_chans); + switchtec_dma_chans_free(swdma_dev); put_device(dma_dev->dev); kfree(swdma_dev); @@ -1316,6 +1322,7 @@ static int switchtec_dma_create(struct pci_dev *pdev) err_chans_release_exit: switchtec_dma_chans_release(pdev, swdma_dev); + switchtec_dma_chans_free(swdma_dev); err_exit: if (swdma_dev->chan_status_irq)
switchtec_dma_chans_release() is called in three places but the underlying memory is not freed in all of those places. In order to clean this up, introduce a switchtec_dma_chans_free() helper that will free the memory. Ensure each call to switchtec_dma_chans_release() has a corresponding switchtec_dma_chans_free() call. (The release in switchtec_dma_remove() pairs with the free in switchtec_dma_release()). swdma_dev->chan_cnt is now set to the number of channels that succeeded when one fails to initialise, so switchtec_dma_chans_free() can still be used if not all channels succeed in being allocated. switchtec_dma_chans_free() also removes each channel from dma_dev->channels before freeing it, since the channel status ISR walks that list and would otherwise dereference freed memory. Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup") Reported-by: Sashiko <sashiko-bot@kernel.org> Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org Link: https://lore.kernel.org/dmaengine/20260727190604.397DB1F000E9@smtp.kernel.org Reviewed-by: Frank Li <Frank.Li@nxp.com> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/dma/switchtec_dma.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)