linux-kernel - [PATCH] dmaengine: pl330: Fix some race conditions in residue calculation

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-next>] [day] [month] [year] [list]

Message-ID: <1456319674.2867.15.camel@linaro.org>
Date:	Wed, 24 Feb 2016 13:14:34 +0000
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	Vinod Koul <vinod.koul@...el.com>,
	Robert Baldyga <r.baldyga@...sung.com>
Cc:	Lukasz Czerwinski <l.czerwinski@...sung.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Jaswinder Singh <jassisinghbrar@...il.com>,
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: [PATCH] dmaengine: pl330: Fix some race conditions in residue
 calculation

The residue calculation in pl330_tx_status doesn't handle transitional
states that occur at the time one descriptor (A) is completed and the
next (B) is started. Specifically, both A and B can simultaneously be in
the BUSY state and at this time the thread's 'req_running' may (or may
not) be -1.

To cope with this situation we change the code to ensure A is treated as
complete and B as having not yet started. Prior to the change, the code
would calculate a transferred byte count as if both A and B had
completed.

Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")

Signed-off-by: Jon Medhurst <tixy@...aro.org>
---

I discovered this issue when trying to work out why audio stopped
working on ARM's Juno platform and bisected it to commit aee4d1fac887.
Whilst this patch seems to fix the problems I was seeing, I can't help
but think there are more race conditions with this code. E.g. if the
running descriptor changes under us, pl330_get_current_xferred_count
can end up reading values from hardware that relate to a different
descriptor. And if we're really unlucky, the reading of the 'val' and
'addr' values in pl330_get_current_xferred_count can come from different
descriptors. I don't know if there is any locks we can use to prevent
such races or if we need to try and detect when things have changed and
redo/abort the residue calculation...

 drivers/dma/pl330.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 17ee758..55e3c5f 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	struct dma_pl330_desc *desc, *running = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned int transferred, residual = 0;
+	bool first_busy;

 	ret = dma_cookie_status(chan, cookie, txstate);

@@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,

 	if (pch->thread->req_running != -1)
 		running = pch->thread->req[pch->thread->req_running].desc;
+	first_busy = true;

 	/* Check in pending list */
 	list_for_each_entry(desc, &pch->work_list, node) {
 		if (desc->status == DONE)
 			transferred = desc->bytes_requested;
-		else if (running && desc == running)
-			transferred =
-				pl330_get_current_xferred_count(pch, desc);
-		else
+		else if (desc->status == BUSY && first_busy) {
+			first_busy = false;
+			if (running && desc == running) {
+				transferred =
+					pl330_get_current_xferred_count(pch, desc);
+			} else {
+				/* BUSY but not running means it's just completed */
+				transferred = desc->bytes_requested;
+			}
+		} else {
+			/*
+			 * Descriptor is either in PREP state queued for future
+			 * transfer or it is the second BUSY descriptor we have
+			 * seen. The latter case means it has just, or is about
+			 * to be, started, so treat it as having not yet
+			 * transferred any bytes, the same as PREP.
+			 */
 			transferred = 0;
+		}
 		residual += desc->bytes_requested - transferred;
 		if (desc->txd.cookie == cookie) {
 			switch (desc->status) {
-- 
2.1.4