linux-kernel - [PATCH] locking/static_key: Fix false positive warnings on concurrent dec/inc

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]

Date:   Mon, 18 Mar 2019 14:58:14 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     peterz@...radead.org
Cc:     pbonzini@...hat.com, ard.biesheuvel@...aro.org, tglx@...utronix.de,
        mingo@...nel.org, linux-kernel@...r.kernel.org,
        oss-drivers@...ronome.com,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: [PATCH] locking/static_key: Fix false positive warnings on concurrent dec/inc

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@...ronome.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