Re: [PATCH] dmaengine: sun6i: Fix potential deadlock in sun6i_dma_terminate_all()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional hardware interrupt re-enablement violates caller lock state.

Pre-existing issues:
- [High] Incomplete deadlock fix: sun6i_dma_pause() contains the exact same deadlock vulnerability.
- [High] Lockless read of vchan->phy causes Time-Of-Check to Time-Of-Use (TOCTOU) race condition and hardware corruption.
--

commit 1435f9b5204cbc43aa5b41f133e1e3e88387cce1
Author: Hongling Zeng <zenghongling@kylinos.cn>
dmaengine: sun6i: Fix potential deadlock in sun6i_dma_terminate_all()

This commit modifies sun6i_dma_terminate_all() to use spin_lock_irq()
instead of plain spin_lock() when acquiring sdev->lock. The change aims
to prevent a deadlock caused when a DMA interrupt fires while the lock
is held.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..840f7276ad5d5 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -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 reading vchan->phy locklessly at
the start of this function create a race condition?

In sun6i_dma_terminate_all():

static int sun6i_dma_terminate_all(struct dma_chan *chan)
{
        ...
        struct sun6i_pchan *pchan = vchan->phy;
        ...
        spin_lock_irqsave(&vchan->vc.lock, flags);
        ...
        if (pchan) {
                writel(DMA_CHAN_ENABLE_STOP, pchan->base + DMA_CHAN_ENABLE);

If sun6i_dma_tasklet() completes a transfer, clears vchan->phy, and
reassigns the physical channel to a different virtual channel before the
lock is acquired here, would this operate on a stale pchan and stop
an unrelated transfer?

Similar lockless reads of vchan->phy exist in sun6i_dma_pause(),
sun6i_dma_resume(), and sun6i_dma_tx_status().

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

[Severity: High]
Does this unconditional spin_unlock_irq() risk breaking callers that
invoke dmaengine_terminate_all() with interrupts already disabled?

Since spin_unlock_irq() forces hardware interrupts back on, it could
destroy the caller's interrupt-disabled state and lead to unexpected
nested interrupts. Is it safer to use spin_lock_irqsave() and
spin_unlock_irqrestore() here?

[Severity: High]
This is a pre-existing issue, but does sun6i_dma_pause() have the exact
same deadlock vulnerability being fixed here?

sun6i_dma_pause() acquires the same lock without disabling interrupts:

static int sun6i_dma_pause(struct dma_chan *chan)
{
        ...
        } else {
                spin_lock(&sdev->lock);
                list_del_init(&vchan->node);
                spin_unlock(&sdev->lock);
        }

Could a DMA interrupt fire while this lock is held in process context
and cause the same tasklet deadlock?

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

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