Re: [PATCHv3 5/8] dmaengine: ti: omap-dma: disable IRQs on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `return rc;` in the `of_dma_controller_register` error path causes probe to fall through and register a CPU PM notifier using memory that will subsequently be freed.
- [High] Missing `cpu_pm_unregister_notifier` on module removal for configurations with `needs_busy_check`, leading to a dangling PM notifier.
- [High] Wrong interrupt enable register (`IRQENABLE_L0`) is cleared during `omap_dma_remove`, risking an unhandled IRQ storm.
- [High] Division by zero in SG transfer preparation if `maxburst` is configured to 0.
--

commit 2fb0d3495f0f7caa9bb739f8dcae6f87cb0dcd07
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: ti: omap-dma: disable IRQs on probe failure

The probe failure paths after IRQ setup free channel state while hardware
interrupts can still be enabled. A concurrent interrupt can then walk
lch_map[] and access channel memory that teardown is releasing. Disable
IRQENABLE_L1 and clear irq_enable_mask under irq_lock before teardown,
then read IRQENABLE_L1 back to flush the posted write.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..a5dc07bcbeb87 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1825,6 +1832,13 @@ static int omap_dma_probe(struct platform_device *pdev)
>  		if (rc) {
>  			pr_warn("OMAP-DMA: failed to register DMA controller\n");
>  			dma_async_device_unregister(&od->ddev);
> +			if (!omap_dma_legacy(od)) {
> +				spin_lock_irq(&od->irq_lock);
> +				od->irq_enable_mask = 0;
> +				omap_dma_glbl_write(od, IRQENABLE_L1, 0);
> +				spin_unlock_irq(&od->irq_lock);
> +				omap_dma_glbl_read(od, IRQENABLE_L1);
> +			}
>  			if (od->ll123_supported)
>  				dma_pool_destroy(od->desc_pool);
>  			omap_dma_free(od);
>  		}

[Severity: High]
This is a pre-existing issue, but shouldn't this error path in
omap_dma_probe() return the error code?

If of_dma_controller_register() fails, the code frees the resources and
cleans up, but execution falls through instead of returning. It then proceeds
to register a CPU PM notifier on the newly freed memory:

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
    ...
	if (od->cfg->needs_busy_check) {
		od->nb.notifier_call = omap_dma_busy_notifier;
		cpu_pm_register_notifier(&od->nb);
    ...
}

When devres releases the od structure, od->nb will become a dangling
pointer, which could cause a use-after-free on the next CPU power transition.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does omap_dma_remove()
correctly unregister the CPU PM notifier in all configurations?

In omap_dma_probe(), the notifier is conditionally registered like this:

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
    ...
	if (od->cfg->needs_busy_check) {
		od->nb.notifier_call = omap_dma_busy_notifier;
		cpu_pm_register_notifier(&od->nb);
	} else if (od->cfg->may_lose_context) {
		od->nb.notifier_call = omap_dma_context_notifier;
		cpu_pm_register_notifier(&od->nb);
	}
    ...
}

But in omap_dma_remove(), it only unregisters if may_lose_context is true:

drivers/dma/ti/omap-dma.c:omap_dma_remove() {
    ...
	if (od->cfg->may_lose_context)
		cpu_pm_unregister_notifier(&od->nb);
    ...
}

If needs_busy_check is true and may_lose_context is false, the notifier is
never unregistered, leaving a dangling pointer when the module is removed.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does omap_dma_remove() disable the correct
interrupt enable register?

During omap_dma_remove(), the code attempts to disable interrupts:

drivers/dma/ti/omap-dma.c:omap_dma_remove() {
    ...
	if (!omap_dma_legacy(od)) {
		/* Disable all interrupts */
		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
	}
    ...
}

The driver configures interrupts on IRQENABLE_L1 during probe and channel
allocation. By clearing IRQENABLE_L0 instead, active channels remain enabled in
IRQENABLE_L1. After the IRQ handler is unregistered, any pending hardware
interrupt could cause an unhandled IRQ storm.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could a client passing 0 for
maxburst cause a division by zero in omap_dma_prep_slave_sg()?

In omap_dma_prep_slave_sg(), the burst variable is initialized directly from
the client's configuration:

drivers/dma/ti/omap-dma.c:omap_dma_prep_slave_sg() {
    ...
	if (dir == DMA_DEV_TO_MEM) {
        ...
		burst = c->cfg.src_maxburst;
    ...
	en = burst;
	frame_bytes = es_bytes[es] * en;
    ...
}

Later in the scatterlist loop, frame_bytes is used in a division:

drivers/dma/ti/omap-dma.c:omap_dma_prep_slave_sg() {
    ...
	for_each_sg(sgl, sgent, sglen, i) {
		struct omap_sg *osg = &d->sg[i];
        ...
		osg->fn = sg_dma_len(sgent) / frame_bytes;
    ...
}

If a client passes 0 for maxburst, frame_bytes will be 0, leading to an
unhandled division by zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603030754.288757-6-rosenp@gmail.com?part=1