Skip to contentSkip to conversation

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

PATCH Superseded Rosen Penev 3 participants 3 messages Sep 16 – Sep 17 dmaengine

v6 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

PATCH v6 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 LLM

---
v6: increment ridx before continue;
v5: put some stuff on one line for easier readability.
v4: use break;
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
drivers/dma/fsl_raid.c +21 -8
index 9cc289f7129e..f059ff2e37f8 100644
@@ -836,18 +836,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
-			rc = of_device_is_compatible(child,
-					     "fsl,raideng-v1.0-job-ring");
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
+			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
+			if (!rc) {
+				ridx++;
+				continue;
+			}
+
+			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);
+				ridx++;
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)
@@ -872,7 +884,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
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] dmaengine: fsl_raid: decoupling total_chans from ridx leaks channels on remove - [Medium]…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: fsl_raid: decoupling total_chans from ridx leaks channels on remove
- [Medium] dmaengine: fsl_raid: unconditionally incrementing ridx for incompatible nodes exhausts slots --

commit 698d683c6c412a8530d330dc178b5712397be8bf
Author: Rosen Penev <[email protected]>

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

This commit modifies fsl_re_probe to check the return value of fsl_re_chan_probe and only increment total_chans on success. It also adds NULL checks during channel removal in fsl_re_remove to prevent dereferencing uninitialized slots.

Quoted 11 lines
drivers/dma/fsl_raid.c +1 -2
index 2d86f61105e52..83856d554b12b 100644
@@ -828,18 +828,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
-			rc = of_device_is_compatible(child,
-					     "fsl,raideng-v1.0-job-ring");
+			if (ridx >= FSL_RE_MAX_CHANS) {

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

Quoted 10 lines
drivers/dma/fsl_raid.c +10 -0
+				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
+			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
+			if (!rc) {
+				ridx++;
+				continue;
+			}

[Severity: Medium]
When iterating child nodes, if a node is not compatible, does this code incorrectly increment ridx?

This could waste limited job ring capacity (FSL_RE_MAX_CHANS) by advancing the internal logical index for child nodes that are not job rings.

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

Quoted 11 lines
drivers/dma/fsl_raid.c +7 -2
+
+			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);
+				ridx++;
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;

[Severity: High]
If any child node fails to probe or is skipped as incompatible, ridx is incremented but total_chans is not. Does this create a persistent mismatch between the array index and the channel count?

A subsequent successful probe would store the channel in re_priv->re_jrs at an index greater than or equal to total_chans.

[ ... ]

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

Quoted 7 lines
drivers/dma/fsl_raid.c +2 -1
@@ -864,7 +876,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]
Does this cleanup loop miss successfully populated channels stored at indices greater than or equal to total_chans?

Since total_chans is used as the exclusive upper bound, successfully probed channels placed at higher indices (due to previous skipped or failed nodes) could be bypassed and leaked during device removal.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#2 lore raw
In reply to Rosen Penev
Quoted 26 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 LLM

---
v6: increment ridx before continue;
v5: put some stuff on one line for easier readability.
v4: use break;
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
drivers/dma/fsl_raid.c +0 -0
index 9cc289f7129e..f059ff2e37f8 100644

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

Quoted 3 lines
drivers/dma/fsl_raid.c +0 -0
@@ -836,18 +836,30 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {

look like you need rebase, latest code

use for_each_compatible_node_scoped()

Frank

Quoted 49 lines
drivers/dma/fsl_raid.c +19 -7
-			rc = of_device_is_compatible(child,
-					     "fsl,raideng-v1.0-job-ring");
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev, "too many job rings, max %d\n", FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
+			rc = of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring");
+			if (!rc) {
+				ridx++;
+				continue;
+			}
+
+			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);
+				ridx++;
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;
 
 
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }

static void fsl_re_remove_chan(struct fsl_re_chan *chan)

drivers/dma/fsl_raid.c +2 -2
@@ -872,7 +884,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