Received-SPF: pass (google.com: domain of linux-kernel+bounces-94481-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 6 Mar 2024 11:31:01 -0800
From: Josh Poimboeuf <jpoimboe@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Sam Sun <samsun1006219@gmail.com>, linux-kernel@vger.kernel.org,
	syzkaller@googlegroups.com, xrivendell7@gmail.com, ardb@kernel.org,
	jbaron@akamai.com, peterz@infradead.org, linux-mm@kvack.org,
	akpm@linux-foundation.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Bug] WARNING in static_key_disable_cpuslocked
Message-ID: <20240306193101.s2g33o4viqi2azf3@treble>
References: <CAEkJfYNNZftjpYBpnH4tEnm82orKtQ6SQn9i3sg7YNO-Df3tSQ@mail.gmail.com>
 <20240306105420.6a6bea2c@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240306105420.6a6bea2c@gandalf.local.home>

On Wed, Mar 06, 2024 at 10:54:20AM -0500, Steven Rostedt wrote:
> 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?

Apparently that's an allowed scenario, as the jump label code seems to
be actively trying to support it.  Basically the last one "wins".

See for example:

  1dbb6704de91 ("jump_label: Fix concurrent static_key_enable/disable()")

Also the purpose of the first atomic_read() is to do a quick test before
grabbing the jump lock.  So instead of grabbing the jump lock earlier,
it should actually do the first test atomically:

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d9c822bbffb8..f29c47930d46 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -191,11 +191,14 @@ EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
 void static_key_enable_cpuslocked(struct static_key *key)
 {
+	int tmp;
+
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
-	if (atomic_read(&key->enabled) > 0) {
-		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
+	tmp = atomic_read(&key->enabled);
+	if (tmp != 0) {
+		WARN_ON_ONCE(tmp != 1);
 		return;
 	}
 
@@ -222,11 +225,14 @@ EXPORT_SYMBOL_GPL(static_key_enable);
 
 void static_key_disable_cpuslocked(struct static_key *key)
 {
+	int tmp;
+
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
-	if (atomic_read(&key->enabled) != 1) {
-		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
+	tmp = atomic_read(&key->enabled);
+	if (tmp != 1) {
+		WARN_ON_ONCE(tmp != 0);
 		return;
 	}