Re: [PATCH v2 46/64] dmaengine: imx-sdma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Kernel OOPS on module removal due to uninitialized list head in channel 0.
- [High] Acquiring an uninitialized spinlock for channel 0 during teardown causes a kernel BUG.
- [High] Accessing hardware registers while clocks are disabled causes a synchronous external abort.
- [High] Unbalanced clock disables in `sdma_remove` trigger refcount underflow warnings.
- [High] Use-After-Free race condition in `sdma_update_channel_loop` during concurrent termination.
--

--- Patch [46]: [PATCH v2 46/64] dmaengine: imx-sdma: kill vchan BH on teardown ---
commit 3996df4e4fdc79870fd89eac795a9295e1141104
Author: Allen Pais <[email protected]>

dmaengine: imx-sdma: kill vchan BH on teardown

This commit modifies the virt-dma cleanup path to use dma_chan_kill_bh()
during driver teardown.

> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2399,11 +2399,11 @@ static void sdma_remove(struct platform_device *pdev)
>  	int i;
>  
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> -	/* Kill the tasklet */
> +	/* Kill the channel BH */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>  
> -		tasklet_kill(&sdmac->vc.task);
> +		dma_chan_kill_bh(&sdmac->vc.chan);
>  		sdma_free_chan_resources(&sdmac->vc.chan);

[Severity: High]
This is a pre-existing issue, but does iterating over all channels here lead
to a null pointer dereference for channel 0?

Channel 0 intentionally skips vchan_init() during probe, leaving its list
heads 0-initialized. When sdma_free_chan_resources() calls
vchan_synchronize() for channel 0, it executes:

drivers/dma/virt-dma.h:vchan_synchronize() {
    ...
    list_splice_tail_init(&vc->desc_terminated, &head);

Could this evaluate an empty list on the null next pointer and cause a panic?


[Severity: High]
This isn't a bug introduced by this patch, but does calling
sdma_free_chan_resources() on channel 0 also attempt to acquire an
uninitialized spinlock?

During teardown, sdma_terminate_all() executes:

drivers/dma/imx-sdma.c:sdma_terminate_all() {
    ...
    spin_lock_irqsave(&sdmac->vc.lock, flags);

Since vchan_init() is skipped for channel 0, the lock remains uninitialized
from devm_kzalloc. Can this result in a bad magic system crash?


[Severity: High]
This is a pre-existing issue, but does writing to the hardware registers
during this loop cause a synchronous external abort?

When iterating over all channels, unallocated channels (or when all channels
are inactive) will have the ipg and ahb clocks disabled.

sdma_free_chan_resources() calls sdma_terminate_all(), which invokes
sdma_disable_channel() doing:

drivers/dma/imx-sdma.c:sdma_disable_channel() {
    ...
    writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);

Writing to an i.MX SDMA register while the IP block clocks are gated
typically causes a hardware bus fault. Is there a way to verify the clocks
are enabled before this write?


[Severity: High]
This isn't a bug introduced by this patch, but does this cleanup loop
cause clock refcount underflow warnings?

sdma_free_chan_resources() unconditionally calls:

drivers/dma/imx-sdma.c:sdma_free_chan_resources() {
    ...
    clk_disable(sdma->clk_ipg);
    clk_disable(sdma->clk_ahb);

Since clk_enable() is only invoked dynamically when a client requests a
channel, calling disable on unallocated channels seems unbalanced. Could this
break clock tracking?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free race condition
in sdma_update_channel_loop() during concurrent termination?

sdma_update_channel_loop() drops the lock to invoke the callback:

drivers/dma/imx-sdma.c:sdma_update_channel_loop() {
    ...
    spin_unlock(&sdmac->vc.lock);
    dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
    spin_lock(&sdmac->vc.lock);

    /* Assign buffer ownership to SDMA */
    bd->mode.status |= BD_DONE;

If a concurrent thread calls dmaengine_terminate_sync(), it can take the
lock, move the active descriptor to the terminated list, and schedule the
terminate_worker which frees the descriptor memory.

When the IRQ thread reacquires the lock, it writes to bd->mode.status. Since
bd points inside the freed descriptor, does this overwrite freed memory?

>  	}
>

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