Re: [PATCH v2 07/64] dmaengine: fsl_raid: run completions via dma_chan BH

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

Pre-existing issues:
- [High] The driver requests hardware interrupts via request_irq() without devres, but completely fails to call free_irq() in both the probe error paths and device removal path.
- [High] The interrupt is requested before the channel's data structures and driver data are fully initialized, leading to a potential NULL pointer dereference upon early interrupts.
- [High] The driver ignores the return value of fsl_re_chan_probe() during initialization, leading to NULL pointer dereferences in the cleanup path when iterating over failed channels.
- [High] Channel memory is freed before the device is unregistered from the DMA engine, allowing concurrent clients to use freed memory.
--

--- Patch [7]: [PATCH v2 07/64] dmaengine: fsl_raid: run completions via dma_chan BH ---
commit ab56ab807ba67c4bf376a52b60efd1139b5215d9
Author: Allen Pais <[email protected]>

dmaengine: fsl_raid: run completions via dma_chan BH

Replace the per-channel tasklet with the shared dma_chan 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
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c

[ ... ]

> @@ -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);
> +	dma_chan_init_bh(&chan->chan, fsl_re_dequeue);
>  
>  	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ before the
channel's driver data and locks are initialized?

If an interrupt fires immediately after request_irq() succeeds, fsl_re_isr()
will be executed:

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

Since dev_set_drvdata() hasn't been called yet, dev_get_drvdata() returns
NULL, which is immediately dereferenced.

[Severity: High]
This is a pre-existing issue, but does this function leak the requested IRQ
on failure?

The driver requests the hardware interrupt without devres. If any subsequent
initialization fails, the function jumps to the err_free labels without
calling free_irq().

>  	if (ret) {

[ ... ]

> @@ -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);
> +	dma_chan_kill_bh(&chan->chan);

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference if the probe previously failed?

In fsl_re_probe(), the driver ignores the return value of
fsl_re_chan_probe():

fsl_re_probe()
    rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
    if (rc) {
        fsl_re_chan_probe(ofdev, child, ridx++, off);
        re_priv->total_chans++;
    }

If a channel fails to probe, its pointer in the re_jrs array remains NULL
but total_chans is still incremented. Later, when cleaning up, could passing
that NULL pointer into fsl_re_remove_chan() and calling dma_chan_kill_bh()
cause a kernel crash?

[Severity: High]
This is a pre-existing issue, but shouldn't there be a free_irq() call here?

Since the IRQ was requested without devres during probe, omitting free_irq()
in the teardown path leaves the IRQ active after the driver memory structures
are freed. If an interrupt occurs later, could it lead to a use-after-free?

[Severity: High]
This is a pre-existing issue, but does the driver free channel memory while
the DMA device is still registered?

Looking at fsl_re_remove(), it iterates over all channels and calls
fsl_re_remove_chan() before unregistering the DMA device:

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 concurrent clients still submit requests while this is happening,
leading fsl_re_issue_pending() to copy data into a freed ring buffer?

>  
>  	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
>  		      chan->inb_phys_addr);

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