Re: [PATCH v3 31/34] dmaengine: dw: defer callbacks via channel BH

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but dropping the channel lock during list iteration in `dwc_scan_descriptors` causes a Use-After-Free if `dwc_terminate_all` runs concurrently.
--

--- Patch [31]: [PATCH v3 31/34] dmaengine: dw: defer callbacks via channel BH ---
commit 15e9633a2c2daf5f60e16d885a4f6398581bad8b
Author: Allen Pais <[email protected]>

dmaengine: dw: defer callbacks via channel BH

This commit defers DMA descriptor callbacks to a per-channel bottom half
workqueue instead of running them from the controller tasklet. It introduces
a completed_list to hold descriptors pending callbacks and adds a synchronize
hook to ensure termination waits for the BH to complete.

> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index dd75f97a33b3d..2765acfe56443 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
[ ... ]
> @@ -965,6 +982,11 @@ static int dwc_terminate_all(struct dma_chan *chan)
>  	return 0;
>  }
>  

[Severity: High]
This isn't a bug introduced by this patch, but dropping the channel lock
during list iteration in dwc_scan_descriptors() causes a Use-After-Free if
dwc_terminate_all() runs concurrently.

dwc_scan_descriptors() iterates over dwc->active_list using
list_for_each_entry_safe(). The loop iterator caches _desc (the next element).
Before processing the next element, the lock is dropped to call
dwc_descriptor_complete():

	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
		...
		spin_unlock_irqrestore(&dwc->lock, flags);
		dwc_descriptor_complete(dwc, desc, true);
		spin_lock_irqsave(&dwc->lock, flags);
	}

If dwc_terminate_all() runs concurrently on another CPU, it acquires the lock,
splices active_list into a local list, and frees all of its descriptors:

	spin_lock_irqsave(&dwc->lock, flags);
	...
	list_splice_init(&dwc->active_list, &list);
	spin_unlock_irqrestore(&dwc->lock, flags);

	list_for_each_entry_safe(desc, _desc, &list, desc_node)
		dwc_descriptor_complete(dwc, desc, false);

When dwc_scan_descriptors() regains the lock, it advances the iterator to
_desc, which has now been freed.

Can we address this to prevent memory corruption when channel operation and
termination occur concurrently?

> +static void dwc_synchronize(struct dma_chan *chan)
> +{
> +	dmaengine_kill_bh(chan);
> +}
> +
[ ... ]

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