From: Arnd Bergmann <arnd@arndb.de>
To: Anton Vorontsov <anton@enomsg.org>, Colin Cross <ccross@android.com>,
        Kees Cook <keescook@chromium.org>, Tony Luck <tony.luck@intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>, Joel Fernandes <joelaf@google.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Geliang Tang <geliangtang@gmail.com>, linux-kernel@vger.kernel.org
Subject: [PATCH] pstore: fix warning about conditional locking
Date: Wed, 16 Nov 2016 16:15:41 +0100
Message-Id: <20161116151630.3235006-1-arnd@arndb.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4514
Lines: 141

gcc cannot prove that "prz->flags" remains constant over the
function, so it warns that we might restore flags that have
never been saved:

fs/pstore/ram_core.c: In function ‘buffer_size_add’:
include/linux/spinlock.h:246:3: error: ‘flags’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

This reworks the code to avoid the spinlock entirely, by actually
using the atomic_t for atomic updates. The resulting code is
more efficient and easier to understand, and it avoids the
local variable that the compiler did not track correctly.

Fixes: 95937ddce59a ("pstore: Allow prz to control need for locking")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/pstore/ram.c            |  4 +---
 fs/pstore/ram_core.c       | 46 ++++++++++++++++++----------------------------
 include/linux/pstore_ram.h |  8 --------
 3 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 380222432eff..cb66ba710349 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -787,9 +787,7 @@ static int ramoops_probe(struct platform_device *pdev)
 				: 1;
 	err = ramoops_init_przs("ftrace", dev, cxt, &cxt->fprzs, &paddr,
 				cxt->ftrace_size, -1,
-				&cxt->max_ftrace_cnt, LINUX_VERSION_CODE,
-				(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)
-					? PRZ_FLAG_NO_LOCK : 0);
+				&cxt->max_ftrace_cnt, LINUX_VERSION_CODE, 0);
 	if (err)
 		goto fail_init_fprz;
 
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0cc23cb18719..87be7bdfaf48 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -51,21 +51,17 @@ static inline size_t buffer_start(struct persistent_ram_zone *prz)
 /* increase and wrap the start pointer, returning the old value */
 static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
 {
-	int old;
-	int new;
-	unsigned long flags;
-
-	if (!(prz->flags & PRZ_FLAG_NO_LOCK))
-		raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+	int old, new, before;
 
 	old = atomic_read(&prz->buffer->start);
-	new = old + a;
-	while (unlikely(new >= prz->buffer_size))
-		new -= prz->buffer_size;
-	atomic_set(&prz->buffer->start, new);
+	do {
+		new = old + a;
+		while (unlikely(new >= prz->buffer_size))
+			new -= prz->buffer_size;
 
-	if (!(prz->flags & PRZ_FLAG_NO_LOCK))
-		raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
+		before = old;
+		old = atomic_cmpxchg(&prz->buffer->start, before, new);
+	} while (old != before);
 
 	return old;
 }
@@ -73,25 +69,20 @@ static size_t buffer_start_add(struct persistent_ram_zone *prz, size_t a)
 /* increase the size counter until it hits the max size */
 static void buffer_size_add(struct persistent_ram_zone *prz, size_t a)
 {
-	size_t old;
-	size_t new;
-	unsigned long flags;
-
-	if (!(prz->flags & PRZ_FLAG_NO_LOCK))
-		raw_spin_lock_irqsave(&prz->buffer_lock, flags);
+	size_t old, new, before;
 
 	old = atomic_read(&prz->buffer->size);
-	if (old == prz->buffer_size)
-		goto exit;
+	do {
+		if (old == prz->buffer_size)
+			break;
 
-	new = old + a;
-	if (new > prz->buffer_size)
-		new = prz->buffer_size;
-	atomic_set(&prz->buffer->size, new);
+		new = old + a;
+		if (new > prz->buffer_size)
+			new = prz->buffer_size;
 
-exit:
-	if (!(prz->flags & PRZ_FLAG_NO_LOCK))
-		raw_spin_unlock_irqrestore(&prz->buffer_lock, flags);
+		before = old;
+		old = atomic_cmpxchg(&prz->buffer->size, before, new);
+	} while (old != before);
 }
 
 static void notrace persistent_ram_encode_rs8(struct persistent_ram_zone *prz,
@@ -496,7 +487,6 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
 
 	prz->buffer->sig = sig;
 	persistent_ram_zap(prz);
-	prz->buffer_lock = __RAW_SPIN_LOCK_UNLOCKED(buffer_lock);
 	prz->flags = flags;
 
 	return 0;
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9395f06e8372..e755803ba58a 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -24,13 +24,6 @@
 #include <linux/list.h>
 #include <linux/types.h>
 
-/*
- * Choose whether access to the RAM zone requires locking or not.  If a zone
- * can be written to from different CPUs like with ftrace for example, then
- * PRZ_FLAG_NO_LOCK is used. For all other cases, locking is required.
- */
-#define PRZ_FLAG_NO_LOCK	BIT(0)
-
 struct persistent_ram_buffer;
 struct rs_control;
 
@@ -48,7 +41,6 @@ struct persistent_ram_zone {
 	struct persistent_ram_buffer *buffer;
 	size_t buffer_size;
 	u32 flags;
-	raw_spinlock_t buffer_lock;
 
 	/* ECC correction */
 	char *par_buffer;
-- 
2.9.0