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

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

[Hongling Zeng]

sun6i_dma_terminate_all() and sun6i_dma_pause() acquire sdev->lock
with plain spin_lock() from process context. If a DMA interrupt fires
on the same CPU while the lock is held, the interrupt handler schedules
sun6i_dma_tasklet(), which runs in softirq context and attempts to
acquire the same lock with spin_lock_irq(), causing a deadlock.

Fix by using spin_lock_irq() to disable interrupts while holding
sdev->lock, consistent with other call sites.

Fixes: ba489fd46ab6 ("dmaengine: sun6i: Add support for Allwinner A31 DMA controller")
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 drivers/dma/sun6i-dma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a9a254dbf8cb..37b143a8f75e 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);
 	}
 
 	return 0;
@@ -939,9 +939,9 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
 	unsigned long flags;
 	LIST_HEAD(head);
 
-	spin_lock(&sdev->lock);
+	spin_lock_irq(&sdev->lock);
 	list_del_init(&vchan->node);
-	spin_unlock(&sdev->lock);
+	spin_unlock_irq(&sdev->lock);
 
 	spin_lock_irqsave(&vchan->vc.lock, flags);
 
-- 
2.25.1

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

[sashiko-bot]

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