* Re: [PATCH] dmaengine: fsl_raid: zero CF descriptor pool allocations
2026-08-20 3:16 [PATCH] dmaengine: fsl_raid: zero CF descriptor pool allocations Rosen Penev
@ 2026-08-20 3:28 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-20 3:28 UTC (permalink / raw)
To: Rosen Penev; +Cc: dmaengine, vkoul, Frank.Li
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] When reusing a descriptor from `free_q`, `fsl_re_chan_alloc_desc()` fails to clear the residual state from the previous transaction (e.g., `async_tx.callback`, `async_tx.callback_param`).
- [High] `fsl_re_free_chan_resources()` blindly loops `alloc_count` times without verifying list emptiness, causing list corruption. Furthermore, a post-decrement on `alloc_count` causes integer underflow, leaking descriptors on subsequent allocations.
- [High] `fsl_re_dequeue()` invokes client callbacks while holding a spinlock with interrupts disabled, causing an AA deadlock if the client submits a new transaction.
- [High] The driver teardown and probe error paths completely fail to free the requested IRQ, and teardown frees DMA memory before unregistering the dmaengine device.
--
--- Patch [1]: [PATCH] dmaengine: fsl_raid: zero CF descriptor pool allocations ---
commit 0eda1d4c582e7fd0cd8ab28917a580d65f3d2988
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: fsl_raid: zero CF descriptor pool allocations
This patch replaces dma_pool_alloc() with dma_pool_zalloc() to ensure
compound frame descriptors and embedded CDBs are initialized to zero.
This prevents exposing uninitialized heap contents to the hardware.
> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..1ed41d7e923bf 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -294,8 +294,8 @@ static struct fsl_re_desc *fsl_re_chan_alloc_desc(struct fsl_re_chan *re_chan,
> if (!desc)
> return NULL;
>
> - cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT,
> - &paddr);
> + cf = dma_pool_zalloc(re_chan->re_dev->cf_desc_pool, GFP_NOWAIT,
> + &paddr);
[Severity: High]
This is a pre-existing issue, but when reusing a descriptor from free_q
earlier in fsl_re_chan_alloc_desc(), does it leak residual state?
If a client allocates a descriptor, sets a callback, and executes it, the
descriptor returns to free_q. When a subsequent client allocates from free_q:
drivers/dma/fsl_raid.c:fsl_re_chan_alloc_desc() {
...
if (!list_empty(&re_chan->free_q)) {
/* take one desc from free_q */
desc = list_first_entry(&re_chan->free_q,
struct fsl_re_desc, node);
list_del(&desc->node);
desc->async_tx.flags = flags;
}
...
}
Only async_tx.flags is updated here, leaving async_tx.callback and
callback_param populated. Could this result in spurious callbacks being
invoked with freed structures if the new transaction does not explicitly
clear them?
> if (!cf) {
> kfree(desc);
> return NULL;
[ ... ]
> @@ -583,8 +583,8 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
> if (!desc)
> break;
>
> - cf = dma_pool_alloc(re_chan->re_dev->cf_desc_pool, GFP_KERNEL,
> - &paddr);
> + cf = dma_pool_zalloc(re_chan->re_dev->cf_desc_pool, GFP_KERNEL,
> + &paddr);
[Severity: High]
This isn't a bug introduced by this patch, but does the way alloc_count is
tracked lead to integer underflow and list corruption in
fsl_re_free_chan_resources()?
In fsl_re_free_chan_resources(), there is a blind loop:
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);
list_del(&desc->node);
...
}
If the channel is freed while any descriptors are still active (not in
free_q), the loop will run more times than items exist in free_q. Would
this result in list_first_entry() returning an invalid pointer and causing
list corruption?
Additionally, if alloc_count reaches zero and the post-decrement underflows
it to UINT_MAX, wouldn't subsequent calls to fsl_re_alloc_chan_resources()
permanently break descriptor tracking when incrementing alloc_count?
[Severity: High]
This is a pre-existing issue, but could invoking client callbacks from
fsl_re_dequeue() while holding desc_lock cause an AA deadlock?
drivers/dma/fsl_raid.c:fsl_re_dequeue() {
...
spin_lock_irqsave(&re_chan->desc_lock, flags);
count = FSL_RE_SLOT_FULL(in_be32(&re_chan->jrregs->oubring_slot_full));
while (count--) {
/* ... */
if (found) {
fsl_re_desc_done(desc);
...
}
The fsl_re_desc_done() function executes the client's completion callback.
If the client submits a new transaction from within this callback,
fsl_re_tx_submit() will attempt to acquire re_chan->desc_lock again. Does
this result in a recursive spinlock deadlock?
[Severity: High]
This isn't a bug introduced by this patch, but does the driver fail to free
its requested IRQ, potentially leading to a use-after-free?
In fsl_re_chan_probe(), an IRQ is requested without devm_:
drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
...
ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
if (ret) {
...
}
However, free_irq() does not appear to be called in the probe error paths
or during driver removal in fsl_re_remove_chan(). If the device is removed,
re_chan is freed via devm_kzalloc. If a subsequent hardware interrupt
occurs, won't fsl_re_isr() attempt to dereference the freed re_chan
pointer?
Also, in fsl_re_remove(), memory areas and tasklets are destroyed before
the device is unregistered:
drivers/dma/fsl_raid.c:fsl_re_remove() {
...
/* Cleanup chan related memory areas */
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);
...
}
Does this allow clients to still access DMA resources while they are being
dismantled?
> if (!cf) {
> kfree(desc);
> break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260820031654.28450-1-rosenp@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread