[PATCH] dmaengine: fsl_raid: don't invoke client callback under desc_lock

[PATCH] dmaengine: fsl_raid: don't invoke client callback under desc_lock

[Rosen Penev]

fsl_re_dequeue() holds re_chan->desc_lock while calling
fsl_re_desc_done(), which synchronously invokes the DMA client completion
callback via dmaengine_desc_get_callback_invoke(). If that callback
submits new work (e.g. fsl_re_tx_submit()), it tries to reacquire the
same desc_lock and deadlocks on the spinlock.

Collect completed descriptors into a local list under the lock, then
release the lock and invoke the callbacks before moving the descriptors
to the ack queue.

Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/fsl_raid.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 2d86f61105e5..bfaef6245695 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -162,6 +162,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 	struct fsl_re_hw_desc *hwdesc;
 	unsigned long flags;
 	unsigned int count, oub_count;
+	LIST_HEAD(completed);
 	int found;
 
 	fsl_re_cleanup_descs(re_chan);
@@ -182,8 +183,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 		}
 
 		if (found) {
-			fsl_re_desc_done(desc);
-			list_move_tail(&desc->node, &re_chan->ack_q);
+			list_move_tail(&desc->node, &completed);
 		} else {
 			dev_err(re_chan->dev,
 				"found hwdesc not in sw queue, discard it\n");
@@ -196,6 +196,17 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 			 FSL_RE_RMVD_JOB(1));
 	}
 	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
+
+	/* Invoke the client callbacks outside the channel lock. The callback
+	 * may submit new work which re-acquires desc_lock, so holding it here
+	 * would deadlock.
+	 */
+	list_for_each_entry_safe(desc, _desc, &completed, node) {
+		fsl_re_desc_done(desc);
+		spin_lock_irqsave(&re_chan->desc_lock, flags);
+		list_move_tail(&desc->node, &re_chan->ack_q);
+		spin_unlock_irqrestore(&re_chan->desc_lock, flags);
+	}
 }
 
 /* Per Job Ring interrupt handler */
-- 
2.55.0

Re: [PATCH] dmaengine: fsl_raid: don't invoke client callback under desc_lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Unconditional list extraction without list_empty checks in fsl_re_free_chan_resources() causes list corruption and kernel panic.
- [High] fsl_re_desc_done() marks the DMA cookie complete before unmapping the DMA buffer, causing a TOCTOU race condition.
- [High] Tearing down channel ring resources before unregistering the DMA device in fsl_re_remove() leads to a Use-After-Free.
- [High] Native CPU variables are directly assigned to __be32 hardware descriptor fields without endianness conversions.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: don't invoke client callback under desc_lock ---
commit 1f02130ec4e2af9477cecbe2f9da0fc239f69864
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: fsl_raid: don't invoke client callback under desc_lock

This patch modifies the descriptor dequeue process to collect completed
descriptors into a local list under the lock. It then releases the lock and
invokes the callbacks before moving the descriptors to the ack queue, which
prevents deadlocks when new work is submitted.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
[ ... ]
> @@ -196,6 +196,17 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  			 FSL_RE_RMVD_JOB(1));
>  	}
>  	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
> +
> +	/* Invoke the client callbacks outside the channel lock. The callback
> +	 * may submit new work which re-acquires desc_lock, so holding it here
> +	 * would deadlock.
> +	 */
> +	list_for_each_entry_safe(desc, _desc, &completed, node) {
> +		fsl_re_desc_done(desc);

[Severity: High]
This is a pre-existing issue, but does fsl_re_desc_done() signal completion
too early?

Looking at fsl_re_desc_done(), it marks the DMA cookie complete before
unmapping the memory:

drivers/dma/fsl_raid.c:fsl_re_desc_done() {
	dma_cookie_complete(&desc->async_tx);
	dma_descriptor_unmap(&desc->async_tx);
	dmaengine_desc_get_callback_invoke(&desc->async_tx, NULL);
}

If a client polling for completion observes the complete cookie on another
CPU, could it access the destination buffer before the cache invalidation
or bounce buffer sync completes?

> +		spin_lock_irqsave(&re_chan->desc_lock, flags);
> +		list_move_tail(&desc->node, &re_chan->ack_q);
> +		spin_unlock_irqrestore(&re_chan->desc_lock, flags);
> +	}
>  }

