Re: [PATCH v3 07/34] dmaengine: fsl_raid: run completions via dmaengine BH

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

Pre-existing issues:
- [High] The driver requests an IRQ using `request_irq()` but never frees it on module unload or error paths, leading to a resource leak and potential Use-After-Free.
- [High] The IRQ handler is registered before the channel's critical state (locks, lists, and driver private data) is initialized, creating a race window for early interrupts.
- [High] Probe error handling in `fsl_re_probe()` ignores failures from `fsl_re_chan_probe()`, leaving partial state that causes NULL pointer dereferences and double-frees during device removal.
- [High] Channels are torn down and their memory pools freed before the DMA device is unregistered, creating a race window for use-after-free.
- [High] The RAID hardware engine is not explicitly stopped during channel removal, resulting in DMA continuing into freed memory.
- [High] The driver holds the internal channel spinlock (`desc_lock`) while synchronously invoking the client's DMA completion callback, leading to a potential AA deadlock.
--

--- Patch [7]: [PATCH v3 07/34] dmaengine: fsl_raid: run completions via dmaengine BH ---
commit 12282b5227658b803a18693207b1136ab132efae
Author: Allen Pais <[email protected]>

dmaengine: fsl_raid: run completions via 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/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..0d0403945d91a 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -155,9 +155,10 @@ static void fsl_re_cleanup_descs(struct fsl_re_chan *re_chan)
>  	fsl_re_issue_pending(&re_chan->chan);
>  }
>  
> -static void fsl_re_dequeue(struct tasklet_struct *t)
> +static void fsl_re_dequeue(struct dma_chan *chan)
>  {
> -	struct fsl_re_chan *re_chan = from_tasklet(re_chan, t, irqtask);
> +	struct fsl_re_chan *re_chan = container_of(chan, struct fsl_re_chan,
> +						   chan);

[Severity: High]
This isn't a bug introduced by this patch, but I spotted a potential
deadlock in the completion path.

When fsl_re_dequeue() is running, it acquires the channel's desc_lock:

fsl_re_dequeue() {
    ...
    spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
    if (found) {
        fsl_re_desc_done(desc);
    ...

It then calls fsl_re_desc_done(desc), which invokes the client callback.

If the client calls dma_async_issue_pending() from within that callback,
fsl_re_issue_pending() will try to acquire the exact same spinlock:

fsl_re_issue_pending() {
    ...
    spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

Could this sequence trigger an AA deadlock?

[ ... ]
> @@ -670,7 +671,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  	snprintf(chan->name, sizeof(chan->name), "re_jr%02d", q);
>  
>  	chandev = &chan_ofdev->dev;
> -	tasklet_setup(&chan->irqtask, fsl_re_dequeue);
> +	dmaengine_init_bh(&chan->chan, fsl_re_dequeue);
>  
>  	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
>  	if (ret) {

[Severity: High]
This is a pre-existing issue, but I noticed the driver requests an IRQ
using request_irq(), but it appears there's no matching free_irq() call
on the module unload or error paths.

If the module is unbound, will the IRQ handler remain registered and
potentially dereference a freed chan pointer when it fires?

[Severity: High]
This is also a pre-existing issue, but I noticed the IRQ handler is
registered here before the channel's critical state (locks, lists, and
driver private data) is fully initialized.

If an interrupt fires immediately after request_irq() completes:

fsl_re_isr() {
    re_chan = dev_get_drvdata((struct device *)data);
    irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
}

Could this result in a NULL pointer dereference since dev_set_drvdata()
hasn't been called yet?

[ ... ]
> @@ -846,7 +847,7 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  
>  static void fsl_re_remove_chan(struct fsl_re_chan *chan)
>  {
> -	tasklet_kill(&chan->irqtask);
> +	dmaengine_kill_bh(&chan->chan);
>  
>  	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
>  		      chan->inb_phys_addr);

[Severity: High]
This is a pre-existing issue, but fsl_re_remove_chan() frees the hardware
descriptor rings without explicitly stopping the RAID hardware engine first.

Is there a risk that the hardware continues executing pending transactions
into memory that has already been returned to the pool, potentially causing
memory corruption?

Should we write a disable command to the jr_command register before freeing
the rings?

[Severity: High]
Another pre-existing issue, but fsl_re_probe() doesn't check the return
value of fsl_re_chan_probe():

fsl_re_probe() {
    ...
    fsl_re_chan_probe(ofdev, child, ridx++, off);
    re_priv->total_chans++;
    ...
}

If fsl_re_chan_probe() fails, the channel state remains partially
initialized. During module removal, fsl_re_remove() iterates up to
total_chans:

fsl_re_remove() {
    ...
    for (i = 0; i < re_priv->total_chans; i++)
        fsl_re_remove_chan(re_priv->re_jrs[i]);
    ...
}

Could this lead to double-frees or NULL pointer dereferences when
fsl_re_remove_chan() operates on these failed channels?

[Severity: High]
Finally, this is a pre-existing issue, but I noticed channels are completely
torn down before the DMA device is unregistered in fsl_re_remove():

fsl_re_remove() {
    ...
    for (i = 0; i < re_priv->total_chans; i++)
        fsl_re_remove_chan(re_priv->re_jrs[i]);

    dma_async_device_unregister(&re_priv->dma_dev);
    ...
}

Could the dmaengine core or clients still attempt to submit transfers to
channels that are actively being destroyed? Should
dma_async_device_unregister() happen before the resources are torn down?

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