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

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

[Chengfeng Ye]

As &sdev->lock is acquired by tasklet sun6i_dma_tasklet() executed under
softirq context, other acquisition of the same lock under process context
should disable irq, otherwise deadlock could happen if the soft irq preempt
the execution while the lock is held in process context on the same CPU.

sun6i_dma_terminate_all() and sun6i_dma_pause() callbacks acquire the same
lock without disabling irq inside the function.

Possible deadlock scenario:
sun6i_dma_pause()
    -> spin_lock(&sdev->lock);
        <tasklet softirq interruption>
        -> sun6i_dma_tasklet()
        -> spin_lock_irq(&sdev->lock) (deadlock here)

This flaw was found by an experimental static analysis tool I am developing
for irq-related deadlock.

The tentative patch fixes the potential deadlock by spin_lock_irqsave() to
disable softirq.

Changelog:
v1 - >v2
- Use spin_lock_irqsave() instead of spin_lock_bh(), since outside caller
  could already call with bh disable, in the case spin_unlock_bh() would
  unintentionally enable bh.

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
 drivers/dma/sun6i-dma.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index ebfd29888b2f..30f426299703 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -866,6 +866,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);
 
@@ -873,9 +874,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;
@@ -914,9 +915,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);
 
-- 
2.17.1

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

[Chengfeng Ye]

Dear maintainers,

May I ask if someone would like to spend some time reviewing the patch?

Thanks,
Chengfeng

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

[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>

---
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(-)

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);
-- 
2.25.1

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

[sashiko-bot]

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);

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

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

[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>

---
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.
---
 drivers/dma/sun6i-dma.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a9a254dbf8cb..a6487971ad09 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;
@@ -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);
 
-- 
2.25.1

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The DMA interrupt handler erroneously uses the relative inner loop index `j` to resolve physical channels, causing false transfer completions and silent memory corruption for channels >= 8.
- [High] Unprotected read of `vchan->phy` before acquiring locks creates severe TOCTOU races, risking memory corruption and crashes.
- [High] Concurrent execution of `sun6i_dma_interrupt` and `sun6i_dma_terminate_all` causes a NULL pointer dereference on `pchan->desc`.
- [High] Calling `sun6i_dma_resume` on an already pending channel corrupts the `sdev->pending` list.
- [High] `sun6i_dma_terminate_all()` permanently leaks the active descriptor and its DMA pool entries for non-cyclic transfers.
- [High] Use-after-free when traversing the LLI linked list in the error handling paths of `sun6i_dma_prep_slave_sg()` and `sun6i_dma_prep_dma_cyclic()`.
--

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

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

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.

Cc: stable@vger.kernel.org

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..a6487971ad09e 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 isn't a bug introduced by this patch, but vchan->phy is read without
holding vchan->vc.lock. Can this allow pchan to become stale if the
tasklet completes the transfer and reassigns the physical channel to a
different virtual channel before the locks are acquired?

> +	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;

[ ... ]

> @@ -939,9 +940,9 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)

[Severity: High]
This is a pre-existing issue, but vchan->phy is read unlocked in this
function as well. Could the stale pchan be used to modify hardware registers
and descriptor lists for the wrong transfer inside the lock?

>  	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);


[Severity: High]
This isn't a bug introduced by this patch, but does sun6i_dma_terminate_all()
permanently leak the active descriptor and its DMA pool entries for non-cyclic
transfers?

When a transfer is aborted, the code clears pchan->desc:

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
	...
	if (vchan->cyclic) {
		vchan->cyclic = false;
		if (pchan && pchan->desc) {
			...
			list_add_tail(&vd->node, &vc->desc_completed);
		}
	}

	vchan_get_all_descriptors(&vchan->vc, &head);

	if (pchan) {
		...
		pchan->desc = NULL;
	}
	...
}

It doesn't append the active descriptor to any cleanup list for non-cyclic
transfers before clearing it, which means vchan_get_all_descriptors() fails
to capture it.


[Severity: Critical]
This is a pre-existing issue, but does the DMA interrupt handler erroneously
use the relative inner loop index 'j' to resolve physical channels?

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
	...
	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
		...
		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
			pchan = sdev->pchans + j;
	...
}

On SoCs with more than 8 channels, shouldn't this use the absolute index
(i * DMA_IRQ_CHAN_NR + j)? Using 'j' always resolves to channels 0-7, which
could falsely complete unrelated channels and cause silent memory corruption
for channels >= 8.


[Severity: High]
This isn't a bug introduced by this patch, but can concurrent execution of
sun6i_dma_interrupt() and sun6i_dma_terminate_all() cause a NULL pointer
dereference?

In the hard IRQ handler, sun6i_dma_interrupt() acquires the lock and
unconditionally dereferences pchan->desc:

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
	...
	if (vchan && (status & vchan->irq_type)) {
		if (vchan->cyclic) {
			vchan_cyclic_callback(&pchan->desc->vd);
		} else {
			spin_lock(&vchan->vc.lock);
			vchan_cookie_complete(&pchan->desc->vd);
			pchan->done = pchan->desc;
			spin_unlock(&vchan->vc.lock);
		}
	}
	...
}

If sun6i_dma_terminate_all() runs concurrently, it acquires vchan->vc.lock,
forcefully stops the channel, and sets pchan->desc = NULL. When the interrupt
handler subsequently acquires the lock, wouldn't it dereference the NULL
pchan->desc?


[Severity: High]
This is a pre-existing issue, but does calling sun6i_dma_resume() on an
already pending channel corrupt the sdev->pending list?

drivers/dma/sun6i-dma.c:sun6i_dma_resume() {
	...
	} else if (!list_empty(&vchan->vc.desc_issued)) {
		spin_lock(&sdev->lock);
		list_add_tail(&vchan->node, &sdev->pending);
		spin_unlock(&sdev->lock);
	}
	...
}

Unlike sun6i_dma_issue_pending(), this fails to check
!list_empty(&vchan->node) before adding the node. If a channel is already
waiting in the sdev->pending list, couldn't this re-addition corrupt the
list pointers?


[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
when traversing the LLI linked list in the error handling paths?

drivers/dma/sun6i-dma.c:sun6i_dma_prep_slave_sg() {
	...
err_lli_free:
	for (p_lli = txd->p_lli, v_lli = txd->v_lli; v_lli;
	     p_lli = v_lli->p_lli_next, v_lli = v_lli->v_lli_next)
		dma_pool_free(sdev->pool, v_lli, p_lli);
	...
}

The loop body frees v_lli immediately, but the loop's update expression
then reads v_lli->p_lli_next and v_lli->v_lli_next from the just-freed
memory. This same pattern also appears in sun6i_dma_prep_dma_cyclic().

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