From 7c3cc078620064a6664a456942ff15def5fae68f Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Thu, 12 Sep 2024 13:51:23 +0000 Subject: [PATCH] Revert "jump_label: Fix concurrency issues in static_key_slow_dec()" This reverts commit 6b8ccab544d0704813b5e1364ac4e589b32a7ea1 which is commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 upstream. It breaks the Android kernel abi and can be brought back in the future in an abi-safe way if it is really needed. Bug: 161946584 Change-Id: Ie36013c12e969a77b3f68bd37a3b4caab877d593 Signed-off-by: Greg Kroah-Hartman --- kernel/jump_label.c | 45 ++++++++++++++++----------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/kernel/jump_label.c b/kernel/jump_label.c index eec802175ccc..d9c822bbffb8 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -131,7 +131,7 @@ bool static_key_fast_inc_not_disabled(struct static_key *key) STATIC_KEY_CHECK_USE(key); /* * Negative key->enabled has a special meaning: it sends - * static_key_slow_inc/dec() down the slow path, and it is non-zero + * static_key_slow_inc() down the slow path, and it is non-zero * so it counts as "enabled" in jump_label_update(). Note that * atomic_inc_unless_negative() checks >= 0, so roll our own. */ @@ -150,7 +150,7 @@ bool static_key_slow_inc_cpuslocked(struct static_key *key) lockdep_assert_cpus_held(); /* - * Careful if we get concurrent static_key_slow_inc/dec() calls; + * Careful if we get concurrent static_key_slow_inc() calls; * later calls must wait for the first one to _finish_ the * jump_label_update() process. At the same time, however, * the jump_label_update() call below wants to see @@ -247,32 +247,20 @@ EXPORT_SYMBOL_GPL(static_key_disable); static bool static_key_slow_try_dec(struct static_key *key) { - int v; + int val; + + val = atomic_fetch_add_unless(&key->enabled, -1, 1); + if (val == 1) + return false; /* - * Go into the slow path if key::enabled is less than or equal than - * one. One is valid to shut down the key, anything less than one - * is an imbalance, which is handled at the call site. - * - * That includes the special case of '-1' which is set in - * static_key_slow_inc_cpuslocked(), but that's harmless as it is - * fully serialized in the slow path below. By the time this task - * acquires the jump label lock the value is back to one and the - * retry under the lock must succeed. + * 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. */ - v = atomic_read(&key->enabled); - do { - /* - * Warn about the '-1' case though; since that means a - * decrement is concurrent with a first (0->1) increment. IOW - * people are trying to disable something that wasn't yet fully - * enabled. This suggests an ordering problem on the user side. - */ - WARN_ON_ONCE(v < 0); - if (v <= 1) - return false; - } while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1))); - + WARN(val < 0, "jump label: negative count!\n"); return true; } @@ -283,11 +271,10 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key) if (static_key_slow_try_dec(key)) return; - guard(mutex)(&jump_label_mutex); - if (atomic_cmpxchg(&key->enabled, 1, 0)) + jump_label_lock(); + if (atomic_dec_and_test(&key->enabled)) jump_label_update(key); - else - WARN_ON_ONCE(!static_key_slow_try_dec(key)); + jump_label_unlock(); } static void __static_key_slow_dec(struct static_key *key)