Re: [PATCH v2 11/64] dmaengine: mmp_tdma: hook completions to dma_chan BH

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

Pre-existing issues:
- [High] Missing teardown of bottom-half work during channel release, leading to potential use-after-free.
- [High] Incorrect devres initialization ordering for shared interrupts causes NULL pointer dereferences on probe and use-after-free on unbind.
- [High] SRAM descriptors allocated from the `gen_pool` are leaked when `mmp_tdma_prep_dma_cyclic` fails or is called multiple times.
--

--- Patch [11]: [PATCH v2 11/64] dmaengine: mmp_tdma: hook completions to dma_chan BH ---
commit c186dbcb9d7ae1ce6b3473be898cbd442e74d154
Author: Allen Pais <[email protected]>

dmaengine: mmp_tdma: hook completions to dma_chan BH

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

> diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
> index ba03321eeff79..d1ed2886f0e70 100644
> --- a/drivers/dma/mmp_tdma.c
> +++ b/drivers/dma/mmp_tdma.c

[ ... ]

> @@ -320,7 +319,7 @@ static irqreturn_t mmp_tdma_chan_handler(int irq, void *dev_id)
>  	struct mmp_tdma_chan *tdmac = dev_id;
>  
>  	if (mmp_tdma_clear_chan_irq(tdmac) == 0) {
> -		tasklet_schedule(&tdmac->tasklet);
> +		dma_chan_schedule_bh(&tdmac->chan);
>  		return IRQ_HANDLED;
>  	} else
>  		return IRQ_NONE;
> @@ -346,9 +345,9 @@ static irqreturn_t mmp_tdma_int_handler(int irq, void *dev_id)
>  		return IRQ_NONE;
>  }

[Severity: High]
This is a pre-existing issue, but is it possible for this interrupt handler to
trigger before the channel structures are fully initialized?

Looking at mmp_tdma_probe(), devm_request_irq() is called with IRQF_SHARED
before the channel structures are allocated and assigned:

drivers/dma/mmp_tdma.c:mmp_tdma_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, irq,
                           mmp_tdma_int_handler, IRQF_SHARED, "tdma", tdev);
    ...
}

If the shared IRQ line fires immediately, mmp_tdma_int_handler() iterates over
tdev->tdmac[] and unconditionally passes a potentially uninitialized NULL pointer
to mmp_tdma_chan_handler(). Could this lead to a NULL pointer dereference on
probe, or a use-after-free on unbind since devres reverses the initialization
order?

> -static void dma_do_tasklet(struct tasklet_struct *t)
> +static void dma_do_tasklet(struct dma_chan *c)
>  {
> -	struct mmp_tdma_chan *tdmac = from_tasklet(tdmac, t, tasklet);
> +	struct mmp_tdma_chan *tdmac = container_of(c, struct mmp_tdma_chan, chan);
>  
>  	dmaengine_desc_get_callback_invoke(&tdmac->desc, NULL);
>  }
> @@ -583,7 +582,7 @@ static int mmp_tdma_chan_init(struct mmp_tdma_device *tdev,
>  	tdmac->pool	   = pool;
>  	tdmac->status = DMA_COMPLETE;
>  	tdev->tdmac[tdmac->idx] = tdmac;
> -	tasklet_setup(&tdmac->tasklet, dma_do_tasklet);
> +	dma_chan_init_bh(&tdmac->chan, dma_do_tasklet);

[Severity: High]
This isn't a bug introduced by this patch, but should the bottom-half work be
explicitly torn down during channel release?

Currently, mmp_tdma_free_chan_resources() frees channel resources without
calling dma_chan_kill_bh():

drivers/dma/mmp_tdma.c:mmp_tdma_free_chan_resources() {
    ...
}

Similarly, when the device unbinds, devres frees the channel structure (tdmac)
but mmp_tdma_remove() does not explicitly kill the bottom-half work.

Could this leave pending bottom-half tasks to execute after memory is freed by
devres, leading to a use-after-free?

>  	/* add the channel to tdma_chan list */
>  	list_add_tail(&tdmac->chan.device_node,

[Severity: High]
This is a pre-existing issue, but can mmp_tdma_prep_dma_cyclic() leak SRAM
descriptors allocated from the gen_pool?

If mmp_tdma_config_write() fails, the function branches to err_out without
freeing the newly allocated descriptor:

drivers/dma/mmp_tdma.c:mmp_tdma_prep_dma_cyclic() {
    ...
    desc = mmp_tdma_alloc_descriptor(tdmac);
    if (!desc)
        goto err_out;

    if (mmp_tdma_config_write(chan, direction, &tdmac->slave_config))
        goto err_out;
    ...
}

Additionally, mmp_tdma_alloc_descriptor() unconditionally assigns the newly
allocated memory to tdmac->desc_arr:

drivers/dma/mmp_tdma.c:mmp_tdma_alloc_descriptor() {
    ...
    tdmac->desc_arr = gen_pool_dma_alloc(gpool, size, &tdmac->desc_arr_phys);
    ...
}

If mmp_tdma_prep_dma_cyclic() is called multiple times without an intervening
mmp_tdma_free_chan_resources(), the previous descriptor is blindly overwritten
and permanently leaked. Could this exhaust the limited device SRAM pool and
prevent further DMA operations?

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