Re: [PATCH v3 26/34] dmaengine: mpc512x: route callbacks via channel BH

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

New issues:
- [High] Missing dmaengine_kill_bh() in mpc_dma_remove() leads to Use-After-Free of per-channel BH work upon driver unbind.

Pre-existing issues:
- [High] Concurrent execution of completion processing can lead to backward progression of completed_cookie and out-of-order callbacks.
- [High] Inline descriptor processing in `prep_` functions executes callbacks synchronously across all channels, risking deadlocks.
- [High] Missing `device_synchronize` implementation in dma_device to flush the deferred completion work.
- [High] Interrupt handler is requested before tasklet and locks are initialized, and tasklet cleanup is missing in probe error paths.
- [High] mpc_dma_device_terminate_all does not clear mchan->completed, leading to BUG_ON crash in free_chan_resources.
--

--- Patch [26]: [PATCH v3 26/34] dmaengine: mpc512x: route callbacks via channel BH ---
commit a8db08e8595cca1d5c7b07d2e006d85529289c2e
Author: Allen Pais <[email protected]>
Subject: dmaengine: mpc512x: route callbacks via channel BH

This patch modifies the MPC512x DMA driver to handle descriptor completions
through per-channel bottom halves instead of a global tasklet. It introduces
workqueue scheduling and cleanup for the channels to process callbacks, while
leaving the tasklet to manage IRQ and error handling.

> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 0adc8e01057e1..19e0b2f1b2aa3 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
[ ... ]
> +static void mpc_dma_process_chan_completed(struct mpc_dma_chan *mchan)
>  {
>  	dma_cookie_t last_cookie = 0;
> -	struct mpc_dma_chan *mchan;
>  	struct mpc_dma_desc *mdesc;
>  	struct dma_async_tx_descriptor *desc;
>  	unsigned long flags;
>  	LIST_HEAD(list);
> -	int i;
>  
> -	for (i = 0; i < mdma->dma.chancnt; i++) {
> -		mchan = &mdma->channels[i];
> +	/* Get all completed descriptors */
> +	spin_lock_irqsave(&mchan->lock, flags);
> +	if (!list_empty(&mchan->completed))
> +		list_splice_tail_init(&mchan->completed, &list);
> +	spin_unlock_irqrestore(&mchan->lock, flags);
>  
> -		/* Get all completed descriptors */
> -		spin_lock_irqsave(&mchan->lock, flags);
> -		if (!list_empty(&mchan->completed))
> -			list_splice_tail_init(&mchan->completed, &list);
> -		spin_unlock_irqrestore(&mchan->lock, flags);
> +	if (list_empty(&list))
> +		return;
>  
> -		if (list_empty(&list))
> -			continue;
> +	/* Execute callbacks and run dependencies */
> +	list_for_each_entry(mdesc, &list, node) {
> +		desc = &mdesc->desc;
>  
> -		/* Execute callbacks and run dependencies */
> -		list_for_each_entry(mdesc, &list, node) {
> -			desc = &mdesc->desc;
> +		dmaengine_desc_get_callback_invoke(desc, NULL);
>  
> -			dmaengine_desc_get_callback_invoke(desc, NULL);
> +		last_cookie = desc->cookie;
> +		dma_run_dependencies(desc);
> -		}
> +	}
>  
> -		/* Free descriptors */
> -		spin_lock_irqsave(&mchan->lock, flags);
> -		list_splice_tail_init(&list, &mchan->free);
> -		mchan->chan.completed_cookie = last_cookie;
> -		spin_unlock_irqrestore(&mchan->lock, flags);
> -	}
> +	/* Free descriptors */
> +	spin_lock_irqsave(&mchan->lock, flags);
> +	list_splice_tail_init(&list, &mchan->free);
> +	mchan->chan.completed_cookie = last_cookie;
> +	spin_unlock_irqrestore(&mchan->lock, flags);
> +}

[Severity: High]
This is a pre-existing issue, but can concurrent execution of completion
processing lead to out-of-order callbacks or backward progression of
completed_cookie?

