Re: [PATCH v5 04/14] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Teardown race condition in dw_edma_remove where the hardware is disabled and IRQs are freed before the DMA device is unregistered.
- [High] State modification and linked list accesses are performed locklessly in the pause, resume, and terminate_all routines, leading to data races and list corruption.
- [High] dw_edma_device_terminate_all() fails to stop the hardware synchronously and neglects to clear pending virt-dma descriptors, leading to DMA use-after-free and memory leaks.
--

--- Patch [4]: [PATCH v5 04/14] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs ---
commit 65ff3b1fccd7b06e00ca72a89bc12e41c6366a28
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

This commit prevents a potential NULL pointer dereference in the shared
interrupt handler. It fixes the initialization order by setting the back
pointer before requesting the IRQ.

Cc: stable@vger.kernel.org

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 8a1ec0fd057bd..392981f3b7bb5 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> static int dw_edma_device_resume(struct dma_chan *dchan)
> {
> 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> 	int err = 0;
[ ... ]
> 	} else {
> 		chan->status = EDMA_ST_BUSY;
> 		dw_edma_start_transfer(chan);
> 	}
> 
> 	return err;
> }

[Severity: High]
This is a pre-existing issue, but does dw_edma_start_transfer() need to be
called with the vc.lock held here?

Looking at dw_edma_start_transfer(), it appears to locklessly call
vchan_next_desc(), which reads the desc_issued linked list. Could a
concurrent list modification from dmaengine_issue_pending() or the interrupt
handler corrupt the list pointers?

[ ... ]

> static int dw_edma_device_terminate_all(struct dma_chan *dchan)
> {
> 	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> 	int err = 0;
[ ... ]
> 	} else {
> 		chan->request = EDMA_REQ_STOP;
> 	}
> 
> 	return err;
> }

[Severity: High]
This is a pre-existing issue, but is setting chan->request sufficient to
safely terminate the transfer?

By not synchronously stopping the hardware via dw_edma_core_ch_quiesce(),
the hardware might continue running and writing to memory. If the client
subsequently frees the DMA buffer, could this result in a use-after-free or
IOMMU fault?

Additionally, this locklessly modifies chan->request, bypassing
synchronization with the interrupt handler. Does this routine also need to
acquire the lock and call vchan_get_all_descriptors() to purge pending
queues and prevent descriptor memory leaks?

[ ... ]

> int dw_edma_remove(struct dw_edma_chip *chip)
> {
[ ... ]
> 	/* Disable eDMA */
> 	dw_edma_core_off(dw);
> 
> 	/* Free irqs */
> 	for (i = (dw->nr_irqs - 1); i >= 0; i--)
> 		free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
> 	dw_edma_emul_irq_free(dw);
> 
> 	/* Deregister eDMA device */
> 	dma_async_device_unregister(&dw->dma);

[Severity: High]
This is a pre-existing issue, but is it safe to disable the hardware and
free the IRQs before unregistering the dmaengine device?

If a concurrent client calls dma_async_issue_pending() during driver unbind,
dw_edma_start_transfer() could re-enable the eDMA hardware. Because IRQs are
already freed, the software will never be notified when the transfer
completes.

Could this leave the hardware running while the DMA driver is fully removed,
potentially causing memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717050308.2144108-1-den@valinux.co.jp?part=4