Re: [PATCH v2 16/64] dmaengine: ppc4xx: replace irq tasklet with dma_chan BH

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

Pre-existing issues:
- [High] Missing dma_chan_kill_bh() on probe error paths leads to use-after-free.
- [High] Lockless traversal of chan->chain in hardirq context races with list modification, causing panics.
- [High] Missing channel lock in ppc440spe_adma_issue_pending causes concurrent list/hardware corruption.
- [High] Direct freeing of DMA structures in remove() without waiting for active clients causes Use-After-Free.
--

--- Patch [16]: [PATCH v2 16/64] dmaengine: ppc4xx: replace irq tasklet with dma_chan BH ---
commit f5f6478c931c241a7fdae2271fa2dee782cc4570
Author: Allen Pais <[email protected]>
Subject: dmaengine: ppc4xx: replace irq tasklet with dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index 279a431ccae38..98cfd012ca5bf 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
> @@ -1658,9 +1658,9 @@ static void __ppc440spe_adma_slot_cleanup(struct ppc440spe_adma_chan *chan)
>  /**
>   * ppc440spe_adma_tasklet - clean up watch-dog initiator
>   */
> -static void ppc440spe_adma_tasklet(struct tasklet_struct *t)
> +static void ppc440spe_adma_tasklet(struct dma_chan *c)
>  {
> -	struct ppc440spe_adma_chan *chan = from_tasklet(chan, t, irq_tasklet);
> +	struct ppc440spe_adma_chan *chan = to_ppc440spe_adma_chan(c);
>  
>  	spin_lock_nested(&chan->lock, SINGLE_DEPTH_NESTING);
>  	__ppc440spe_adma_slot_cleanup(chan);

[Severity: High]
This is a pre-existing issue, but does this list modification race with
hardirq context?

The hardirq handlers (ppc440spe_adma_eot_handler and
ppc440spe_adma_err_handler) call ppc440spe_adma_device_clear_eot_status(),
which traverses chan->chain locklessly:

drivers/dma/ppc4xx/adma.c:ppc440spe_adma_device_clear_eot_status() {
    ...
    list_for_each_entry(iter, &chan->chain, chain_node) {
        if (iter->phys == phys)
            break;
    }
    ...
}

Meanwhile, this softirq/BH code modifies the list without disabling local
interrupts (using spin_lock_nested or spin_lock_bh). If a hardware interrupt
fires on the same CPU while the list is being modified, couldn't the hardirq
handler traverse a corrupted list and cause memory corruption or a panic?

[ ... ]

> @@ -4137,7 +4137,7 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev)
>  	chan->common.device = &adev->common;
>  	dma_cookie_init(&chan->common);
>  	list_add_tail(&chan->common.device_node, &adev->common.channels);
> -	tasklet_setup(&chan->irq_tasklet, ppc440spe_adma_tasklet);
> +	dma_chan_init_bh(&chan->common, ppc440spe_adma_tasklet);
>  
>  	/* allocate and map helper pages for async validation or

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path
need to call dma_chan_kill_bh()?

In ppc440spe_adma_probe(), dma_chan_init_bh() initializes the bottom-half
work. Later, hardware IRQs are requested. If an interrupt fires and schedules
the BH via dma_chan_schedule_bh(), but a subsequent initialization step fails,
the error path uses free_irq() and then frees the channel structure without
canceling the pending work:

err_page_alloc:
    kfree(chan);

When the background work queue executes later, won't it access the freed
chan memory and cause a use-after-free?

[ ... ]

> @@ -4247,7 +4247,7 @@ static void ppc440spe_adma_remove(struct platform_device *ofdev)
>  				 device_node) {
>  		ppc440spe_chan = to_ppc440spe_adma_chan(chan);
>  		ppc440spe_adma_release_irqs(adev, ppc440spe_chan);
> -		tasklet_kill(&ppc440spe_chan->irq_tasklet);
> +		dma_chan_kill_bh(&ppc440spe_chan->common);
>  		if (adev->id != PPC440SPE_XOR_ID) {
>  			dma_unmap_page(&ofdev->dev, ppc440spe_chan->pdest,
>  					PAGE_SIZE, DMA_BIDIRECTIONAL);

[Severity: High]
This is a pre-existing issue, but does this function safely wait for all
clients to drop their references?

In ppc440spe_adma_remove(), the driver calls dma_async_device_unregister()
which unregisters the device but does not synchronously block for active
client references to be dropped.

Immediately after, it frees the channel and device structures:

    list_del(&chan->device_node);
    kfree(ppc440spe_chan);
    ...
    kfree(adev);

If a client (like the network stack or filesystem) still holds a reference
and attempts to use the channel or release it later, will it dereference
these freed structures and cause a use-after-free?

[ ... ]

[Severity: High]
This is another pre-existing issue in the driver. Is channel locking missing 
in ppc440spe_adma_issue_pending()?

The DMA engine callback executes without acquiring chan->lock when checking
chan->pending and calling ppc440spe_chan_append():

drivers/dma/ppc4xx/adma.c:ppc440spe_adma_issue_pending() {
    ...
    if (ppc440spe_chan->pending) {
        ppc440spe_chan->pending = 0;
        ppc440spe_chan_append(ppc440spe_chan);
    }
    ...
}

Other parts of the driver (like ppc440spe_adma_tx_submit) correctly lock
around these calls using spin_lock_bh. Because issue_pending is an
asynchronous dmaengine API that can be called concurrently across multiple
CPUs, couldn't this race and corrupt hardware MMIO registers and descriptor
lists?

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