If mpc_dma_prep_memcpy() calls mpc_dma_process_completed() synchronously
while the BH work executes concurrently, both paths take mchan->lock,
splice a batch of completed descriptors to a local list, and drop the lock
to execute callbacks. 

If Thread A splices older descriptors and Thread B subsequently splices newer
ones, Thread B might finish first and update completed_cookie. When Thread A
finishes, could it overwrite completed_cookie with the older value?

>  
> -/* process completed descriptors */
> -static void mpc_dma_process_completed(struct mpc_dma *mdma)
> +/* process completed descriptors */
> +static void mpc_dma_process_completed(struct mpc_dma *mdma)
> +{
> +	int i;
> +
> +	for (i = 0; i < mdma->dma.chancnt; i++)
> +		mpc_dma_process_chan_completed(&mdma->channels[i]);
>  }

[Severity: High]
This is a pre-existing issue, but does iterating over all channels here risk
cross-channel deadlocks?

When mpc_dma_prep_memcpy() or mpc_dma_prep_slave_sg() run out of free
descriptors, they call mpc_dma_process_completed() directly, running
callbacks inline for all channels. 

If a client calls the prep_ function with a lock held, and a callback for
another channel attempts to acquire the same lock, could this cause an ABBA
deadlock?

[ ... ]
> @@ -551,6 +568,8 @@ static void mpc_dma_free_chan_resources(struct dma_chan *chan)
>  	unsigned long flags;
>  	LIST_HEAD(descs);
>  
> +	dmaengine_kill_bh(&mchan->chan);
> +
>  	spin_lock_irqsave(&mchan->lock, flags);
>  
>  	/* Channel must be idle */

[Severity: High]
This is a pre-existing issue, but does mpc_dma_device_terminate_all() leave
completed descriptors on the list, causing a crash here?

mpc_dma_device_terminate_all() disables channel requests and splices
prepared, queued, and active lists back to the free list, but it completely
ignores mchan->completed.

If a hardware completion interrupt fires just before terminate_all is called,
completed descriptors remain in mchan->completed. When the client subsequently
calls dma_release_channel(), will mpc_dma_free_chan_resources() assert
BUG_ON(!list_empty(&mchan->completed)) and crash the kernel?

[ ... ]
> @@ -1007,6 +1026,7 @@ static int mpc_dma_probe(struct platform_device *op)
>  		INIT_LIST_HEAD(&mchan->completed);
>  
>  		spin_lock_init(&mchan->lock);
> +		dmaengine_init_bh(&mchan->chan, mpc_dma_chan_bh);
>  		list_add_tail(&mchan->chan.device_node, &dma->channels);
>  	}
>  

[Severity: High]
Is it possible for per-channel BH work to remain queued after the driver is
unbound?

The new workqueue is initialized here, but looking at mpc_dma_remove(), it
only calls tasklet_kill(&mdma->tasklet) and does not iterate over channels
to call dmaengine_kill_bh().

Could this cause the workqueue to execute mpc_dma_chan_bh() on freed memory 
if the driver is forcefully unbound via sysfs while DMA is active?

[Severity: High]
This is a pre-existing issue, but is the driver missing a device_synchronize
callback?

Without this callback implemented in struct dma_device,
dmaengine_terminate_sync() will return immediately without waiting for
the bottom-half to finish.

If the client frees its DMA buffers immediately after terminate_sync()
returns, will the pending workqueue execute callbacks that access the
freed memory?

[Severity: High]
This is a pre-existing issue, but is it safe to request the interrupt before
the tasklet and locks are initialized?

In mpc_dma_probe(), request_irq() is called before tasklet_setup(),
per-channel list initialization, and spin_lock_init().

If a pending interrupt from the hardware fires immediately, could the ISR
execute on uninitialized spinlocks and lists, or call tasklet_schedule()
on a zero-initialized tasklet? Additionally, if probe fails later, the
error path frees the IRQ but fails to call tasklet_kill(), which could
allow a pending tasklet to run after devres frees the mdma structure.

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