Re: [PATCH 3/5] dmaengine: sf-pdma: fix NULL pointer dereference in error and done handlers

[Frank Li <Frank.li@nxp.com>]

On Sat, Feb 21, 2026 at 03:43:55AM +0800, Max Hsu wrote:
> Fix NULL pointer dereferences in both the error and done tasklets that
> can occur due to race conditions during channel termination or completion.
>
> Both tasklets (sf_pdma_errbh_tasklet and sf_pdma_donebh_tasklet)
> dereference chan->desc without checking if it's NULL. However,
> chan->desc can be NULL in legitimate scenarios:
>
> 1. During sf_pdma_terminate_all(): The function sets chan->desc = NULL
>    while holding vchan.lock, but interrupts for previously submitted
>    transactions could fire after the lock is released, before the
>    hardware is fully quiesced. These interrupts can schedule tasklets
>    that will run with chan->desc = NULL.
>
> 2. During channel cleanup: Similar race condition during
>    sf_pdma_free_chan_resources().
>
> The fix adds NULL checks at the beginning of both tasklets, protected
> by vchan.lock, using the same lock that terminate_all and
> free_chan_resources use when setting chan->desc = NULL. This ensures
> that either:
> - The descriptor is valid and we can safely process it, or
> - The descriptor was already freed and we safely skip processing
>
> Fixes: 6973886ad58e ("dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00")
> Cc: stable@vger.kernel.org
> Signed-off-by: Max Hsu <max.hsu@sifive.com>
> ---
>  drivers/dma/sf-pdma/sf-pdma.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index ac7d3b127a24..70e4afcda52a 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -298,33 +298,56 @@ static void sf_pdma_free_desc(struct virt_dma_desc *vdesc)
>  static void sf_pdma_donebh_tasklet(struct tasklet_struct *t)
>  {
>  	struct sf_pdma_chan *chan = from_tasklet(chan, t, done_tasklet);
> +	struct sf_pdma_desc *desc;
>  	unsigned long flags;
>
> -	spin_lock_irqsave(&chan->lock, flags);
> -	if (chan->xfer_err) {
> -		chan->retries = MAX_RETRY;
> -		chan->status = DMA_COMPLETE;
> -		chan->xfer_err = false;
> +	spin_lock_irqsave(&chan->vchan.lock, flags);
> +	desc = chan->desc;
> +	if (!desc) {
> +		/*
> +		 * The descriptor was already freed (e.g., by terminate_all
> +		 * or completion on another CPU). Nothing to do.
> +		 */
> +		spin_unlock_irqrestore(&chan->vchan.lock, flags);
> +		return;

suggest use scoped_guard(spin_lock_irqsave)(&chan->lock)

Frank
>  	}
> -	spin_unlock_irqrestore(&chan->lock, flags);
>
> -	spin_lock_irqsave(&chan->vchan.lock, flags);
> -	list_del(&chan->desc->vdesc.node);
> -	vchan_cookie_complete(&chan->desc->vdesc);
> +	list_del(&desc->vdesc.node);
> +	vchan_cookie_complete(&desc->vdesc);
>
>  	chan->desc = sf_pdma_get_first_pending_desc(chan);
>  	if (chan->desc)
>  		sf_pdma_xfer_desc(chan);
>
>  	spin_unlock_irqrestore(&chan->vchan.lock, flags);
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	if (chan->xfer_err) {
> +		chan->retries = MAX_RETRY;
> +		chan->status = DMA_COMPLETE;
> +		chan->xfer_err = false;
> +	}
> +	spin_unlock_irqrestore(&chan->lock, flags);
>  }
>
>  static void sf_pdma_errbh_tasklet(struct tasklet_struct *t)
>  {
>  	struct sf_pdma_chan *chan = from_tasklet(chan, t, err_tasklet);
> -	struct sf_pdma_desc *desc = chan->desc;
> +	struct sf_pdma_desc *desc;
>  	unsigned long flags;
>
> +	spin_lock_irqsave(&chan->vchan.lock, flags);
> +	desc = chan->desc;
> +	if (!desc) {
> +		/*
> +		 * The descriptor was already freed (e.g., by terminate_all
> +		 * or completion on another CPU). Nothing to do.
> +		 */
> +		spin_unlock_irqrestore(&chan->vchan.lock, flags);
> +		return;
> +	}
> +	spin_unlock_irqrestore(&chan->vchan.lock, flags);
> +
>  	spin_lock_irqsave(&chan->lock, flags);
>  	if (chan->retries <= 0) {
>  		/* fail to recover */
>
> --
> 2.43.0
>