Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     peterz@infradead.org
Cc:     pbonzini@redhat.com, ard.biesheuvel@linaro.org, tglx@linutronix.de,
        mingo@kernel.org, linux-kernel@vger.kernel.org,
        oss-drivers@netronome.com,
        Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: [PATCH] locking/static_key: Fix false positive warnings on concurrent dec/inc
Date:   Mon, 18 Mar 2019 14:58:14 -0700
Message-Id: <20190318215814.3724-1-jakub.kicinski@netronome.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Even though the atomic_dec_and_mutex_lock() in
__static_key_slow_dec_cpuslocked() can never see a negative
value in key->enabled the subsequent sanity check is re-reading
key->enabled, which may have been set to -1 in the meantime by
static_key_slow_inc_cpuslocked().

Instead of using -1 as a "enable in progress" constant use
-0xffff, this way we can still treat smaller negative values
as errors.

Alternatively we could implement atomic_dec_and_mutex_lock_return().

Fixes: 4c5ea0a9cd02 ("locking/static_key: Fix concurrent static_key_slow_inc()")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 kernel/jump_label.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index bad96b476eb6..4a227e70a8f3 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -89,7 +89,7 @@ static void jump_label_update(struct static_key *key);
 int static_key_count(struct static_key *key)
 {
 	/*
-	 * -1 means the first static_key_slow_inc() is in progress.
+	 * -0xffff means the first static_key_slow_inc() is in progress.
 	 *  static_key_enabled() must return true, so return 1 here.
 	 */
 	int n = atomic_read(&key->enabled);
@@ -125,7 +125,10 @@ void static_key_slow_inc_cpuslocked(struct static_key *key)
 
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
-		atomic_set(&key->enabled, -1);
+		/* Use a large enough negative number so we can still
+		 * catch underflow bugs in static_key_slow_dec().
+		 */
+		atomic_set(&key->enabled, -0xffff);
 		jump_label_update(key);
 		/*
 		 * Ensure that if the above cmpxchg loop observes our positive
@@ -158,7 +161,7 @@ void static_key_enable_cpuslocked(struct static_key *key)
 
 	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
-		atomic_set(&key->enabled, -1);
+		atomic_set(&key->enabled, -0xffff);
 		jump_label_update(key);
 		/*
 		 * See static_key_slow_inc().
@@ -208,15 +211,11 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key,
 {
 	lockdep_assert_cpus_held();
 
-	/*
-	 * The negative count check is valid even when a negative
-	 * key->enabled is in use by static_key_slow_inc(); a
-	 * __static_key_slow_dec() before the first static_key_slow_inc()
-	 * returns is unbalanced, because all other static_key_slow_inc()
-	 * instances block while the update is in progress.
-	 */
 	if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
-		WARN(atomic_read(&key->enabled) < 0,
+		int v;
+
+		v = atomic_read(&key->enabled);
+		WARN(v < 0 && v != -0xffff,
 		     "jump label: negative count!\n");
 		return;
 	}
-- 
2.19.2