Re: [Bug] WARNING in static_key_disable_cpuslocked

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 5 Mar 2024 15:54:24 +0800
Sam Sun <samsun1006219@gmail.com> wrote:

> We analyzed the cause of this bug. It seems that in function
> static_key_disable_cpuslocked(), there is a small racing window
> between two atomic_read(&key->enabled) in line 228 & 229, triggering
> the WARN_ON_ONCE macro. This might cause function returned without
> actually disabling the static_key "key". I am not sure if there is any
> other potential threat here.
> 
> I searched on web and found that there was a similar bug reported by
> syzbot several years ago
> (https://groups.google.com/g/syzkaller-bugs/c/_W3lgRCwlb4/m/TqzyQcPpAQAJ).
> At that time the fix was in the net instead of kernel/jump_label.c. So
> I checked the call stack and cc this email to memory management
> maintainers. Hope there is no confusion.
> 
> If you have any questions, please contact us.
> Reported by: Yue Sun <samsun1006219@gmail.com>
> Reported by: xingwei lee <xrivendell7@gmail.com>

Thanks for the report. I wonder if it simply needs to add the tests in the
locking? Like the patch below.

Because I could see:

	CPU 0				CPU 1
	-----				-----

key->enabled = 0
        
 static_key_enable_cpus_locked()
   jump_label_lock();
				  static_key_disable_cpus_locked()
				      if (key->enabled != 1) {

    key->enabled = 1;
				          WARN_ON(key->enabled != 0)

Now I guess the question is, why is something trying to disable something
that is not enabled? Is the above scenario OK? Or should the users of
static_key also prevent this?

-- Steve


diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d9c822bbffb8..f154caf2a62b 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -194,12 +194,12 @@ void static_key_enable_cpuslocked(struct static_key *key)
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
+	jump_label_lock();
 	if (atomic_read(&key->enabled) > 0) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
-		return;
+		goto unlock;
 	}
 
-	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
 		atomic_set(&key->enabled, -1);
 		jump_label_update(key);
@@ -208,6 +208,7 @@ void static_key_enable_cpuslocked(struct static_key *key)
 		 */
 		atomic_set_release(&key->enabled, 1);
 	}
+unlock:
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
@@ -225,14 +226,15 @@ void static_key_disable_cpuslocked(struct static_key *key)
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
+	jump_label_lock();
 	if (atomic_read(&key->enabled) != 1) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
-		return;
+		goto unlock;
 	}
 
-	jump_label_lock();
 	if (atomic_cmpxchg(&key->enabled, 1, 0))
 		jump_label_update(key);
+unlock:
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);