Re: [PATCH v2 48/64] dmaengine: owl-dma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] The interrupt handler incorrectly processes cyclic DMA transactions as if they were one-shot transfers, breaking cyclic DMA (e.g., audio playback) by stopping the channel after the first period.
- [High] Virtual channels stall indefinitely if their transactions are queued when all physical DMA channels are busy.
- [High] Missing `device_synchronize` callback exposes client drivers to Use-After-Free vulnerabilities during channel teardown.
- [High] The driver fails to halt active hardware DMA channels during `remove()`, causing rogue DMA transfers and potential bus lockups.
--

--- Patch [48]: [PATCH v2 48/64] dmaengine: owl-dma: kill vchan BH on teardown ---
commit 2420d60507a6a038239d14db32bd418a2fe41702
Author: Allen Pais <[email protected]>

dmaengine: owl-dma: kill vchan BH on teardown

This commit updates the driver to use dma_chan_kill_bh() for virt-dma cleanup
during teardown, replacing the older tasklet_kill() interface.

> diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
> index 7c80572fc71d1..5f2140dc01cec 100644
> --- a/drivers/dma/owl-dma.c
> +++ b/drivers/dma/owl-dma.c
> @@ -1055,7 +1055,7 @@ static inline void owl_dma_free(struct owl_dma *od)
>  	list_for_each_entry_safe(vchan,
>  				 next, &od->dma.channels, vc.chan.device_node) {
>  		list_del(&vchan->vc.chan.device_node);
> -		tasklet_kill(&vchan->vc.task);
> +		dma_chan_kill_bh(&vchan->vc.chan);
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
incorrectly process cyclic DMA transactions as if they were one-shot transfers?

In drivers/dma/owl-dma.c:owl_dma_interrupt():

    txd = vchan->txd;
    if (txd) {
        vchan->txd = NULL;
        vchan_cookie_complete(&txd->vd);

        if (vchan_next_desc(&vchan->vc))
            owl_dma_start_next_txd(vchan);
        else
            owl_dma_phy_free(od, vchan);
    }

It appears this unconditionally completes and removes the descriptor without
checking txd->cyclic or calling vchan_cyclic_callback(). Can this break
cyclic DMA transfers by stopping the channel after the first period?


[Severity: High]
This is a pre-existing issue, but could virtual channels stall indefinitely if
their transactions are queued when all physical DMA channels are busy?

When allocating a physical channel in owl_dma_phy_alloc_and_start():

    pchan = owl_dma_get_pchan(od, vchan);
    if (!pchan)
        return;

If no physical channel is available, the request silently remains in the
virtual queue. However, when a physical channel is later freed in
owl_dma_phy_free():

    owl_dma_terminate_pchan(od, vchan->pchan);
    vchan->pchan = NULL;

The physical channel is released, but it doesn't appear to be reassigned to
other waiting virtual channels. Does this cause queued transfers on those
unassigned virtual channels to stall indefinitely?


[Severity: High]
While this isn't a bug introduced by your change, is the device_synchronize
callback missing from the DMA engine operations?

In drivers/dma/owl-dma.c:owl_dma_probe():

    od->dma.device_pause = owl_dma_pause;
    od->dma.device_resume = owl_dma_resume;
    od->dma.device_terminate_all = owl_dma_terminate_all;
    od->dma.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);

Without mapping device_synchronize to vchan_synchronize,
dmaengine_terminate_sync() acts as a no-op for synchronization. Could this
expose client drivers to use-after-free vulnerabilities if the bottom-half
continues to run concurrently after the client assumes the channel is safely
halted?


[Severity: High]
This is a pre-existing issue, but does the driver fail to cleanly halt active
hardware DMA channels during removal?

In drivers/dma/owl-dma.c:owl_dma_remove():

    of_dma_controller_free(pdev->dev.of_node);
    dma_async_device_unregister(&od->dma);

    /* Mask all interrupts for this execution environment */
    dma_writel(od, OWL_DMA_IRQ_EN0, 0x0);

    /* Make sure we won't have any further interrupts */
    devm_free_irq(od->dma.dev, od->irq, od);

    owl_dma_free(od);

    clk_disable_unprepare(od->clk);

The removal function destroys driver resources and disables clocks, but
it doesn't seem to loop over od->pchans to smoothly halt running physical
channels via owl_dma_terminate_pchan(). Can disabling the clock while the
DMA controller is actively mastering the bus lock up the system, or cause
rogue DMA writes to freed memory?

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