[PATCH] dmaengine: pl330: Fix lock-order inversion in the channel release path

[PATCH] dmaengine: pl330: Fix lock-order inversion in the channel release path

[Ginger Li]

pl330 uses two locks per transfer: the channel lock pch->lock and the
controller lock pl330->lock. My static analyzer reported that they can
be taken concurrently in opposite orders, leading to potential deadlocks.

The terminate/stop paths take pch->lock first and then pl330->lock, e.g.
  pl330_terminate_all()
    -> spin_lock_irqsave(&pch->lock, flags);
    -> spin_lock(&pl330->lock);
and
  pl330_pause()
    -> spin_lock_irqsave(&pch->lock, flags);
    -> spin_lock(&pl330->lock);

While on the other hand, the channel release path takes pl330->lock first 
and then pch->lock in pl330_free_chan_resources():
  pl330_free_chan_resources()
    -> spin_lock_irqsave(&pl330->lock, flags);
    -> pl330_release_channel(pch->thread);
    -> dma_pl330_rqcb()
      -> spin_lock_irqsave(&pch->lock, flags);

Freeing a channel (dma_release_channel() -> dma_chan_put() ->
pl330_free_chan_resources()) while another CPU is in pl330_terminate_all()
or pl330_pause() on the same channel is therefore an ABBA deadlock: the 
one thread spins on pl330->lock that the other holds, and vice versa.

Inspecting the driver code suggests the rule that dma_pl330_rqcb() must 
not be called with pl330->lock held: pl330_dotask() and the callback 
drain loop  in pl330_update() both releases pl330->lock before their 
dma_pl330_rqcb() calls. Thus, pl330_release_channel() is expected 
to follow the same manner.

Let pl330_release_channel() manage pl330->lock itself and keep the two
dma_pl330_rqcb() calls outside the critical section, and stop holding
pl330->lock around the call in pl330_free_chan_resources().

This was found by a static analyzer on Linux 7.3-rc4; it reported
DeadLock::AllLock (Certain) for

    pl330_free_chan_resources() <-> pl330_terminate_all()
    pl330_free_chan_resources() <-> pl330_pause().

Fixes: 91539eb1fda2 ("dmaengine: pl330: fix double lock")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/dma/pl330.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1805,16 +1805,32 @@
 
 static void pl330_release_channel(struct pl330_thread *thrd)
 {
+	struct pl330_dmac *pl330;
+	unsigned long flags;
+
 	if (!thrd || thrd->free)
 		return;
 
+	pl330 = thrd->dmac;
+
+	spin_lock_irqsave(&pl330->lock, flags);
 	_stop(thrd);
+	spin_unlock_irqrestore(&pl330->lock, flags);
 
+	/*
+	 * dma_pl330_rqcb() takes the channel lock, which is acquired before
+	 * pl330->lock on the terminate/pause/tx_status paths.  Calling it with
+	 * pl330->lock held would invert the lock order, so keep it outside the
+	 * critical section - the same convention pl330_dotask() and
+	 * pl330_update() already follow.
+	 */
 	dma_pl330_rqcb(thrd->req[1 - thrd->lstenq].desc, PL330_ERR_ABORT);
 	dma_pl330_rqcb(thrd->req[thrd->lstenq].desc, PL330_ERR_ABORT);
 
+	spin_lock_irqsave(&pl330->lock, flags);
 	_free_event(thrd, thrd->ev);
 	thrd->free = true;
+	spin_unlock_irqrestore(&pl330->lock, flags);
 }
 
 /* Initialize the structure for PL330 configuration, that can be used
@@ -2358,9 +2374,15 @@
 	tasklet_kill(&pch->task);
 
 	pm_runtime_get_sync(pch->dmac->ddma.dev);
-	spin_lock_irqsave(&pl330->lock, flags);
 
+	/*
+	 * pl330_release_channel() takes pl330->lock itself and calls
+	 * dma_pl330_rqcb(), which takes the channel lock.  It must therefore
+	 * not be called with pl330->lock held (see the comment there).
+	 */
 	pl330_release_channel(pch->thread);
+
+	spin_lock_irqsave(&pl330->lock, flags);
 	pch->thread = NULL;
 
 	if (pch->cyclic)
-- 
2.43.0