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

[sashiko-bot@kernel.org]

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