dmaengine: fsl_raid: check fsl_re_chan_probe() return value

PATCH Superseded Rosen Penev 3 participants 4 messages Jul 21 – Jul 22 dmaengine

Share & tools

Rosen Penev

author

2026-07-21 22:18 UTC #0 lore raw

PATCH v3 1/1 dmaengine: fsl_raid: check fsl_re_chan_probe() return value

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.

Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")

Assisted-by opencode:hy3-free

Signed-off-by Rosen Penev <[email protected]>

---
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/dma/fsl_raid.c +21 -3

index bfaef6245695..66de27eb4365 100644
@@ -841,13 +841,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		for_each_child_of_node(np, child) {
 			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);
 			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);
 
 	return 0;

@@ -875,7 +892,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-21 22:29 UTC #1 lore raw

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 <[email protected]>

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.

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 832

Quoted 6 lines

drivers/dma/fsl_raid.c +0 -0

index 99945845d8b5e..4c83cde7a7388 100644
@@ -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?

Quoted 4 lines

drivers/dma/fsl_raid.c +2 -0

 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");
+			if (!rc)
+				continue;

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 855

Quoted 10 lines

drivers/dma/fsl_raid.c +10 -0

+
+			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?

Quoted 5 lines

drivers/dma/fsl_raid.c +2 -2

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

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 868

Quoted 10 lines

drivers/dma/fsl_raid.c +5 -0

+					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?

Quoted 3 lines

drivers/dma/fsl_raid.c +0 -0
 	return 0;
 }

[ ... ]

drivers/dma/fsl_raid.c static void fsl_re_remove(struct platform_device *ofdev) · line 887

Quoted 7 lines

drivers/dma/fsl_raid.c +2 -1

@@ -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?

drivers/dma/fsl_raid.c static void fsl_re_remove(struct platform_device *ofdev) · line 899

Quoted 3 lines

drivers/dma/fsl_raid.c +0 -0

 	/* 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?

Quoted 1 line

drivers/dma/fsl_raid.c +0 -0
 }

Signature · 1 line

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

Frank Li

2026-07-22 00:00 UTC #2 lore raw

In reply to Rosen Penev

Quoted 25 lines · Rosen Penev

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.
Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.
Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>
---
v3: fix sashiko review
v2: fix description
 drivers/dma/fsl_raid.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
drivers/dma/fsl_raid.c +0 -0
index bfaef6245695..66de27eb4365 100644
@@ -841,13 +841,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		for_each_child_of_node(np, child) {
 			rc = of_device_is_compatible(child,

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 852

Quoted 10 lines

drivers/dma/fsl_raid.c +9 -0

 					     "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;

does break work here?

Frank

Quoted 33 lines

drivers/dma/fsl_raid.c +12 -4

+			}
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
 			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);
 
 	return 0;

@@ -875,7 +892,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
--

2.55.0

Rosen Penev

author

2026-07-22 00:38 UTC #3 lore raw

Quoted 31 lines · Frank Li

Quoted 29 lines · Rosen Penev
fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.
Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.
Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>
---
v3: fix sashiko review
v2: fix description
 drivers/dma/fsl_raid.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
drivers/dma/fsl_raid.c +3 -0
index bfaef6245695..66de27eb4365 100644
@@ -841,13 +841,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
              for_each_child_of_node(np, child) {
                      rc = of_device_is_compatible(child,
                                           "fsl,raideng-v1.0-job-ring");
+                     if (!rc)
+                             continue;
+

Quoted 8 lines

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 852

Quoted 6 lines

drivers/dma/fsl_raid.c +6 -0

+                     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;

does break work here?

Oh I see what's going on. A different commit I have adds another goto label in this function. I'll rework.

Quoted 35 lines

Frank

Quoted 33 lines

drivers/dma/fsl_raid.c +12 -4

+                     }
+
+                     rc = fsl_re_chan_probe(ofdev, child, ridx, off);
                      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);
 
      return 0;

@@ -875,7 +892,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]);
 
      /* Unregister the driver */
      dma_async_device_unregister(&re_priv->dma_dev);
--

2.55.0