linux-kernel - [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1507117044-3456-2-git-send-email-al.kochet@gmail.com>
Date:   Wed,  4 Oct 2017 14:37:23 +0300
From:   Alexander Kochetkov <al.kochet@...il.com>
To:     dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Vinod Koul <vinod.koul@...el.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Alexander Kochetkov <al.kochet@...il.com>
Subject: [PATCH v2 1/2] dmaengine: pl330: fix descriptor allocation fail

If two concurrent threads call pl330_get_desc() when DMAC descriptor
pool is empty it is possible that allocation for one of threads will fail
with message:

kernel: dma-pl330 20078000.dma-controller: pl330_get_desc:2469 ALERT!

Here how that can happen. Thread A calls pl330_get_desc() to get
descriptor. If DMAC descriptor pool is empty pl330_get_desc() allocates
new descriptor on shared pool using add_desc() and then get newly
allocated descriptor using pluck_desc(). At the same time thread B calls
pluck_desc() and take newly allocated descriptor. In that case descriptor
allocation for thread A will fail.

Using on-stack pool for new descriptor allow avoid the issue described.
The patch modify pl330_get_desc() to use on-stack pool for allocation
new descriptors.

Signed-off-by: Alexander Kochetkov <al.kochet@...il.com>
---
 drivers/dma/pl330.c |   39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index b19ee04..deec4a4 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2390,7 +2390,8 @@ static inline void _init_desc(struct dma_pl330_desc *desc)
 }
 
 /* Returns the number of descriptors added to the DMAC pool */
-static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
+static int add_desc(struct list_head *pool, spinlock_t *lock,
+		    gfp_t flg, int count)
 {
 	struct dma_pl330_desc *desc;
 	unsigned long flags;
@@ -2400,27 +2401,28 @@ static int add_desc(struct pl330_dmac *pl330, gfp_t flg, int count)
 	if (!desc)
 		return 0;
 
-	spin_lock_irqsave(&pl330->pool_lock, flags);
+	spin_lock_irqsave(lock, flags);
 
 	for (i = 0; i < count; i++) {
 		_init_desc(&desc[i]);
-		list_add_tail(&desc[i].node, &pl330->desc_pool);
+		list_add_tail(&desc[i].node, pool);
 	}
 
-	spin_unlock_irqrestore(&pl330->pool_lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 
 	return count;
 }
 
-static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
+static struct dma_pl330_desc *pluck_desc(struct list_head *pool,
+					 spinlock_t *lock)
 {
 	struct dma_pl330_desc *desc = NULL;
 	unsigned long flags;
 
-	spin_lock_irqsave(&pl330->pool_lock, flags);
+	spin_lock_irqsave(lock, flags);
 
-	if (!list_empty(&pl330->desc_pool)) {
-		desc = list_entry(pl330->desc_pool.next,
+	if (!list_empty(pool)) {
+		desc = list_entry(pool->next,
 				struct dma_pl330_desc, node);
 
 		list_del_init(&desc->node);
@@ -2429,7 +2431,7 @@ static struct dma_pl330_desc *pluck_desc(struct pl330_dmac *pl330)
 		desc->txd.callback = NULL;
 	}
 
-	spin_unlock_irqrestore(&pl330->pool_lock, flags);
+	spin_unlock_irqrestore(lock, flags);
 
 	return desc;
 }
@@ -2441,20 +2443,18 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 	struct dma_pl330_desc *desc;
 
 	/* Pluck one desc from the pool of DMAC */
-	desc = pluck_desc(pl330);
+	desc = pluck_desc(&pl330->desc_pool, &pl330->pool_lock);
 
 	/* If the DMAC pool is empty, alloc new */
 	if (!desc) {
-		if (!add_desc(pl330, GFP_ATOMIC, 1))
-			return NULL;
+		DEFINE_SPINLOCK(lock);
+		LIST_HEAD(pool);
 
-		/* Try again */
-		desc = pluck_desc(pl330);
-		if (!desc) {
-			dev_err(pch->dmac->ddma.dev,
-				"%s:%d ALERT!\n", __func__, __LINE__);
+		if (!add_desc(&pool, &lock, GFP_ATOMIC, 1))
 			return NULL;
-		}
+
+		desc = pluck_desc(&pool, &lock);
+		WARN_ON(!desc || !list_empty(&pool));
 	}
 
 	/* Initialize the descriptor */
@@ -2868,7 +2868,8 @@ static int __maybe_unused pl330_resume(struct device *dev)
 	spin_lock_init(&pl330->pool_lock);
 
 	/* Create a descriptor pool of default size */
-	if (!add_desc(pl330, GFP_KERNEL, NR_DEFAULT_DESC))
+	if (!add_desc(&pl330->desc_pool, &pl330->pool_lock,
+		      GFP_KERNEL, NR_DEFAULT_DESC))
 		dev_warn(&adev->dev, "unable to allocate desc\n");
 
 	INIT_LIST_HEAD(&pd->channels);
-- 
1.7.9.5