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

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

Pre-existing issues:
- [High] For cyclic DMA transfers, the updater descriptor's DMA address is leaked, and the first descriptor's DMA address is double-freed in the DMA pool.
--

--- Patch [1]: [PATCH v2] dmaengine: pxa: fix double counting of the hw descriptors ---
commit 5e37421334e2f54f86d40ee190a04a05eb99596b
Author: Sascha Hauer <[email protected]>

dmaengine: pxa: fix double counting of the hw descriptors

This commit fixes a double counting issue with sw_desc->nb_desc in
pxad_alloc_desc() caused by the transition to kzalloc_flex(). It explicitly
sets nb_desc and removes the redundant increment in the allocation loop.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..fc43124fefa89 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c

[ ... ]

> @@ -744,6 +744,7 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  	sw_desc = kzalloc_flex(*sw_desc, hw_desc, nb_hw_desc, GFP_NOWAIT);
>  	if (!sw_desc)
>  		return NULL;
> +	sw_desc->nb_desc = nb_hw_desc;

[Severity: High]
This isn't a bug introduced by this patch, but since this patch corrects the
nb_desc tracking used by set_updater_desc() and pxad_free_desc(), is there a
pre-existing double-free for cyclic transfers?

When a cyclic transfer is set up, set_updater_desc() creates a hardware loop
by overwriting the ddadr 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 this overwritten field to retrieve the DMA
address to free the next descriptor in the chain:

drivers/dma/pxa_dma.c:pxad_free_desc() {
    ...
    for (i = sw_desc->nb_desc - 1; i >= 0; i--) {
        if (i > 0)
            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);
    }
    ...
}

Because ddadr was overwritten with sw_desc->first, doesn't pxad_free_desc()
read sw_desc->first instead of the original DMA address when freeing the
updater descriptor?

Could this lead to double-freeing the first descriptor's DMA address and
permanently leaking the updater descriptor's original address in the DMA pool?

>  	sw_desc->desc_pool = chan->desc_pool;
>  
>  	for (i = 0; i < nb_hw_desc; i++) {

[ ... ]

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