Re: [PATCH v2 05/64] dmaengine: ep93xx: hook callbacks via dma_chan BH

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

Pre-existing issues:
- [High] Missing `dma_chan_kill_bh()` in `ep93xx_dma_synchronize()` and `ep93xx_dma_free_chan_resources()` violates the dmaengine API and leads to premature callback execution for new clients.
- [High] The ep93xx_dma driver is vulnerable to a Use-After-Free and kernel panic if unbound via sysfs, due to the lack of a .remove callback and missing .suppress_bind_attrs = true.
--

--- Patch [5]: [PATCH v2 05/64] dmaengine: ep93xx: hook callbacks via dma_chan BH ---
commit e03616bfd332cb745207d3436713d688064c9cb5
Author: Allen Pais <[email protected]>

dmaengine: ep93xx: hook callbacks via dma_chan BH

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

> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index 311e55a97ba9f..8f5200750fe98 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c

[ ... ]

> @@ -1420,7 +1418,7 @@ static struct ep93xx_dma_engine *ep93xx_dma_of_probe(struct platform_device *pde
>  		INIT_LIST_HEAD(&edmac->active);
>  		INIT_LIST_HEAD(&edmac->queue);
>  		INIT_LIST_HEAD(&edmac->free_list);
> -		tasklet_setup(&edmac->tasklet, ep93xx_dma_tasklet);
> +		dma_chan_init_bh(&edmac->chan, ep93xx_dma_tasklet);
>  
>  		list_add_tail(&edmac->chan.device_node,
>  			      &dma_dev->channels);

[Severity: High]
This is a pre-existing issue, but I noticed ep93xx_dma_synchronize() and
ep93xx_dma_free_chan_resources() do not cancel the pending BH work.

If a client calls dma_terminate_all(), the active list is emptied, but a
scheduled BH is not cancelled because the driver does not call
dma_chan_kill_bh() (previously missing tasklet_kill()).

When the channel is reallocated, the pending BH can run, see the new client's
descriptor, and unconditionally invoke its callback before completion:

drivers/dma/ep93xx_dma.c:ep93xx_dma_tasklet()
    ...
	desc = ep93xx_dma_get_active(edmac);
	if (desc) {
		if (desc->complete) {
			/* mark descriptor complete for non cyclic case only */
			if (!test_bit(EP93XX_DMA_IS_CYCLIC, &edmac->flags))
				dma_cookie_complete(&desc->txd);
			list_splice_init(&edmac->active, &list);
		}
		dmaengine_desc_get_callback(&desc->txd, &cb);
	}
    ...

Does this need to call dma_chan_kill_bh() during synchronization or channel
release to prevent state corruption for new DMA clients?

[Severity: High]
This isn't a bug introduced by this patch, but the ep93xx_dma driver appears
vulnerable to a Use-After-Free and kernel panic if unbound via sysfs.

Because the driver lacks a .remove callback and doesn't set
.suppress_bind_attrs = true in ep93xx_dma_driver:

drivers/dma/ep93xx_dma.c:
static struct platform_driver ep93xx_dma_driver = {
	.driver		= {
		.name	= "ep93xx-dma",
		.of_match_table = ep93xx_dma_of_ids,
	},
	.probe		= ep93xx_dma_probe,
};

If a privileged user unbinds the driver via sysfs, it never calls
dma_async_device_unregister() or frees IRQs. The device core's devres
mechanism will free the edma and edmac structures, but any subsequent IRQ
firing or scheduled BH execution will dereference the freed memory.

Should this driver include a .remove callback or set
.suppress_bind_attrs = true to prevent this?

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