diff mbox series

[v2] dmaengine: sun6i: Fix potential deadlock on sdev->lock

Message ID 20260706030347.22730-1-zenghongling@kylinos.cn (mailing list archive)
State New
Headers
Series [v2] dmaengine: sun6i: Fix potential deadlock on sdev->lock |

Commit Message

Hongling Zeng July 6, 2026, 3:03 a.m. UTC 
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>

---
Change in v2:
 -Fix by using spin_lock_irqsave() and spin_unlock_irqrestore() to
  disable interrupts while holding sdev->lock and properly save/restore
  interrupt state.
 -Fix inconsistent locking pattern in sun6i_dma_resume() and
  sun6i_dma_issue_pending().
---
 drivers/dma/sun6i-dma.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

sashiko-bot@kernel.org July 6, 2026, 3:11 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch incorrectly uses spin_lock_irq() and spin_unlock_irq() inside an existing spin_lock_irqsave() critical section, prematurely enabling local interrupts and risking a recursive spinlock deadlock.

Pre-existing issues:
- [High] Lockless reading of vchan->phy (pchan) leads to a time-of-check to time-of-use (TOCTOU) race condition with concurrent channel reallocation.
--

commit 540abaecd705cc77dff5d58414b0bd84a163d5b5
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: sun6i: Fix potential deadlock on sdev->lock

This patch replaces plain spin_lock() with spin_lock_irq() and 
spin_lock_irqsave() around sdev->lock acquisitions in process context. This 
aims to prevent a deadlock where the lock is held by a process and a DMA 
interrupt schedules a softirq tasklet that attempts to acquire the same lock.

Cc: stable@vger.kernel.org

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..742a0822a8da7 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -891,6 +891,7 @@ static int sun6i_dma_pause(struct dma_chan *chan)
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct sun6i_pchan *pchan = vchan->phy;

[Severity: High]
This is a pre-existing issue, but does reading vchan->phy locklessly lead to a
time-of-check to time-of-use (TOCTOU) race condition?

If sun6i_dma_tasklet() runs concurrently in softirq context, it could complete
the current transfer, clear vchan->phy, and reassign the physical channel to a
different virtual channel.

The original thread would then continue using the stale pchan pointer, applying
commands like PAUSE_PAUSE to a physical channel now actively processing a
different client's transfer.

[ ... ]

> @@ -921,9 +922,9 @@ static int sun6i_dma_resume(struct dma_chan *chan)
>  		writel(DMA_CHAN_PAUSE_RESUME,
>  		       pchan->base + DMA_CHAN_PAUSE);
>  	} else if (!list_empty(&vchan->vc.desc_issued)) {
> -		spin_lock(&sdev->lock);
> +		spin_lock_irq(&sdev->lock);
>  		list_add_tail(&vchan->node, &sdev->pending);
> -		spin_unlock(&sdev->lock);
> +		spin_unlock_irq(&sdev->lock);

[Severity: High]
Does this spin_unlock_irq() prematurely enable local interrupts inside the
spin_lock_irqsave(&vchan->vc.lock, flags) critical section?

Since spin_unlock_irq() unconditionally enables local interrupts, it breaks the
protection established by the outer irqsave lock.

If a DMA hardware interrupt for the same virtual channel fires immediately after
this unlock, the interrupt handler will attempt to acquire vchan->vc.lock. Since
the interrupted thread already holds that lock, would this result in a recursive
spinlock deadlock?

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

[ ... ]

> @@ -1021,7 +1022,7 @@ static void sun6i_dma_issue_pending(struct dma_chan *chan)
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
>  	if (vchan_issue_pending(&vchan->vc)) {
> -		spin_lock(&sdev->lock);
> +		spin_lock_irq(&sdev->lock);
>  
>  		if (!vchan->phy && list_empty(&vchan->node)) {
>  			list_add_tail(&vchan->node, &sdev->pending);
> @@ -1030,7 +1031,7 @@ static void sun6i_dma_issue_pending(struct dma_chan *chan)
>  				&vchan->vc);
>  		}
>  
> -		spin_unlock(&sdev->lock);
> +		spin_unlock_irq(&sdev->lock);

[Severity: High]
Similarly here, does this unconditionally enable interrupts while the outer
vchan->vc.lock is still held?

Could this also expose the CPU to a recursive spinlock deadlock if an interrupt
arrives before the outer spin_unlock_irqrestore() is called?

>  	} else {
>  		dev_dbg(chan2dev(chan), "vchan %p: nothing to issue\n",
>  			&vchan->vc);
diff mbox series

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a9a254dbf8cb..742a0822a8da 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -891,6 +891,7 @@  static int sun6i_dma_pause(struct dma_chan *chan)
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
 	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
 	struct sun6i_pchan *pchan = vchan->phy;
+	unsigned long flags;
 
 	dev_dbg(chan2dev(chan), "vchan %p: pause\n", &vchan->vc);
 
@@ -898,9 +899,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_irqsave(&sdev->lock, flags);
 		list_del_init(&vchan->node);
-		spin_unlock(&sdev->lock);
+		spin_unlock_irqrestore(&sdev->lock, flags);
 	}
 
 	return 0;
@@ -921,9 +922,9 @@  static int sun6i_dma_resume(struct dma_chan *chan)
 		writel(DMA_CHAN_PAUSE_RESUME,
 		       pchan->base + DMA_CHAN_PAUSE);
 	} else if (!list_empty(&vchan->vc.desc_issued)) {
-		spin_lock(&sdev->lock);
+		spin_lock_irq(&sdev->lock);
 		list_add_tail(&vchan->node, &sdev->pending);
-		spin_unlock(&sdev->lock);
+		spin_unlock_irq(&sdev->lock);
 	}
 
 	spin_unlock_irqrestore(&vchan->vc.lock, flags);
@@ -939,9 +940,9 @@  static int sun6i_dma_terminate_all(struct dma_chan *chan)
 	unsigned long flags;
 	LIST_HEAD(head);
 
-	spin_lock(&sdev->lock);
+	spin_lock_irqsave(&sdev->lock, flags);
 	list_del_init(&vchan->node);
-	spin_unlock(&sdev->lock);
+	spin_unlock_irqrestore(&sdev->lock, flags);
 
 	spin_lock_irqsave(&vchan->vc.lock, flags);
 
@@ -1021,7 +1022,7 @@  static void sun6i_dma_issue_pending(struct dma_chan *chan)
 	spin_lock_irqsave(&vchan->vc.lock, flags);
 
 	if (vchan_issue_pending(&vchan->vc)) {
-		spin_lock(&sdev->lock);
+		spin_lock_irq(&sdev->lock);
 
 		if (!vchan->phy && list_empty(&vchan->node)) {
 			list_add_tail(&vchan->node, &sdev->pending);
@@ -1030,7 +1031,7 @@  static void sun6i_dma_issue_pending(struct dma_chan *chan)
 				&vchan->vc);
 		}
 
-		spin_unlock(&sdev->lock);
+		spin_unlock_irq(&sdev->lock);
 	} else {
 		dev_dbg(chan2dev(chan), "vchan %p: nothing to issue\n",
 			&vchan->vc);