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

Frank Li
2026-02-20T20:28:43Z

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
>