Re: [PATCH] dmaengine: pl330: remove debugfs file on teardown

[sashiko-bot@kernel.org]

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