Re: [PATCH v3 13/34] dmaengine: mxs-dma: use dmaengine BH scheduling

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

Pre-existing issues:
- [High] The driver does not kill the deferred bottom-half work during channel teardown or error paths, nor does it provide a device_synchronize callback, leading to a potential use-after-free.
- [High] Requesting the IRQ before enabling the device clock can result in a synchronous external abort and bus hang.
--

--- Patch [13]: [PATCH v3 13/34] dmaengine: mxs-dma: use dmaengine BH scheduling ---
commit 84e84c7d902dd9157e789fe8d760954306bc43d6
Author: Allen Pais <[email protected]>
dmaengine: mxs-dma: use dmaengine BH scheduling

Replace 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/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 7acb3d29dad30..149ac402f7b4b 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
[ ... ]
> @@ -386,8 +385,8 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
>  		dma_cookie_complete(&mxs_chan->desc);
>  	}
>  
> -	/* schedule tasklet on this channel */
> -	tasklet_schedule(&mxs_chan->tasklet);
> +	/* schedule BH on this channel */
> +	dmaengine_schedule_bh(&mxs_chan->chan);
>  
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but since the driver is moving to the BH
mechanism, should we also ensure the scheduled work is properly flushed during
teardown and error paths?

Looking at mxs_dma_alloc_chan_resources(), if clk_prepare_enable() fails,
the error path frees the IRQ and the channel ccw structure:

mxs_dma_alloc_chan_resources() {
    ...
err_clk:
    free_irq(mxs_chan->chan_irq, mxs_dma);
err_irq:
    dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
            mxs_chan->ccw, mxs_chan->ccw_phys);
    ...
}

Because free_irq() waits for the hardirq handler but not the scheduled
workqueue, if an early interrupt schedules the BH work, can the workqueue
execute after the channel resources have been freed, leading to a
use-after-free?

Similarly, mxs_dma_free_chan_resources() frees channel resources without
calling dmaengine_kill_bh(). The driver also lacks a device_synchronize
callback, which makes dmaengine_terminate_sync() a no-op.

Should we add dmaengine_kill_bh() and a synchronization callback to prevent
the scheduled work from running after the client frees the channel?

[ ... ]
> @@ -781,7 +780,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
>  		mxs_chan->chan.device = &mxs_dma->dma_device;
>  		dma_cookie_init(&mxs_chan->chan);
>  
> -		tasklet_setup(&mxs_chan->tasklet, mxs_dma_tasklet);
> +		dmaengine_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
>  
>  
>  		/* Add the channel to mxs_chan list */

[Severity: High]
This isn't a bug introduced by this patch, but I wanted to ask about the
initialization ordering which can trigger the interrupt handler early.

When a channel is allocated in mxs_dma_alloc_chan_resources(), the IRQ is
requested before the clock is enabled:

mxs_dma_alloc_chan_resources() {
    ...
    ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
              0, "mxs-dma", mxs_dma);
    if (ret)
        goto err_irq;

    ret = clk_prepare_enable(mxs_dma->clk);
    ...
}

If an interrupt line is asserted early (for example, from a previously
aborted transaction where the client didn't call terminate_all),
mxs_dma_int_handler() will fire immediately. It then performs an MMIO read
on the memory-mapped controller registers (mxs_dma->base + HW_APBHX_CTRL1).

Since accessing device registers with a disabled clock results in a bus hang
on this platform, would it be safer to enable the clock before requesting
the IRQ to prevent a synchronous external abort?

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