Re: [PATCH v4 28/33] dmaengine: sf-pdma: route error callbacks through channel BH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] dmaengine: sf-pdma: unlocked chan->desc read in
  sf_pdma_errbh_tasklet()
- [Medium] dmaengine: sf-pdma: stale error tx_result on reused
  descriptors
--

commit cbd0063b16af2f825d62b2888a259c514966e4ac
Author: Allen Pais <allen.lkml@gmail.com>

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

This commit routes failed transfer completions in sf-pdma through
vchan_cookie_complete() with DMA_TRANS_ABORTED instead of invoking the
client callback directly from the error tasklet, and kills the driver
tasklets before killing the channel BH on removal.

> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index ca60fb46ade00..992f9f1037028 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -326,9 +326,21 @@ static void sf_pdma_errbh_tasklet(struct tasklet_struct *t)
>  
>  	spin_lock_irqsave(&chan->lock, flags);
>  	if (chan->retries <= 0) {
> -		/* fail to recover */
> +		/* fail to recover: abort this descriptor and move on */
> +		chan->retries = MAX_RETRY;
> +		chan->xfer_err = false;
>  		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);

[Severity: High]
Can this cause a use-after-free or NULL pointer dereference on desc?

sf_pdma_errbh_tasklet() reads desc = chan->desc at entry before acquiring
chan->vchan.lock, and neither sf_pdma_terminate_all() nor
sf_pdma_free_chan_resources() synchronizes with err_tasklet when freeing
chan->desc:

CPU0                                    CPU1
sf_pdma_errbh_tasklet()
  desc = chan->desc;
                                        sf_pdma_terminate_all()
                                          spin_lock_irqsave(&chan->vchan.lock, ...)
                                          kfree(chan->desc);
                                          chan->desc = NULL;
                                          spin_unlock_irqrestore(&chan->vchan.lock, ...)
  ...
  spin_lock_irqsave(&chan->vchan.lock, ...)
  list_del(&desc->vdesc.node);          <-- use-after-free
  vchan_cookie_complete(&desc->vdesc);

If sf_pdma_terminate_all() or sf_pdma_donebh_tasklet() clears or replaces
chan->desc before sf_pdma_errbh_tasklet() runs, desc can also be NULL or
already completed, corrupting the virt-dma descriptor lists.  Re-reading
chan->desc under chan->vchan.lock and checking that it is non-NULL before
unlinking and completing it would avoid this.

> +		desc->vdesc.tx_result.result = DMA_TRANS_ABORTED;
> +		desc->vdesc.tx_result.residue = desc->xfer_size;
> +		vchan_cookie_complete(&desc->vdesc);

[Severity: Medium]
Does a reusable descriptor (DMA_CTRL_REUSE) keep reporting
DMA_TRANS_ABORTED on later successful transfers after an abort?

sf_pdma_probe() sets pdma->dma_dev.descriptor_reuse = true, and
vchan_tx_prep() only initializes tx_result to DMA_TRANS_NOERROR and
residue 0 when the descriptor is first prepared.  Once
sf_pdma_errbh_tasklet() overwrites desc->vdesc.tx_result here, neither
sf_pdma_xfer_desc() nor sf_pdma_donebh_tasklet() resets tx_result back to
DMA_TRANS_NOERROR with zero residue, so subsequent successful completions
of the reused descriptor will pass the stale error result and residue to
the client callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794315350.git.allen.lkml@gmail.com?part=28