Skip to contentSkip to conversation

dmaengine: mv_xor: return descriptor to free pool on io_win failure

PATCH Applied Rosen Penev 4 participants 4 messages Sep 9 – Sep 10 dmaengine

lore
Share & tools
lore

Rosen Penev

author
#0 lore raw

PATCH 1/1 dmaengine: mv_xor: return descriptor to free pool on io_win failure

When mv_xor_prep_dma_xor() allocates a descriptor slot and then mv_xor_add_io_win() fails in the source-address loop, the slot stays on the allocated list and is never reclaimed, leaking a descriptor resource.

Return the slot to the free pool under the channel lock before returning NULL on failure.

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by opencode:big-pickle
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +6 -1
index 25ed61f1b089..dd4f150d5fb1 100644
@@ -583,8 +583,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
 		while (src_cnt--) {
 			/* Check if a new window needs to get added for 'src' */
 			ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
-			if (ret)
+			if (ret) {
+				spin_lock_bh(&mv_chan->lock);
+				list_move_tail(&sw_desc->node,
+					       &mv_chan->free_slots);
+				spin_unlock_bh(&mv_chan->lock);
 				return NULL;
+			}
 			mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
 		}
 	}
Signature · 1 line
2.55.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

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

Pre-existing issues:
- [High] Concurrent access to shared MBUS window configuration in mv_xor_add_io_win without synchronization leads to arbitrary DMA memory corruption.
- [High] Missing tasklet_kill() during channel removal leads to Use-After-Free.
- [High] free_irq() is called after freeing DMA descriptors, risking Use-After-Free in the interrupt handler.
--

commit 8b478eae94fa4a954430bcc670fc167e9a399a15
Author: Rosen Penev <[email protected]>

dmaengine: mv_xor: return descriptor to free pool on io_win failure

When mv_xor_prep_dma_xor() allocates a descriptor slot and then mv_xor_add_io_win() fails in the source-address loop, the slot stays on the allocated list and is never reclaimed. This returns the slot to the free pool under the channel lock before returning NULL on failure.

drivers/dma/mv_xor.c mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src, · line 585

Quoted 8 lines
drivers/dma/mv_xor.c +0 -0
index 25ed61f1b0899..dd4f150d5fb12 100644
@@ -583,8 +583,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
 		while (src_cnt--) {
 			/* Check if a new window needs to get added for 'src' */
 			ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);

[Severity: High]
This isn't a bug introduced by this patch, but can this concurrent access corrupt the MBUS window mappings?

mv_xor_prep_dma_xor() is called concurrently on multiple channels sharing the same XOR device. It calls mv_xor_add_io_win() which performs an unprotected read:

	win_enable = readl(base + WINDOW_BAR_ENABLE(0));

It then modifies the shared device-level window cache:

	xordev->win_start[i] = addr;
	xordev->win_end[i] = addr + size;

And writes the clobbered value back to the hardware:

	writel(win_enable, base + WINDOW_BAR_ENABLE(0));

Since this lacks synchronization across channels, could this race condition lead to the DMA engine accessing incorrect physical addresses?

drivers/dma/mv_xor.c mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src, · line 595

Quoted 11 lines
drivers/dma/mv_xor.c +6 -1
-			if (ret)
+			if (ret) {
+				spin_lock_bh(&mv_chan->lock);
+				list_move_tail(&sw_desc->node,
+					       &mv_chan->free_slots);
+				spin_unlock_bh(&mv_chan->lock);
 				return NULL;
+			}
 			mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
 		}
 	}

[Severity: High]
This is a pre-existing issue, but does the channel teardown leak the pending background execution context?

Looking at mv_xor_channel_remove(), it frees the interrupt handler and DMA device but neglects to call tasklet_kill(&mv_chan->irq_tasklet):

static int mv_xor_channel_remove(struct mv_xor_chan *mv_chan)

{
	struct dma_chan *chan, *_chan;
	[ ... ]

Since free_irq() does not wait for tasklets to finish, can a pending tasklet execute asynchronously after mv_xor_channel_remove() returns and devres has freed the mv_chan memory, resulting in a use-after-free?

[Severity: High]
This is a pre-existing issue, but does the teardown order risk a use-after-free in the interrupt handler?

In mv_xor_channel_remove(), the DMA memory used by the hardware and IRQ handler is freed here:

	dma_free_wc(dev, MV_XOR_POOL_SIZE,
			  mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);

The interrupt handler is freed later:

	free_irq(mv_chan->irq, mv_chan);

If a hardware interrupt fires between these two calls, or is already running on another CPU, wouldn't it trigger the interrupt handler and subsequently schedule the tasklet?

This would access the already-freed descriptor memory (mv_chan->dma_desc_pool_virt). Should the teardown order disable and free IRQs before freeing the resources the IRQ handler relies on?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Rosen Penev
Quoted 5 lines · Rosen Penev

When mv_xor_prep_dma_xor() allocates a descriptor slot and then mv_xor_add_io_win() fails in the source-address loop, the slot stays on the allocated list and is never reclaimed, leaking a descriptor resource.

Quoted 7 lines

Return the slot to the free pool under the channel lock before returning NULL on failure.

Fixes 77ff7a706f01 ("mv_xor: Add support for IO (PCIe) src/dst areas")
Assisted-by opencode:big-pickle
Signed-off-by Rosen Penev <[email protected]>

---

Reviewed-by Frank Li <[email protected]>
Quoted 25 lines
 drivers/dma/mv_xor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +6 -2
index 25ed61f1b089..dd4f150d5fb1 100644
@@ -583,8 +583,13 @@ mv_xor_prep_dma_xor(struct dma_chan *chan, dma_addr_t dest, dma_addr_t *src,
 		while (src_cnt--) {
 			/* Check if a new window needs to get added for 'src' */
 			ret = mv_xor_add_io_win(mv_chan, src[src_cnt]);
-			if (ret)
+			if (ret) {
+				spin_lock_bh(&mv_chan->lock);
+				list_move_tail(&sw_desc->node,
+					       &mv_chan->free_slots);
+				spin_unlock_bh(&mv_chan->lock);
 				return NULL;
+			}
 			mv_desc_set_src_addr(sw_desc, src_cnt, src[src_cnt]);
 		}
 	}
--

2.55.0

maintainer Applied
#3 lore raw
In reply to Rosen Penev
Quoted 9 lines · Rosen Penev

When mv_xor_prep_dma_xor() allocates a descriptor slot and then mv_xor_add_io_win() fails in the source-address loop, the slot stays on the allocated list and is never reclaimed, leaking a descriptor resource.

Return the slot to the free pool under the channel lock before returning NULL on failure.

[...]

Applied, thanks!

[1/1] dmaengine: mv_xor: return descriptor to free pool on io_win failure

      commit: 5d5f4b3407d6ec86e76d094c332301135f493636

Best regards,

Signature · 1 line
~Vinod