[PATCH] dmaengine: pxa: fix double counting of the hw descriptors

[PATCH] dmaengine: pxa: fix double counting of the hw descriptors

[Sascha Hauer]

pxad_alloc_desc() was converted from

	kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc), GFP_NOWAIT)

to kzalloc_flex(). hw_desc[] is annotated with __counted_by(nb_desc), so
__alloc_flex() now initializes sw_desc->nb_desc to nb_hw_desc itself.
The loop below it still increments nb_desc for every descriptor it
allocates though, so nb_desc ends up being twice the number of
descriptors that are actually there.

Drop the now redundant increment. The error path has to set nb_desc to
the number of descriptors allocated so far, otherwise pxad_free_desc()
would free entries that were never allocated.

Fixes: 69050f8d6d075 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
pxad_alloc_desc() was converted from

        kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc), GFP_NOWAIT)

to kzalloc_flex(). hw_desc[] is annotated with __counted_by(nb_desc), so
__alloc_flex() now initializes sw_desc->nb_desc to nb_hw_desc itself.
The loop below it still increments nb_desc for every descriptor it
allocates though, so nb_desc ends up being twice the number of
descriptors that are actually there.

Drop the now redundant increment. The error path has to set nb_desc to
the number of descriptors allocated so far, otherwise pxad_free_desc()
would free entries that were never allocated.
---
 drivers/dma/pxa_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index fa2ee0b3e09f8..8252d27be8c3c 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -752,10 +752,11 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
 			dev_err(&chan->vc.chan.dev->device,
 				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
 				__func__, i, sw_desc->desc_pool);
+			/* Only the descriptors below i have been allocated */
+			sw_desc->nb_desc = i;
 			goto err;
 		}
 
-		sw_desc->nb_desc++;
 		sw_desc->hw_desc[i] = desc;
 
 		if (i == 0)

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260817-dmaengine-pxa-64152bb34313

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH] dmaengine: pxa: fix double counting of the hw descriptors

[sashiko-bot]

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

New issues:
- [Critical] Removing the manual `sw_desc->nb_desc++` increment breaks the driver on compilers without `__builtin_counted_by_ref` support, permanently leaving `nb_desc` at 0 on success and causing out-of-bounds memory corruption.

Pre-existing issues:
- [High] In cyclic DMA mode, the updater descriptor is permanently leaked and its DMA address is mismatched, causing a `dma_pool_free` error and a potential double-free of the first descriptor's DMA address.
- [High] The driver's teardown sequence leaks DMA channels and creates a use-after-free window in the IRQ handler due to conflicting devm registration and manual cleanup.
--

--- Patch [1]: [PATCH] dmaengine: pxa: fix double counting of the hw descriptors ---
commit 31bd2b3ddeaa75fd6d698e4a2da9964bd5fc7adc
Author: Sascha Hauer <s.hauer@pengutronix.de>

dmaengine: pxa: fix double counting of the hw descriptors

This commit drops a manual increment of nb_desc in pxad_alloc_desc(),
relying on kzalloc_flex() to initialize the counted_by variable. It also
updates the error path to ensure nb_desc accurately reflects allocated
descriptors.

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..8252d27be8c3c 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -752,10 +752,11 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  			dev_err(&chan->vc.chan.dev->device,
>  				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
>  				__func__, i, sw_desc->desc_pool);
> +			/* Only the descriptors below i have been allocated */
> +			sw_desc->nb_desc = i;
>  			goto err;
>  		}
>  
> -		sw_desc->nb_desc++;

[Severity: Critical]
Does removing this manual increment break the driver on compilers without
__builtin_counted_by_ref() support (such as GCC < 15 or Clang < 19)?

It appears that without compiler support, the kzalloc_flex() macro acts as
a no-op for initializing the counter, which would leave sw_desc->nb_desc as 0
on a successful allocation.

If nb_desc remains 0, could this cause out-of-bounds memory corruption later
in set_updater_desc()?

