Re: [PATCH v3 10/34] dmaengine: mmp_pdma: replace per-chan tasklet with dmaengine BH

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

Pre-existing issues:
- [High] The driver fails to flush or kill the dmaengine BH work item during channel teardown or module removal, leading to a potential use-after-free.
- [High] The interrupt handler `mmp_pdma_chan_handler` unconditionally dereferences `phy->vchan`, which can be NULL, leading to a kernel panic.
--

--- Patch [10]: [PATCH v3 10/34] dmaengine: mmp_pdma: replace per-chan tasklet with dmaengine BH ---
commit 8e2fd9bba919782b8255f7948fa3a9fec6c35960
Author: Allen Pais <[email protected]>

dmaengine: mmp_pdma: replace per-chan tasklet with dmaengine BH

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

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..4f7d3193d7a3c 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c

[ ... ]

> @@ -374,7 +373,7 @@ static irqreturn_t mmp_pdma_chan_handler(int irq, void *dev_id)
>  	if (clear_chan_irq(phy) != 0)
>  		return IRQ_NONE;
>  
> -	tasklet_schedule(&phy->vchan->tasklet);
> +	dmaengine_schedule_bh(&phy->vchan->chan);

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference if a concurrent teardown happens?

The mmp_pdma_chan_handler() is executed in interrupt context. If the client
driver terminates a transfer via dmaengine_terminate_all() concurrently with
an interrupt firing for that channel, mmp_pdma_free_phy() will set
phy->vchan = NULL without an accompanying synchronize_irq().

The handler then blindly dereferences phy->vchan when calling
dmaengine_schedule_bh(). Interestingly, clear_chan_irq() explicitly checks if
phy->vchan is NULL just above this, which suggests the state is structurally
possible during interrupts.

>  	return IRQ_HANDLED;
>  }

[ ... ]

> @@ -1170,7 +1167,7 @@ static int mmp_pdma_chan_init(struct mmp_pdma_device *pdev, int idx, int irq)
>  	spin_lock_init(&chan->desc_lock);
>  	chan->dev = pdev->dev;
>  	chan->chan.device = &pdev->device;
> -	tasklet_setup(&chan->tasklet, dma_do_tasklet);
> +	dmaengine_init_bh(&chan->chan, dma_do_tasklet);

[Severity: High]
This is a pre-existing issue, but does the driver ever flush or kill the
pending work item before freeing resources?

During channel teardown or module removal, the driver doesn't seem to cancel
the pending BH work. If a DMA operation completes right before or during
mmp_pdma_free_chan_resources() or mmp_pdma_remove(), the work item might be
scheduled on the dmaengine_bh_wq.

The cleanup code destroys chan->desc_pool in mmp_pdma_free_chan_resources()
or frees the chan memory via devres without first calling
dmaengine_kill_bh(). When the scheduled BH later executes in
dma_do_tasklet(), it could access the destroyed pool or freed chan structure:

drivers/dma/mmp_pdma.c:dma_do_tasklet() {
    ...
    dma_pool_free(chan->desc_pool, desc, txd->phys);
    ...
}

Should there be a call to dmaengine_kill_bh() on the teardown paths?

>  	INIT_LIST_HEAD(&chan->chain_pending);
>  	INIT_LIST_HEAD(&chan->chain_running);
>

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