Re: [PATCH v2 29/64] dmaengine: sf-pdma: route error callbacks through channel BH

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

New issues:
- [Critical] Concurrent execution of errbh_tasklet and donebh_tasklet causes the driver to corrupt the active DMA hardware state.
- [High] Race condition between errbh_tasklet and donebh_tasklet causes a kernel panic via BUG_ON() during descriptor completion.

Pre-existing issues:
- [High] Double free of the active descriptor during channel termination or resource cleanup.
- [High] Memory and resource leak in `sf_pdma_remove` due to premature channel list removal.
- [High] Lockless read of `chan->desc` in `sf_pdma_errbh_tasklet` leads to use-after-free and virt-dma list corruption.
- [High] Missing NULL pointer check on `chan->desc` causes a kernel panic on concurrent termination or stray interrupts.
- [Medium] Residue reporting always returns 0 for actively running transfers.
--

--- Patch [29]: [PATCH v2 29/64] dmaengine: sf-pdma: route error callbacks through channel BH ---
commit 4199e17e8ac9bf5b639472caa463955dc33436b1
Author: Allen Pais <[email protected]>

dmaengine: sf-pdma: route error callbacks through channel BH

This patch modifies the error handling path for SF-PDMA to route completions
through the virt-dma per-channel bottom half instead of invoking callbacks
directly from the error tasklet. It updates the error tasklet to complete
the aborted descriptor using virt-dma primitives and start the next transfer.
It also attempts to ensure tasklets are killed before the channel BH during
removal.

> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index 6f79cc28703e6..a6c2f53f005ed 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -328,7 +328,17 @@ static void sf_pdma_errbh_tasklet(struct tasklet_struct *t)

[Severity: High]
This is a pre-existing issue, but is there a risk of a use-after-free
here? At the beginning of sf_pdma_errbh_tasklet(), we read the descriptor
pointer outside of any lock:

sf_pdma_errbh_tasklet() {
    ...
    struct sf_pdma_desc *desc = chan->desc;
    unsigned long flags;

    spin_lock_irqsave(&chan->lock, flags);
}

If a client concurrently calls sf_pdma_terminate_all(), it frees chan->desc.
Since the tasklet later unconditionally dereferences this cached pointer,
could this corrupt memory?

>  	if (chan->retries <= 0) {
>  		/* fail to recover */
>  		spin_unlock_irqrestore(&chan->lock, flags);
> -		dmaengine_desc_get_callback_invoke(desc->async_tx, NULL);
> +
> +		spin_lock_irqsave(&chan->vchan.lock, flags);
> +		list_del(&desc->vdesc.node);
> +		desc->vdesc.tx_result.result = DMA_TRANS_ABORTED;
> +		desc->vdesc.tx_result.residue = desc->xfer_size;
> +		vchan_cookie_complete(&desc->vdesc);

[Severity: High]
If both sf_pdma_donebh_tasklet() and sf_pdma_errbh_tasklet() run concurrently
due to rapid sequential interrupts, can they race on this completion?

If the done tasklet acquires the lock first, it completes the descriptor
and clears its cookie. When this error tasklet acquires the lock next, it
will attempt to complete the exact same descriptor again. Will this double
completion trigger a BUG_ON() in the core dmaengine code?

> +
> +		chan->desc = sf_pdma_get_first_pending_desc(chan);
> +		if (chan->desc)
> +			sf_pdma_xfer_desc(chan);

[Severity: Critical]
Could this hardware re-triggering logic corrupt the DMA state if the done
tasklet is executing simultaneously?

If this error tasklet completes the current descriptor and starts the next
one in hardware, the done tasklet might still assume the original descriptor
is active. Will the done tasklet then incorrectly complete the actively
running descriptor and start a third one, corrupting the DMA registers
during an active transfer?

> +		spin_unlock_irqrestore(&chan->vchan.lock, flags);
>  	} else {
>  		/* retry */

[ ... ]

> @@ -602,9 +612,9 @@ static void sf_pdma_remove(struct platform_device *pdev)
>  		devm_free_irq(&pdev->dev, ch->txirq, ch);
>  		devm_free_irq(&pdev->dev, ch->errirq, ch);
>  		list_del(&ch->vchan.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does removing this device node early
break the dmaengine core cleanup?

When sf_pdma_remove() calls list_del() here, it empties the channels list.
Later in the removal process, dma_async_device_unregister() iterates over
the channels list to free resources like per-cpu memory and IDAs. Since
the list is already empty, will those resources be permanently leaked?

> -		tasklet_kill(&ch->vchan.task);
>  		tasklet_kill(&ch->done_tasklet);
>  		tasklet_kill(&ch->err_tasklet);
> +		dma_chan_kill_bh(&ch->vchan.chan);
>  	}


[Severity: High]
This is a pre-existing issue, but can sf_pdma_terminate_all() trigger
a double free of the active descriptor?

sf_pdma_terminate_all() explicitly frees the descriptor:

sf_pdma_terminate_all() {
    ...
    kfree(chan->desc);
    chan->desc = NULL;
    ...
    vchan_get_all_descriptors(&chan->vchan, &head);
    ...
    vchan_dma_desc_free_list(&chan->vchan, &head);
}

Because the active descriptor isn't removed from the virt-dma desc_issued
list before being freed, won't vchan_get_all_descriptors() collect it
again, causing vchan_dma_desc_free_list() to free the same pointer a
second time?


[Severity: High]
This is a pre-existing issue, but is there a missing NULL pointer check
when handling partial completions?

If a concurrent client terminates the channel, chan->desc is set to NULL.
However, the done ISR unconditionally dereferences it:

sf_pdma_done_isr() {
    ...
    struct sf_pdma_desc *desc = chan->desc;

    desc->src_addr += desc->xfer_size - residue;
    ...
}

Could this cause a kernel panic if the ISR fires right after the channel
is terminated?


[Severity: Medium]
This is a pre-existing issue, but does sf_pdma_desc_residue() fail to
report accurate remaining bytes for actively running transfers?

sf_pdma_desc_residue() only searches the submitted list:

sf_pdma_desc_residue() {
    ...
    list_for_each_entry(vd, &chan->vchan.desc_submitted, node)
        if (vd->tx.cookie == cookie)
            tx = &vd->tx;

    if (!tx)
        goto out;
    ...
}

Since actively running descriptors are moved to the desc_issued list,
won't this always fail to find the active transfer and incorrectly return
0 remaining bytes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=29