[Severity: High]
This is a pre-existing issue, but can fsl_re_free_chan_resources() crash if
the free_q empties prematurely?

When a client releases a channel with pending or unacknowledged descriptors,
the loop executes alloc_count times without checking if the list is empty:

drivers/dma/fsl_raid.c:fsl_re_free_chan_resources() {
	...
	while (re_chan->alloc_count--) {
		desc = list_first_entry(&re_chan->free_q,
					struct fsl_re_desc,
					node);
	...
}

Does this result in an invalid pointer dereference and list corruption when
list_first_entry() returns the list head itself?

[Severity: High]
This is a pre-existing issue, but does driver removal risk a use-after-free
if a DMA client is still active?

During unbind, fsl_re_remove() destroys the channel rings before
unregistering the DMA device:

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]);

	/* Unregister the driver */
	dma_async_device_unregister(&re_priv->dma_dev);
	...
}

If a client issues pending requests in this window, could
fsl_re_issue_pending() copy descriptor structures into the freed ring buffer?

[Severity: High]
This is a pre-existing issue, but does this driver miss endianness conversions
on hardware structures?

Both fill_cfd_frame() and fsl_re_init_desc() directly assign native CPU
variables to __be32 hardware fields:

drivers/dma/fsl_raid.c:fill_cfd_frame() {
	...
	cf[index].efrl32 = efrl;
	cf[index].addr_high = upper_32_bits(addr);
	cf[index].addr_low = lower_32_bits(addr);
	...
}

drivers/dma/fsl_raid.c:fsl_re_init_desc() {
	...
	desc->hwdesc.lbea32 = upper_32_bits(paddr);
	desc->hwdesc.addr_low = lower_32_bits(paddr);
	...
}

Will this cause hardware faults or silent data corruption when executed on a
Little-Endian architecture?

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

Re: [PATCH] dmaengine: fsl_raid: don't invoke client callback under desc_lock

[Frank Li]

On Fri, Jul 17, 2026 at 10:30:10PM -0700, Rosen Penev wrote:
> fsl_re_dequeue() holds re_chan->desc_lock while calling
> fsl_re_desc_done(), which synchronously invokes the DMA client completion
> callback via dmaengine_desc_get_callback_invoke(). If that callback
> submits new work (e.g. fsl_re_tx_submit()), it tries to reacquire the
> same desc_lock and deadlocks on the spinlock.
>
> Collect completed descriptors into a local list under the lock, then
> release the lock and invoke the callbacks before moving the descriptors
> to the ack queue.
>
> Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
> Assisted-by: opencode:hy3-free
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/fsl_raid.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e5..bfaef6245695 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -162,6 +162,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  	struct fsl_re_hw_desc *hwdesc;
>  	unsigned long flags;
>  	unsigned int count, oub_count;
> +	LIST_HEAD(completed);
>  	int found;
>
>  	fsl_re_cleanup_descs(re_chan);
> @@ -182,8 +183,7 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  		}
>
>  		if (found) {
> -			fsl_re_desc_done(desc);
> -			list_move_tail(&desc->node, &re_chan->ack_q);
> +			list_move_tail(&desc->node, &completed);
>  		} else {
>  			dev_err(re_chan->dev,
>  				"found hwdesc not in sw queue, discard it\n");
> @@ -196,6 +196,17 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
>  			 FSL_RE_RMVD_JOB(1));
>  	}
>  	spin_unlock_irqrestore(&re_chan->desc_lock, flags);
> +
> +	/* Invoke the client callbacks outside the channel lock. The callback
> +	 * may submit new work which re-acquires desc_lock, so holding it here
> +	 * would deadlock.
> +	 */
> +	list_for_each_entry_safe(desc, _desc, &completed, node) {
> +		fsl_re_desc_done(desc);
> +		spin_lock_irqsave(&re_chan->desc_lock, flags);
> +		list_move_tail(&desc->node, &re_chan->ack_q);
> +		spin_unlock_irqrestore(&re_chan->desc_lock, flags);
> +	}
>  }
>
>  /* Per Job Ring interrupt handler */
> --
> 2.55.0
>