Re: [PATCH 1/6] dmaengine: zynqmp_dma: Fix alloc_chan_resources error cleanup

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

Pre-existing issues:
- [High] Stale list iteration pointers after dropping and reacquiring the lock in zynqmp_dma_chan_desc_cleanup.
--

--- Patch [1]: [PATCH 1/6] dmaengine: zynqmp_dma: Fix alloc_chan_resources error cleanup ---
commit 8086a707a0f2604d248437e0453b7591161223d9
Author: Golla Nagendra <[email protected]>

dmaengine: zynqmp_dma: Fix alloc_chan_resources error cleanup

Channel resource allocation can fail after runtime PM has been acquired
and after part of the descriptor state has been initialized. Without
proper rollback, the error path leaks a runtime PM reference and, on
coherent allocation failure, also leaks the software descriptor pool.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..b7c5612806948 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c

[ ... ]

> @@ -516,6 +520,13 @@ static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)
>  	}
>  
>  	return ZYNQMP_DMA_NUM_DESCS;
> +
> +err_free_sw_desc_pool:
> +	kfree(chan->sw_desc_pool);
> +	chan->sw_desc_pool = NULL;
> +err_pm:
> +	pm_runtime_put_autosuspend(chan->dev);
> +	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at
zynqmp_dma_chan_desc_cleanup() in drivers/dma/xilinx/zynqmp_dma.c,
can dropping the lock during iteration lead to list corruption?

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_desc_cleanup() {
    ...
    list_for_each_entry_safe(desc, next, &chan->done_list, node) {
        ...
        if (dmaengine_desc_callback_valid(&cb)) {
            spin_unlock_irqrestore(&chan->lock, irqflags);
            dmaengine_desc_callback_invoke(&cb, NULL);
            spin_lock_irqsave(&chan->lock, irqflags);
        }
    ...
}

If a DMA client invokes dmaengine_terminate_all() from the callback,
zynqmp_dma_device_terminate_all() will acquire the lock and move all pending
and done descriptors (including the cached next pointer) to chan->free_list.

When the callback returns and the lock is reacquired, would the loop iterator
operate on nodes linked in the free_list instead of done_list?

Since the loop condition checks against &chan->done_list, could this iterate
past the end of the list and trigger an out-of-bounds dereference?

Additionally, would calling zynqmp_dma_free_descriptor() again on a descriptor
already moved to free_list cause desc_free_cnt to exceed its maximum limit
and corrupt the allocator state?

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