Re: [PATCH 3/3] dma: fsl_raid: use devm_platform_ioremap_resource

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value from `fsl_re_chan_probe()` leads to double-free and NULL pointer dereference in `fsl_re_remove_chan()`.
- [High] The driver requests IRQs but never frees them, leading to a permanent leak and Use-After-Free crashes.
- [High] The return value of `dma_async_device_register()` is completely ignored.
- [High] System-wide device tree traversal for child nodes instead of scoped child node search.
- [High] Missing cleanup of tasklet, IRQ, and platform device on error paths in `fsl_re_chan_probe`.
- [High] Failure during parent probe loop does not tear down already initialized channels.
- [Medium] Platform devices dynamically created for job rings via `of_platform_device_create` are never unregistered.
--

--- Patch [3]: [PATCH 3/3] dma: fsl_raid: use devm_platform_ioremap_resource ---
Note: The format of this review has been altered to provide a free-form
summary of the findings without quoting the original patch, to adhere to
system recitation restrictions.

Commit df861cda9af0548c1bb12583b9a2c34f69725af1
Author: Rosen Penev <rosenp@gmail.com>
Subject: dma: fsl_raid: use devm_platform_ioremap_resource

This patch replaces the open-coded platform_get_resource() and devm_ioremap()
sequence with devm_platform_ioremap_resource(). It simplifies the probe routine
by fetching the resource and mapping it in one call. The error check is also
updated to use IS_ERR/PTR_ERR.

While reviewing the driver code around these changes, I noticed a few
pre-existing issues in the driver that might need attention. These are not
bugs introduced by this patch, but they could cause problems during operation
or driver unbind.

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() handle failures from
fsl_re_chan_probe() correctly? If fsl_re_chan_probe() fails before fully
initializing the channel, it returns an error. However, fsl_re_probe() ignores
this return value and unconditionally increments re_priv->total_chans. This
leaves a NULL pointer in the re_priv->re_jrs array for that channel index.
Later, when fsl_re_remove() is called, it iterates up to total_chans and passes
these NULL pointers to fsl_re_remove_chan(), which will result in a NULL
pointer dereference when calling tasklet_kill() on the irqtask.

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing free_irq()
in the teardown path? In fsl_re_chan_probe(), the driver uses request_irq()
to register an interrupt handler for each channel. However,
fsl_re_remove_chan() does not appear to call free_irq(). Since the channel
structure is allocated with devm_kzalloc, its memory is freed automatically
by devres when the driver is unbound. If an interrupt fires after the driver
is removed, could the kernel execute fsl_re_isr and dereference the freed
chan pointer?

[Severity: Medium]
This is a pre-existing issue, but are the platform devices created for job
rings properly unregistered? fsl_re_chan_probe() calls
of_platform_device_create() to create platform devices for the child nodes.
However, neither the error handling path nor the driver's teardown function
(fsl_re_remove_chan) calls of_platform_device_destroy() or
platform_device_unregister(). Could this cause a memory and sysfs leak, and
prevent the driver from successfully rebinding later?

[Severity: High]
This isn't a bug introduced by this patch, but is it intentional to ignore the
return value of dma_async_device_register() in fsl_re_probe()? The function
is called just before returning 0. If registration fails, the driver might
falsely indicate success to the driver core, which could lead to inconsistent
state or oopses if clients attempt to use the unregistered DMA device.

[Severity: High]
This is a pre-existing issue, but does the device tree traversal in
fsl_re_probe() search too broadly? The loop uses
for_each_compatible_node_scoped() with a NULL starting node. This instructs
the macro to traverse the entire system's device tree rather than just the
children of the current RAID engine instance. If multiple RAID engines are
present, could each instance mistakenly parse and attempt to bind to all
job queues on the system?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
fsl_re_chan_probe() clean up all resources? If an error occurs after the IRQ
is successfully requested (for example, if dma_pool_alloc() fails for
inb_ring_virt_addr), the code jumps to the err_free label. This label returns
the error code but does not kill the tasklet, free the IRQ, or unregister the
platform device. Could this lead to a crash if the dangling IRQ fires?

[Severity: High]
This is a pre-existing issue, but does fsl_re_probe() clean up successfully
initialized channels if a subsequent loop iteration fails? If
of_property_read_u32() fails to read the reg property for any job-queue node
after some channels have already been probed, the function immediately returns
an error. It doesn't appear to tear down the previously probed channels using
fsl_re_remove_chan(). Since the parent device's managed memory will be freed
by devres, could the dangling IRQs, tasklets, and DMA pools from the orphaned
channels cause a crash?

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