Re: [PATCHv3] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver fails to clean up requested IRQs, IRQ mappings, and platform devices on channel probe error paths and during driver removal, leading to resource leaks and potential use-after-free crashes.
- [High] The driver initializes DMA channels but fails to actually add them to the dma_device's channel list, preventing registration and rendering the driver completely non-functional.
- [High] Failing to read the 'reg' property of a subsequent job queue node causes a direct return, permanently leaking all resources for successfully probed channels from earlier iterations.
- [High] The return value of `dma_async_device_register()` is ignored, which can lead to IDA double-free bugs and kernel crashes during driver removal if registration fails.
- [High] Channel tasklets and DMA descriptor rings are freed before disabling the hardware and unregistering the DMA device, causing a use-after-free race condition during driver removal.
--

--- Patch [1]: [PATCHv3] dmaengine: fsl_raid: check fsl_re_chan_probe() return value ---
commit 28b38bba753033b4ceec348d232f01622ff9794d
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

This commit addresses an issue where fsl_re_probe() ignored the return
value of fsl_re_chan_probe(). By checking the return value and only
counting successfully probed channels, it prevents fsl_re_remove_chan()
from dereferencing a NULL pointer during device removal.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..4c83cde7a7388 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -832,13 +832,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		for_each_child_of_node(np, child) {

[Severity: High]
This isn't a bug introduced by this patch, but is there a resource leak if
multiple job queues are present and a subsequent one fails to probe?

Just above this loop in fsl_re_probe(), the driver iterates over job queue
nodes:

    rc = of_property_read_u32(np, "reg", &off);
    if (rc) {
        dev_err(dev, "Reg property not found in JQ node\n");
        return -ENODEV;
    }

If the driver successfully processes one job queue, and then encounters a
failure reading the "reg" property of the next node, it directly returns
-ENODEV.

Doesn't this bypass the cleanup logic for any channels that were
successfully probed during the earlier iterations?

>  			rc = of_device_is_compatible(child,
>  					     "fsl,raideng-v1.0-job-ring");
> +			if (!rc)
> +				continue;
> +
> +			if (ridx >= FSL_RE_MAX_CHANS) {
> +				dev_warn(dev,
> +					"too many job rings, max %d\n",
> +					FSL_RE_MAX_CHANS);
> +				of_node_put(child);
> +				goto done;
> +			}
> +
> +			rc = fsl_re_chan_probe(ofdev, child, ridx, off);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver ever actually
link the initialized channels into the DMA device's channel list?

Looking at fsl_re_chan_probe(), it initializes the channel:

    chan->chan.device = dma_dev;
    chan->chan.private = chan;

But it appears to never call list_add_tail() to add the channel to
dma_dev->channels.

When dma_async_device_register() iterates over the channel list, won't it
see an empty list and register 0 channels, rendering the driver
non-functional?

>  			if (rc) {
> -				fsl_re_chan_probe(ofdev, child, ridx++, off);
> -				re_priv->total_chans++;
> +				dev_err(dev,
> +					"job ring %d probe failed: %d\n",
> +					ridx, rc);
> +				continue;
>  			}
> +			ridx++;
> +			re_priv->total_chans++;
>  		}
>  	}
>  
> +done:
>  	dma_async_device_register(dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but should the return value of
dma_async_device_register() be checked here?

If registration fails (for example, due to memory allocation failure), the
driver remains bound but its DMA ID is freed internally by the DMA subsystem.

Later, when the driver unbinds, fsl_re_remove() unconditionally calls
dma_async_device_unregister(), which will attempt to free the already-freed
IDA and decrement a zeroed kref.

Could this result in a kernel oops during removal?

>  
>  	return 0;
>  }

[ ... ]

> @@ -866,7 +883,8 @@ static void fsl_re_remove(struct platform_device *ofdev)
>  
>  	/* Cleanup chan related memory areas */
>  	for (i = 0; i < re_priv->total_chans; i++)
> -		fsl_re_remove_chan(re_priv->re_jrs[i]);
> +		if (re_priv->re_jrs[i])
> +			fsl_re_remove_chan(re_priv->re_jrs[i]);

[Severity: High]
This isn't a bug introduced by this patch, but does the driver fail to clean
up requested IRQs and platform devices during removal?

Looking at fsl_re_remove_chan(), it only kills the tasklet and frees the DMA
pools:

static void fsl_re_remove_chan(struct fsl_re_chan *chan)
{
    tasklet_kill(&chan->irqtask);
    dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
                  chan->inb_phys_addr);
    ...
}

It doesn't seem to call free_irq(), irq_dispose_mapping(), or
of_platform_device_destroy(). The error path err_free inside
fsl_re_chan_probe() has the same behavior.

If the module is unloaded, wouldn't the interrupt remain active, potentially
causing a use-after-free in fsl_re_isr() if it fires?

>  
>  	/* Unregister the driver */
>  	dma_async_device_unregister(&re_priv->dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown order in
fsl_re_remove() cause a race condition?

The driver frees the channel DMA memory regions and kills the tasklets in
the loop above before it disables the hardware (via jr_command) and before
it calls dma_async_device_unregister().

If the DMA engine is actively processing rings when removal begins, could it
access the freed physical memory before the device is unregistered?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721221807.664865-1-rosenp@gmail.com?part=1