dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

PATCH Applied Rosen Penev 4 participants 4 messages Jul 18 – Sep 3 dmaengine

Share & tools

Rosen Penev

author

2026-07-18 23:52 UTC #0 lore raw

PATCH 1/1 dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

fsl_re_free_chan_resources() loops alloc_count times calling list_first_entry() on free_q without checking whether the list still has entries. If descriptors remain un-acked or pending in submit_q / active_q / ack_q when the channel resources are freed, free_q becomes smaller than alloc_count and list_first_entry() returns the list head, causing list_del() to corrupt the list head and dereference a bogus pointer.

Walk free_q with list_for_each_entry_safe() until it is empty, decrementing alloc_count per freed descriptor, instead of relying on the alloc_count count.

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

Assisted-by opencode:hy3-free

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

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

drivers/dma/fsl_raid.c +3 -6

index 30aaca81d855..db86f3e4e460 100644
@@ -614,18 +614,15 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
 static void fsl_re_free_chan_resources(struct dma_chan *chan)
 {
 	struct fsl_re_chan *re_chan;
-	struct fsl_re_desc *desc;
+	struct fsl_re_desc *desc, *_desc;
 
 	re_chan = container_of(chan, struct fsl_re_chan, chan);
-	while (re_chan->alloc_count--) {
-		desc = list_first_entry(&re_chan->free_q,
-					struct fsl_re_desc,
-					node);
-
+	list_for_each_entry_safe(desc, _desc, &re_chan->free_q, node) {
 		list_del(&desc->node);
 		dma_pool_free(re_chan->re_dev->cf_desc_pool, desc->cf_addr,
 			      desc->cf_paddr);
 		kfree(desc);
+		re_chan->alloc_count--;
 	}
 
 	if (!list_empty(&re_chan->free_q))

Signature · 1 line

2.55.0

[email protected]

AI review

2026-07-19 00:05 UTC #1 lore raw

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

New issues:
- [High] The patch avoids a crash by exclusively iterating over free_q, but permanently leaks descriptors in other queues and introduces a dead-code check.

Pre-existing issues:
- [High] Iterating and modifying free_q without holding desc_lock can race with the hardware tasklet, leading to list corruption.
- [High] The driver never frees the requested IRQ, causing an IRQ leak and a subsequent Use-After-Free on driver removal.
- [High] Hardware descriptor rings are destroyed before the DMA device is unregistered, causing memory corruption if clients submit concurrent requests.
- [High] Papering over the crash without halting the underlying hardware risks severe silent DMA memory corruption.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources --- commit 6689b4246e148c9519335c9bfdee192da8124eb9
Author: Rosen Penev <[email protected]>

dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

This commit modifies fsl_re_free_chan_resources() to walk free_q using list_for_each_entry_safe() rather than relying on a fixed count. This prevents a list corruption crash when descriptors are still located in other queues during teardown.

drivers/dma/fsl_raid.c static int fsl_re_alloc_chan_resources(struct dma_chan *chan) · line 604

Quoted 8 lines

drivers/dma/fsl_raid.c +0 -0

index 99945845d8b5e..5d86ff99624c0 100644
@@ -602,18 +602,15 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
 static void fsl_re_free_chan_resources(struct dma_chan *chan)
 {
 	struct fsl_re_chan *re_chan;

[Severity: High]
This is a pre-existing issue, but does this function completely fail to halt the hardware RAID engine?

If a client frees the channel while DMA transactions are still active, the client will unmap its data buffers. Because the RAID engine and its interrupts are not halted here, will it continue performing DMA operations into memory that has been freed or reassigned?

drivers/dma/fsl_raid.c static int fsl_re_alloc_chan_resources(struct dma_chan *chan) · line 620

Quoted 10 lines

drivers/dma/fsl_raid.c +2 -6

-	struct fsl_re_desc *desc;
+	struct fsl_re_desc *desc, *_desc;
 
 	re_chan = container_of(chan, struct fsl_re_chan, chan);
-	while (re_chan->alloc_count--) {
-		desc = list_first_entry(&re_chan->free_q,
-					struct fsl_re_desc,
-					node);
-
+	list_for_each_entry_safe(desc, _desc, &re_chan->free_q, node) {

[Severity: High]
This is a pre-existing issue, but is it safe to iterate and modify free_q using list_del() without acquiring re_chan->desc_lock?

Since the hardware is not halted during teardown, an interrupt could fire and schedule the fsl_re_dequeue tasklet on another CPU. If the tasklet calls fsl_re_cleanup_descs(), which acquires desc_lock and attempts to move completed descriptors to free_q using list_move_tail(), could this concurrent modification corrupt the list pointers?

[Severity: High]
By exclusively iterating over free_q, does this deliberately strand and leak any DMA pool buffers currently sitting in submit_q, active_q, and ack_q?

If a client frees the channel while descriptors remain in those other queues, they are now ignored and the memory is never freed.

drivers/dma/fsl_raid.c static int fsl_re_alloc_chan_resources(struct dma_chan *chan) · line 628

Quoted 8 lines

drivers/dma/fsl_raid.c +1 -0

 		list_del(&desc->node);
 		dma_pool_free(re_chan->re_dev->cf_desc_pool, desc->cf_addr,
 			      desc->cf_paddr);
 		kfree(desc);
+		re_chan->alloc_count--;
 	}
 
 	if (!list_empty(&re_chan->free_q))

[Severity: High]
Because the list_for_each_entry_safe loop above runs until free_q is empty, won't this list_empty() check mathematically always evaluate to false?

This makes the error logging dead code, effectively masking the fact that descriptors were left behind in the other queues.

Quoted 2 lines

dev_err(re_chan->dev, "chan resource cannot be cleaned!\n");
}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the driver leak the IRQ requested in fsl_re_chan_probe()?

Looking at fsl_re_chan_probe(), an interrupt is requested using request_irq(), but neither fsl_re_remove_chan() nor fsl_re_remove() calls free_irq().

Because the re_chan structure is allocated with devm_kzalloc() on the parent device, its memory is automatically freed when the device unbinds. If an interrupt fires after the driver is unloaded, will the kernel execute the registered fsl_re_isr and dereference the freed re_chan structure?

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove() destroy resources in the wrong order?

Looking at fsl_re_remove(), it destroys the hardware descriptor queues by calling fsl_re_remove_chan() before calling dma_async_device_unregister():

drivers/dma/fsl_raid.c: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);
    ...
}

By destroying the underlying DMA rings while the device is still visible to the core framework, will any active or racing DMA transfers attempt to write to freed memory pools?

Signature · 1 line

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

Frank Li

Reviewed-by on v1 1/1

2026-07-20 15:57 UTC #2 lore raw

In reply to Rosen Penev

Quoted 8 lines · Rosen Penev

fsl_re_free_chan_resources() loops alloc_count times calling list_first_entry() on free_q without checking whether the list still has entries. If descriptors remain un-acked or pending in submit_q / active_q / ack_q when the channel resources are freed, free_q becomes smaller than alloc_count and list_first_entry() returns the list head, causing list_del() to corrupt the list head and dereference a bogus pointer.

Quoted 8 lines

Walk free_q with list_for_each_entry_safe() until it is empty, decrementing alloc_count per freed descriptor, instead of relying on the alloc_count count.
Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>
---

Reviewed-by Frank Li <[email protected]>

Quoted 32 lines

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

drivers/dma/fsl_raid.c +3 -7

index 30aaca81d855..db86f3e4e460 100644
@@ -614,18 +614,15 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
 static void fsl_re_free_chan_resources(struct dma_chan *chan)
 {
 	struct fsl_re_chan *re_chan;
-	struct fsl_re_desc *desc;
+	struct fsl_re_desc *desc, *_desc;
 
 	re_chan = container_of(chan, struct fsl_re_chan, chan);
-	while (re_chan->alloc_count--) {
-		desc = list_first_entry(&re_chan->free_q,
-					struct fsl_re_desc,
-					node);
-
+	list_for_each_entry_safe(desc, _desc, &re_chan->free_q, node) {
 		list_del(&desc->node);
 		dma_pool_free(re_chan->re_dev->cf_desc_pool, desc->cf_addr,
 			      desc->cf_paddr);
 		kfree(desc);
+		re_chan->alloc_count--;
 	}
 
 	if (!list_empty(&re_chan->free_q))
--

2.55.0

Vinod Koul

maintainer Applied

2026-09-03 17:04 UTC #3 lore raw

In reply to Rosen Penev

Quoted 9 lines · Rosen Penev

fsl_re_free_chan_resources() loops alloc_count times calling list_first_entry() on free_q without checking whether the list still has entries. If descriptors remain un-acked or pending in submit_q / active_q / ack_q when the channel resources are freed, free_q becomes smaller than alloc_count and list_first_entry() returns the list head, causing list_del() to corrupt the list head and dereference a bogus pointer.
[...]

Applied, thanks!

[1/1] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

      commit: d805aa5b6088b193debabaa80cc0991bfa2bd057

Best regards,

Signature · 1 line

~Vinod