Re: [PATCH 2/6] dmaengine: zynqmp_dma: Fix chan probe error handling

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

Pre-existing issues:
- [High] Dropping a spinlock inside `list_for_each_entry_safe` in `zynqmp_dma_chan_desc_cleanup` allows concurrent modification by `zynqmp_dma_device_terminate_all`, leading to list corruption and out-of-bounds memory accesses.
--

--- Patch [2]: [PATCH 2/6] dmaengine: zynqmp_dma: Fix chan probe error handling ---
commit d9496c3d465362f9e2e30dc80285227c14d66d2b
Author: Golla Nagendra <[email protected]>

dmaengine: zynqmp_dma: Fix chan probe error handling

This commit improves the channel probe error handling in the zynqmp_dma driver.
It fixes the IRQ registration by ensuring the correct error value is returned
and that IRQ freeing in the remove path only occurs for valid numbers.
Additionally, it delays channel list registration until after successful IRQ
setup and cleans up the tasklet on probe failure.

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

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing issue
while reviewing the file.

In zynqmp_dma_chan_desc_cleanup():

	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
		struct dmaengine_desc_callback cb;

		dmaengine_desc_get_callback(&desc->async_tx, &cb);
		if (dmaengine_desc_callback_valid(&cb)) {
			spin_unlock_irqrestore(&chan->lock, irqflags);
			dmaengine_desc_callback_invoke(&cb, NULL);
			spin_lock_irqsave(&chan->lock, irqflags);
		}

		/* Run any dependencies, then free the descriptor */
		zynqmp_dma_free_descriptor(chan, desc);
	}

Does dropping the spinlock inside list_for_each_entry_safe() expose the traversal
state to concurrent modifications?

If a client driver calls dmaengine_terminate_async() concurrently,
zynqmp_dma_device_terminate_all() will acquire chan->lock and move all
descriptors from chan->done_list to chan->free_list (including the cached
'next' pointer).

When the lock is reacquired, the loop will blindly continue traversal on
chan->free_list instead of chan->done_list.

Since the loop termination checks against &chan->done_list, could it iterate
past the list head and treat it as a descriptor, leading to out-of-bounds
memory accesses?

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