Re: [PATCH v2 37/64] dmaengine: idma64: kill vchan BH on teardown

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

Pre-existing issues:
- [High] The channel cleanup loop in idma64_remove() is dead code, leading to a Use-After-Free of bottom-half work items.
- [High] A race condition during device removal can cause an unhandled interrupt storm.
- [High] Use-After-Free due to missing device_release callback with devres-managed core structure.
--

--- Patch [37]: [PATCH v2 37/64] dmaengine: idma64: kill vchan BH on teardown ---
commit e368659298f3842bf78bcfc4c53afeb968ac845c
Author: Allen Pais <[email protected]>

dmaengine: idma64: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/idma64.c b/drivers/dma/idma64.c
> index 5fcd1befc92da..8e25d828e553d 100644
> --- a/drivers/dma/idma64.c
> +++ b/drivers/dma/idma64.c
> @@ -617,14 +617,14 @@ static void idma64_remove(struct idma64_chip *chip)
>  
>  	/*
>  	 * Explicitly call devm_request_irq() to avoid the side effects with
> -	 * the scheduled tasklets.
> +	 * scheduled BH work.
>  	 */
>  	devm_free_irq(chip->dev, chip->irq, idma64);

[Severity: High]
This is a pre-existing issue, but does calling devm_free_irq() after
dma_async_device_unregister() create a race condition that can cause an
interrupt storm?

The earlier call to dma_async_device_unregister() iterates over the channels
and decrements idma64->dma.chancnt to 0.

If a hardware interrupt triggers in the narrow window before devm_free_irq()
disables it, idma64_irq() executes this loop:

idma64_irq() {
    ...
    for (i = 0; i < idma64->dma.chancnt; i++)
        idma64_chan_irq(idma64, i, status_err, status_xfer);
    ...
}

Because chancnt is 0, it skips clearing the active hardware interrupt but
still returns IRQ_HANDLED. Will the hardware continually re-assert the
uncleared interrupt and hang the CPU?

>  
>  	for (i = 0; i < idma64->dma.chancnt; i++) {
>  		struct idma64_chan *idma64c = &idma64->chan[i];
>  
> -		tasklet_kill(&idma64c->vchan.task);
> +		dma_chan_kill_bh(&idma64c->vchan.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but will this cleanup loop ever execute?

Since the preceding call to dma_async_device_unregister() decrements
idma64->dma.chancnt to 0 before we reach this loop, the loop is completely
skipped and dma_chan_kill_bh() is never called.

When devres subsequently frees the idma64 structure, won't any pending
bottom-half work items execute on freed memory?

[Severity: High]
This is also a pre-existing issue, but is there a use-after-free risk because
struct idma64 is allocated with devm_kzalloc() but lacks a
dma_device->device_release callback?

When the driver unbinds and idma64_remove() completes, devres will
automatically free the idma64 structure:

idma64_probe() {
    ...
    idma64 = devm_kzalloc(chip->dev, sizeof(*idma64), GFP_KERNEL);
    ...
}

If a DMA client still holds a reference to a channel, won't the client
dereference the freed idma64 memory when accessing or releasing the channel
later, since the DMA core manages reference counts via device->ref?

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