Re: [PATCH v3 08/34] dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH

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

Pre-existing issues:
- [High] The watchdog timer is not safely canceled during teardown or error paths, leading to Use-After-Free.
- [High] Completed descriptors are moved to the free list before callback invocation, allowing lockless data races.
- [High] Bottom-half work is not flushed before freeing channel descriptors, leading to Use-After-Free.
- [High] IRQ is requested before initializing channel data structures and masking interrupts, leading to a NULL pointer dereference.
--

--- Patch [8]: [PATCH v3 08/34] dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH ---
commit a54288aab8aa66cb0228a7320efe5075981ac5ce
Author: Allen Pais <[email protected]>

dmaengine: imx-dma: flip per-chan tasklet to dmaengine BH

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/imx-dma.c b/drivers/dma/imx-dma.c
> index 81c6276436f81..308fe407fe161 100644
> --- a/drivers/dma/imx-dma.c
> +++ b/drivers/dma/imx-dma.c

[ ... ]

> @@ -592,9 +591,10 @@ static int imxdma_xfer_desc(struct imxdma_desc *d)
>  	return 0;
>  }
>  
> -static void imxdma_tasklet(struct tasklet_struct *t)
> +static void imxdma_tasklet(struct dma_chan *chan)
>  {
> -	struct imxdma_channel *imxdmac = from_tasklet(imxdmac, t, dma_tasklet);
> +	struct imxdma_channel *imxdmac = container_of(chan, struct imxdma_channel,
> +						      chan);
>  	struct imxdma_engine *imxdma = imxdmac->imxdma;
>  	struct imxdma_desc *desc, *next_desc;
>  	unsigned long flags;

[Severity: High]
This isn't a bug introduced by this patch, but could this lockless data race
cause a loss of completion callbacks?

In imxdma_tasklet(), the completed descriptor is placed on the ld_free list
before the channel lock is dropped and the callback is invoked:

imxdma_tasklet()
    list_move_tail(imxdmac->ld_active.next, &imxdmac->ld_free);
    spin_unlock_irqrestore(&imxdma->lock, flags);
    dmaengine_desc_get_callback_invoke(&desc->desc, NULL);

If another thread calls imxdma_prep_dma_memcpy() concurrently, it locklessly
inspects ld_free. If ld_free was empty, it might immediately pick up this
newly freed descriptor and overwrite its fields (desc->desc.callback = NULL)
before imxdma_tasklet() invokes the callback.

Does this need to hold the descriptor off the free list until after the
callback completes?

[ ... ]

> @@ -1142,7 +1142,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&imxdmac->ld_free);
>  		INIT_LIST_HEAD(&imxdmac->ld_active);
>  
> -		tasklet_setup(&imxdmac->dma_tasklet, imxdma_tasklet);
> +		dmaengine_init_bh(&imxdmac->chan, imxdma_tasklet);
>  		imxdmac->chan.device = &imxdma->dma_device;
>  		dma_cookie_init(&imxdmac->chan);
>  		imxdmac->channel = i;

[Severity: High]
This is a pre-existing issue, but does the interrupt handler get registered
too early in imxdma_probe()?

For IMX1 DMA, devm_request_irq() is called before the interrupt mask register
(DMA_DIMR) is disabled and before channel list heads like ld_active are
initialized.

If an interrupt is pending or asserts immediately, dma_irq_handler() runs
and calls dma_irq_handle_channel(). This evaluates list_empty() on
uninitialized memory, incorrectly returning false, and then dereferences an
invalid pointer from list_first_entry(), causing a crash.

Should the IRQ registration be moved after the hardware masking and list
initializations?

[ ... ]

> @@ -1211,7 +1211,7 @@ static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *
>  		if (!is_imx1_dma(imxdma))
>  			disable_irq(imxdmac->irq);
>  
> -		tasklet_kill(&imxdmac->dma_tasklet);
> +		dmaengine_kill_bh(&imxdmac->chan);
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free with the watchdog timer?

In imxdma_free_irq(), the dmaengine BH is killed, but the imxdmac->watchdog
timer is never safely canceled. When devm frees the imxdma structure, a
still-active timer could fire, executing imxdma_watchdog(). This would
dereference freed memory to call dmaengine_schedule_bh().

Similarly in imxdma_probe(), if devm_request_irq() fails for a later channel,
the error path jumps to disable_dma_ahb_clk and omits timer cleanup for
already initialized channels.

Does this require timer_shutdown_sync() during teardown and error paths?

[Severity: High]
This isn't a bug introduced by this patch, but could descriptors be freed
while the bottom-half work is still running?

When a channel is released, imxdma_free_chan_resources() iterates through
ld_free and frees the descriptors with kfree(). However, the driver does
not implement device_synchronize, and imxdma_free_chan_resources() does
not flush pending bottom-half work.

If imxdma_tasklet() is executing concurrently, imxdma_free_chan_resources()
could acquire the lock, splice the lists, and free the descriptor just as
the tasklet drops the spinlock to invoke the callback. The bottom-half
would then dereference the freed desc->desc to invoke the callback.

Does imxdma_free_chan_resources() need to flush pending BH work before
freeing descriptors?

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