Re: [PATCH v3 03/34] dmaengine: apple-admac: use dmaengine BH callback

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

Pre-existing issues:
- [High] Missing cancellation of the bottom half (BH) work in the driver's remove path (`admac_remove`) can lead to Use-After-Free if the driver is unbound via sysfs while channels are in use.
--

--- Patch [3]: [PATCH v3 03/34] dmaengine: apple-admac: use dmaengine BH callback ---
commit 6eb12029fd22d727d6a26b7c59c7dfc98c07ffa3
Author: Allen Pais <[email protected]>

dmaengine: apple-admac: use dmaengine BH callback

This patch replaces the per-channel tasklet with the shared dmaengine BH
helper. The handler continues to run in softirq context while dmaengine
owns the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
> index 14a5ee14a4815..c5d4fb96d3c4e 100644
> --- a/drivers/dma/apple-admac.c
> +++ b/drivers/dma/apple-admac.c

[ ... ]

> @@ -886,7 +882,7 @@ static int admac_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&adchan->issued);
>  		INIT_LIST_HEAD(&adchan->to_free);
>  		list_add_tail(&adchan->chan.device_node, &dma->channels);
> -		tasklet_setup(&adchan->tasklet, admac_chan_tasklet);
> +		dmaengine_init_bh(&adchan->chan, admac_chan_bh);
>  	}
>  
>  	err = reset_control_reset(ad->rstc);

[Severity: High]
This is a pre-existing issue, but should the teardown path in admac_remove()
explicitly terminate active hardware channels and flush the asynchronous work
items?

If the driver is unbound via sysfs while channels are in use, admac_remove()
returns without stopping the hardware or killing the BH work:

drivers/dma/apple-admac.c:admac_remove() {
	struct admac_data *ad = platform_get_drvdata(pdev);

	of_dma_controller_free(pdev->dev.of_node);
	dma_async_device_unregister(&ad->dma);
	free_irq(ad->irq, ad);
	reset_control_rearm(ad->rstc);
}

The devres framework then automatically frees the admac_data memory which
contains the bh_work struct.

If an interrupt fires immediately before free_irq(), it will schedule the BH
work item via dmaengine_schedule_bh(). The workqueue would then execute
admac_chan_bh() after the memory has been freed, potentially leading to a
use-after-free.

Could a loop be added to call dmaengine_kill_bh() for each channel in
admac_remove() before letting devres free the memory?

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