On 3/6/24 2:31 PM, Josh Poimboeuf wrote:
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:
Makes sense but the enable path can also set key->enabled to -1. So I
think a concurrent disable could then see the -1 in tmp and still
trigger the WARN. So I think we could change the WARN to be:
WARN_ON_ONCE(tmp != 0 && tmp != -1). And also add a similar check
for enable if we have enable vs enable racing?
Although it seems like the set key->enabled to -1 while used in the
inc/dec API isn't really doing anything in the enable/disable part here?
But then the key->enabled I think has to move in front of the
jump_label_update() to make that part work right...
Thanks,
-Jason
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;
}