drivers/dma/pxa_dma.c:set_updater_desc() {
	struct pxad_desc_hw *updater =
		sw_desc->hw_desc[sw_desc->nb_desc - 1];
...

If nb_desc is 0, this evaluates to hw_desc[-1], which would overlay and
corrupt the adjacent desc_pool pointer in the pxad_desc_sw struct.


[Severity: High]
This isn't a bug introduced by this patch, but does the current driver
permanently leak the updater descriptor in cyclic DMA mode?

During cyclic setup, set_updater_desc() overwrites the ddadr field of the
second-to-last descriptor:

drivers/dma/pxa_dma.c:set_updater_desc() {
	...
	if (sw_desc->cyclic)
		sw_desc->hw_desc[sw_desc->nb_desc - 2]->ddadr = sw_desc->first;
	...
}

Later, pxad_free_desc() relies on that overwritten ddadr to find the physical
address for freeing the updater:

drivers/dma/pxa_dma.c:pxad_free_desc() {
	...
	dma = sw_desc->hw_desc[i - 1]->ddadr;
	else
		dma = sw_desc->first;
	dma_pool_free(sw_desc->desc_pool,
		      sw_desc->hw_desc[i], dma);
	...
}

Could this pass the wrong physical address (sw_desc->first) to
dma_pool_free(), failing to free the updater descriptor and potentially
causing a double-free of the first descriptor's address?


[Severity: High]
This is a pre-existing issue, but does the driver's teardown sequence leak
DMA channels and create a use-after-free window in the IRQ handler?

In pxad_probe(), devres allocates the IRQ before the pxad_chan structures:

drivers/dma/pxa_dma.c:pxad_init_phys() {
	...
	ret = devm_request_irq(&op->dev, irq,
			       pxad_chan_handler,
			       IRQF_SHARED, "pxa-dma", phy);
	...
}

On teardown, pxad_remove() manually empties the channel list before the
devm-managed DMA engine unregistration occurs:

drivers/dma/pxa_dma.c:pxad_free_channels() {
	...
	list_del(&c->vc.chan.device_node);
	tasklet_kill(&c->vc.task);
	...
}

When devres later executes dma_async_device_unregister(), does the empty
list cause it to skip unregistering the individual channels?

If the channels are skipped, devres frees the pxad_chan memory while the
IRQ remains registered. Could an interrupt firing in this window cause
pxad_chan_handler() to dereference the freed chan pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-dmaengine-pxa-v1-1-850c215c1196@pengutronix.de?part=1

Re: [PATCH] dmaengine: pxa: fix double counting of the hw descriptors

[Frank Li]

On Mon, Aug 17, 2026 at 06:09:23PM +0200, Sascha Hauer wrote:
> pxad_alloc_desc() was converted from
>
> 	kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc), GFP_NOWAIT)
>
> to kzalloc_flex(). hw_desc[] is annotated with __counted_by(nb_desc), so
> __alloc_flex() now initializes sw_desc->nb_desc to nb_hw_desc itself.
> The loop below it still increments nb_desc for every descriptor it
> allocates though, so nb_desc ends up being twice the number of
> descriptors that are actually there.
>
> Drop the now redundant increment. The error path has to set nb_desc to
> the number of descriptors allocated so far, otherwise pxad_free_desc()
> would free entries that were never allocated.
>
> Fixes: 69050f8d6d075 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---

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

> pxad_alloc_desc() was converted from
>
>         kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc), GFP_NOWAIT)
>
> to kzalloc_flex(). hw_desc[] is annotated with __counted_by(nb_desc), so
> __alloc_flex() now initializes sw_desc->nb_desc to nb_hw_desc itself.
> The loop below it still increments nb_desc for every descriptor it
> allocates though, so nb_desc ends up being twice the number of
> descriptors that are actually there.
>
> Drop the now redundant increment. The error path has to set nb_desc to
> the number of descriptors allocated so far, otherwise pxad_free_desc()
> would free entries that were never allocated.
> ---
>  drivers/dma/pxa_dma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..8252d27be8c3c 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -752,10 +752,11 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  			dev_err(&chan->vc.chan.dev->device,
>  				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
>  				__func__, i, sw_desc->desc_pool);
> +			/* Only the descriptors below i have been allocated */
> +			sw_desc->nb_desc = i;
>  			goto err;
>  		}
>
> -		sw_desc->nb_desc++;
>  		sw_desc->hw_desc[i] = desc;
>
>  		if (i == 0)
>
> ---
> base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
> change-id: 20260817-dmaengine-pxa-64152bb34313
>
> Best regards,
> --
> Sascha Hauer <s.hauer@pengutronix.de>
>