* Re: [PATCH] dmaengine: pl330: remove debugfs file on teardown
2026-06-15 9:16 [PATCH] dmaengine: pl330: remove debugfs file on teardown Pengpeng Hou
@ 2026-06-15 9:32 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-15 9:32 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: vkoul, dmaengine, Frank.Li
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Concurrent modification of the shared `desc_pool` list without acquiring `pool_lock` causes list corruption.
- [High] The IRQ handler is registered before the driver state is fully initialized, allowing the interrupt handler to run and access uninitialized data structures if an interrupt fires early.
- [High] Inconsistent lock acquisition order between the per-channel `pch->lock` and the device-wide `pl330->lock` creates an AB-BA deadlock.
- [High] Potential NULL pointer dereference in `pl330_debugfs_show()` due to an unprotected TOCTOU data race on `pch->thread`.
--
commit e5237f92634c12f29e973c872d0bf6600d10e173
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
dmaengine: pl330: remove debugfs file on teardown
init_pl330_debugfs() creates a debugfs file with struct pl330_dmac as
private data. pl330_remove() then unregisters the DMA device and frees
the PL330 channel/thread state without removing that file. This commit
adds the cleanup of the debugfs dentry during teardown to prevent a
use-after-free.
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 25ba84b187041..6214d9000db8e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
[ ... ]
> @@ -2952,14 +2953,24 @@ DEFINE_SHOW_ATTRIBUTE(pl330_debugfs);
[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
debugfs lifecycle, is there a potential NULL pointer dereference in
pl330_debugfs_show() due to a data race on pch->thread?
In pl330_debugfs_show(), the code checks:
if (!pch->thread || thrd->id != pch->thread->id)
This happens without holding pl330->lock or using READ_ONCE(). A concurrent
call to pl330_free_chan_resources() could set pch->thread = NULL between the
two evaluations. If the second load fetches NULL, evaluating pch->thread->id
would dereference a NULL pointer.
> static inline void init_pl330_debugfs(struct pl330_dmac *pl330)
> {
> - debugfs_create_file(dev_name(pl330->ddma.dev),
> - S_IFREG | 0444, NULL, pl330,
> - &pl330_debugfs_fops);
> + pl330->dbgfs = debugfs_create_file(dev_name(pl330->ddma.dev),
> + S_IFREG | 0444, NULL, pl330,
> + &pl330_debugfs_fops);
> }
[ ... ]
> @@ -3204,6 +3215,8 @@ static void pl330_remove(struct amba_device *adev)
> struct dma_pl330_chan *pch, *_p;
> int i, irq;
>
> + deinit_pl330_debugfs(pl330);
> +
> pm_runtime_get_noresume(pl330->ddma.dev);
>
> if (adev->dev.of_node)
> of_dma_controller_free(adev->dev.of_node);
>
> for (i = 0; i < AMBA_NR_IRQS; i++) {
> irq = adev->irq[i];
> if (irq)
> devm_free_irq(&adev->dev, irq, pl330);
> }
[Severity: High]
This is a pre-existing issue, but looking at the IRQ handling, is there an
initialization race in pl330_probe()?
In pl330_probe(), devm_request_irq() is called before pl330_add():
drivers/dma/pl330.c:pl330_probe() {
...
ret = devm_request_irq(&adev->dev, irq, pl330_irq_handler, 0,
dev_name(&adev->dev), pl330);
...
ret = pl330_add(pl330);
}
However, pl330_add() is where critical state used by the interrupt handler
is initialized, such as the pl330->lock spinlock and pl330->tasks tasklet.
If an interrupt fires between the IRQ request and pl330_add(), wouldn't
pl330_irq_handler() attempt to lock an uninitialized spinlock?
> dma_async_device_unregister(&pl330->ddma);
>
> /* Idle the DMAC */
> list_for_each_entry_safe(pch, _p, &pl330->ddma.channels,
> chan.device_node) {
>
> /* Remove the channel */
> list_del(&pch->chan.device_node);
>
> /* Flush the channel */
> if (pch->thread) {
> pl330_terminate_all(&pch->chan);
> pl330_free_chan_resources(&pch->chan);
[Severity: Critical]
This isn't a bug introduced by this patch, but does calling
pl330_terminate_all() and pl330_free_chan_resources() here cause concurrent
modification of the shared desc_pool?
When descriptors are returned to the pool, the driver modifies the shared
desc_pool while only holding the per-channel pch->lock or the DMAC
pl330->lock, rather than the required pool_lock:
drivers/dma/pl330.c:pl330_free_chan_resources() {
...
list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);
...
}
Could this lead to list corruption if multiple channels perform unlocked
modifications concurrently?
[Severity: High]
This is also a pre-existing issue, but does this cleanup path introduce an
AB-BA deadlock?
In normal operating paths like pl330_tx_status(), the locks are acquired in
this order:
spin_lock_irqsave(&pch->lock, flags);
spin_lock(&pch->thread->dmac->lock);
However, pl330_free_chan_resources() acquires pl330->lock first, and then
calls pl330_release_channel(), which subsequently calls dma_pl330_rqcb()
where it acquires pch->lock:
drivers/dma/pl330.c:pl330_free_chan_resources() {
spin_lock_irqsave(&pl330->lock, flags);
...
pl330_release_channel(pch->thread);
-> dma_pl330_rqcb()
-> spin_lock_irqsave(&pch->lock, flags);
Could this reversed locking order deadlock if another CPU runs a state check
concurrently?
> }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615091645.28878-1-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread