Re: [PATCH] dmaengine: sun6i: Fix potential deadlock in dma_terminate_all and pause

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use of spin_lock_irq()/spin_unlock_irq() unconditionally re-enables hardware interrupts, breaking caller atomic contexts.

Pre-existing issues:
- [High] Lockless reads of vchan->phy introduce Time-of-Check to Time-of-Use (TOCTOU) races causing cross-channel interference.
- [High] Active, non-cyclic DMA descriptors are leaked upon channel termination.
--

commit 5ad36fe6cb5ac09d034a98b6efc7e6a3152d1705
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: sun6i: Fix potential deadlock in dma_terminate_all and pause

This patch addresses a deadlock between process context and the DMA tasklet
in softirq context. It replaces plain spin_lock() and spin_unlock() with
their irq variants for sdev->lock in sun6i_dma_pause() and
sun6i_dma_terminate_all().

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..37b143a8f75e2 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -898,9 +898,9 @@ static int sun6i_dma_pause(struct dma_chan *chan)
>  		writel(DMA_CHAN_PAUSE_PAUSE,
>  		       pchan->base + DMA_CHAN_PAUSE);
>  	} else {
> -		spin_lock(&sdev->lock);
> +		spin_lock_irq(&sdev->lock);
>  		list_del_init(&vchan->node);
> -		spin_unlock(&sdev->lock);
> +		spin_unlock_irq(&sdev->lock);

[Severity: High]
Does spin_unlock_irq() unintentionally enable hardware interrupts for callers
that enter this function with interrupts disabled?

DMA engine API functions like dmaengine_pause() are frequently called by
client drivers from atomic contexts where interrupts are already disabled (for
example, while holding a lock via spin_lock_irqsave()). Since
spin_unlock_irq() unconditionally re-enables hardware interrupts, could this
destroy the caller's atomicity and lead to scheduling while atomic panics or
deadlocks?

Would it be safer to use spin_lock_irqsave() and spin_unlock_irqrestore()
here instead?

[ ... ]

> @@ -939,9 +939,9 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  	unsigned long flags;
>  	LIST_HEAD(head);

[Severity: High]
This is a pre-existing issue, but does fetching the physical channel pointer
locklessly at the start of this function introduce a race condition?

At the top of sun6i_dma_terminate_all(), we have:

    struct sun6i_pchan *pchan = vchan->phy;

If a DMA transfer completes concurrently on another CPU, the tasklet might
clear vchan->phy and reassign the same pchan to a different virtual channel.
When sun6i_dma_terminate_all() later acquires the lock and calls writel() to
stop the channel, could it mistakenly stop an unrelated transfer that was just
started on that physical channel?

[Severity: High]
This is a pre-existing issue, but does sun6i_dma_terminate_all() leak active
non-cyclic descriptors?

When terminating, the function has logic to rescue cyclic descriptors and move
them to the completed list:

    if (vchan->cyclic) {
        vchan->cyclic = false;
        if (pchan && pchan->desc) {
            ...
            list_add_tail(&vd->node, &vc->desc_completed);
        }
    }

However, for non-cyclic transfers, it ignores the active descriptor entirely
and sets pchan->desc = NULL. Because vchan_get_all_descriptors() only grabs
descriptors from the tracking lists, the active non-cyclic descriptor is
bypassed and lost. Could this lead to a permanent memory leak of sun6i_desc
structures and their associated hardware pools when non-cyclic transfers are
aborted?

> -	spin_lock(&sdev->lock);
> +	spin_lock_irq(&sdev->lock);
>  	list_del_init(&vchan->node);
> -	spin_unlock(&sdev->lock);
> +	spin_unlock_irq(&sdev->lock);

[Severity: High]
Similar to sun6i_dma_pause(), will this spin_unlock_irq() unconditionally
enable hardware interrupts and break the caller's atomic context?

Also, because it re-enables interrupts before the immediately following
spin_lock_irqsave(&vchan->vc.lock, flags) call, could it corrupt the IRQ
state saved in flags?

>  
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705082105.13325-1-zenghongling@kylinos.cn?part=1