[PATCH] static_key: fix concurrent static_key_slow_inc

[PATCH] static_key: fix concurrent static_key_slow_inc

[Paolo Bonzini]

The following scenario is possible:

    CPU 1                                   CPU 2
    static_key_slow_inc
     atomic_inc_not_zero
      -> key.enabled == 0, no increment
     jump_label_lock
     atomic_inc_return
      -> key.enabled == 1 now
                                            static_key_slow_inc
                                             atomic_inc_not_zero
                                              -> key.enabled == 1, inc to 2
                                             return
                                            ** static key is wrong!
     jump_label_update
     jump_label_unlock

Testing the static key at the point marked by (**) will follow the wrong
path for jumps that have not been patched yet.  This can actually happen
when creating many KVM virtual machines with userspace LAPIC emulation;
just run several copies of the following program:

    #include <fcntl.h>
    #include <unistd.h>
    #include <sys/ioctl.h>
    #include <linux/kvm.h>

    int main(void)
    {
        for (;;) {
            int kvmfd = open("/dev/kvm", O_RDONLY);
            int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
            close(ioctl(vmfd, KVM_CREATE_VCPU, 1));
            close(vmfd);
            close(kvmfd);
        }
        return 0;
    }

Every KVM_CREATE_VCPU ioctl will attempt a static_key_slow_inc.  The
static key's purpose is to skip NULL pointer checks and indeed one of
the processes eventually dereferences NULL.

As explained in the commit that introduced the bug (which is 706249c222f6,
"locking/static_keys: Rework update logic", 2015-07-24), jump_label_update
needs key.enabled to be true.  The solution adopted here is to temporarily
make key.enabled == -1, and use go down the slow path when key.enabled
<= 0.

Cc: stable@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Fixes: 706249c222f6
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	So it wasn't as easy as I thought :) and this patch is
	a bit clumsy, but I cannot think of anything better.

 include/linux/jump_label.h | 16 +++++++++++++---
 kernel/jump_label.c        | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 0536524bb9eb..4bed5cefdaa9 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -117,13 +117,18 @@ struct module;
 
 #include <linux/atomic.h>
 
+#ifdef HAVE_JUMP_LABEL
+
 static inline int static_key_count(struct static_key *key)
 {
-	return atomic_read(&key->enabled);
+	/*
+	 * -1 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);
+	return n >= 0 ? n : 1;
 }
 
-#ifdef HAVE_JUMP_LABEL
-
 #define JUMP_TYPE_FALSE	0UL
 #define JUMP_TYPE_TRUE	1UL
 #define JUMP_TYPE_MASK	1UL
@@ -162,6 +167,11 @@ extern void jump_label_apply_nops(struct module *mod);
 
 #else  /* !HAVE_JUMP_LABEL */
 
+static inline int static_key_count(struct static_key *key)
+{
+	return atomic_read(&key->enabled);
+}
+
 static __always_inline void jump_label_init(void)
 {
 	static_key_initialized = true;
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 05254eeb4b4e..1ce0663bfa33 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -58,13 +58,34 @@ static void jump_label_update(struct static_key *key);
 
 void static_key_slow_inc(struct static_key *key)
 {
+	int v, v1;
 	STATIC_KEY_CHECK_USE();
-	if (atomic_inc_not_zero(&key->enabled))
-		return;
+
+	/*
+	 * 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
+	 * static_key_enabled(&key) for jumps to be updated properly.
+	 *
+	 * So give a special meaning to negative key->enabled: it sends
+	 * 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.
+	 */
+	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
+		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
+		if (likely(v1 == v))
+			return;
+        }
 
 	jump_label_lock();
-	if (atomic_inc_return(&key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0) {
+		atomic_set(&key->enabled, -1);
 		jump_label_update(key);
+		atomic_set(&key->enabled, 1);
+	} else
+		atomic_inc(&key->enabled);
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -72,6 +93,13 @@ EXPORT_SYMBOL_GPL(static_key_slow_inc);
 static void __static_key_slow_dec(struct static_key *key,
 		unsigned long rate_limit, struct delayed_work *work)
 {
+	/*
+	 * 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
+	 * block while the update is in progress.
+	 */
 	if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
 		WARN(atomic_read(&key->enabled) < 0,
 		     "jump label: negative count!\n");
-- 
1.8.3.1

Re: [PATCH] static_key: fix concurrent static_key_slow_inc

[Peter Zijlstra]

On Tue, Jun 21, 2016 at 06:52:17PM +0200, Paolo Bonzini wrote:
> The following scenario is possible:
> 
>     CPU 1                                   CPU 2
>     static_key_slow_inc
>      atomic_inc_not_zero
>       -> key.enabled == 0, no increment
>      jump_label_lock
>      atomic_inc_return
>       -> key.enabled == 1 now
>                                             static_key_slow_inc
>                                              atomic_inc_not_zero
>                                               -> key.enabled == 1, inc to 2
>                                              return
>                                             ** static key is wrong!
>      jump_label_update
>      jump_label_unlock
> 
> Testing the static key at the point marked by (**) will follow the wrong
> path for jumps that have not been patched yet.  This can actually happen
> when creating many KVM virtual machines with userspace LAPIC emulation;
> just run several copies of the following program:
> 
>     #include <fcntl.h>
>     #include <unistd.h>
>     #include <sys/ioctl.h>
>     #include <linux/kvm.h>
> 
>     int main(void)
>     {
>         for (;;) {
>             int kvmfd = open("/dev/kvm", O_RDONLY);
>             int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>             close(ioctl(vmfd, KVM_CREATE_VCPU, 1));
>             close(vmfd);
>             close(kvmfd);
>         }
>         return 0;
>     }
> 
> Every KVM_CREATE_VCPU ioctl will attempt a static_key_slow_inc.  The
> static key's purpose is to skip NULL pointer checks and indeed one of
> the processes eventually dereferences NULL.
> 
> As explained in the commit that introduced the bug (which is 706249c222f6,
> "locking/static_keys: Rework update logic", 2015-07-24), jump_label_update
> needs key.enabled to be true.  The solution adopted here is to temporarily
> make key.enabled == -1, and use go down the slow path when key.enabled
> <= 0.

Thanks!

(I frobbed a whitespace fail and fixed the Fixes line).

Re: [PATCH] static_key: fix concurrent static_key_slow_inc

[Christian Borntraeger]

On 06/21/2016 06:52 PM, Paolo Bonzini wrote:
> The following scenario is possible:
> 
>     CPU 1                                   CPU 2
>     static_key_slow_inc
>      atomic_inc_not_zero
>       -> key.enabled == 0, no increment
>      jump_label_lock
>      atomic_inc_return
>       -> key.enabled == 1 now
>                                             static_key_slow_inc
>                                              atomic_inc_not_zero
>                                               -> key.enabled == 1, inc to 2
>                                              return
>                                             ** static key is wrong!
>      jump_label_update
>      jump_label_unlock
> 
> Testing the static key at the point marked by (**) will follow the wrong
> path for jumps that have not been patched yet.  This can actually happen
> when creating many KVM virtual machines with userspace LAPIC emulation;
> just run several copies of the following program:
> 
>     #include <fcntl.h>
>     #include <unistd.h>
>     #include <sys/ioctl.h>
>     #include <linux/kvm.h>
> 
>     int main(void)
>     {
>         for (;;) {
>             int kvmfd = open("/dev/kvm", O_RDONLY);
>             int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>             close(ioctl(vmfd, KVM_CREATE_VCPU, 1));
>             close(vmfd);
>             close(kvmfd);
>         }
>         return 0;
>     }
> 
> Every KVM_CREATE_VCPU ioctl will attempt a static_key_slow_inc.  The
> static key's purpose is to skip NULL pointer checks and indeed one of
> the processes eventually dereferences NULL.


Interesting. Some time ago I had a spurious bug on the preempt_notifier
when starting/stopping lots of guests, but I was never able to reliably 
reproduce it. I was chasing some other bug, so I did not even considered
static_key to be broken, but this might actually be the fix for that
problem.

Re: [PATCH] static_key: fix concurrent static_key_slow_inc

[Paolo Bonzini]

On 22/06/2016 10:50, Christian Borntraeger wrote:
> On 06/21/2016 06:52 PM, Paolo Bonzini wrote:
>> The following scenario is possible:
>>
>>     CPU 1                                   CPU 2
>>     static_key_slow_inc
>>      atomic_inc_not_zero
>>       -> key.enabled == 0, no increment
>>      jump_label_lock
>>      atomic_inc_return
>>       -> key.enabled == 1 now
>>                                             static_key_slow_inc
>>                                              atomic_inc_not_zero
>>                                               -> key.enabled == 1, inc to 2
>>                                              return
>>                                             ** static key is wrong!
>>      jump_label_update
>>      jump_label_unlock
>>
>> Testing the static key at the point marked by (**) will follow the wrong
>> path for jumps that have not been patched yet.  This can actually happen
>> when creating many KVM virtual machines with userspace LAPIC emulation;
>> just run several copies of the following program:
>>
>>     #include <fcntl.h>
>>     #include <unistd.h>
>>     #include <sys/ioctl.h>
>>     #include <linux/kvm.h>
>>
>>     int main(void)
>>     {
>>         for (;;) {
>>             int kvmfd = open("/dev/kvm", O_RDONLY);
>>             int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>>             close(ioctl(vmfd, KVM_CREATE_VCPU, 1));
>>             close(vmfd);
>>             close(kvmfd);
>>         }
>>         return 0;
>>     }
>>
>> Every KVM_CREATE_VCPU ioctl will attempt a static_key_slow_inc.  The
>> static key's purpose is to skip NULL pointer checks and indeed one of
>> the processes eventually dereferences NULL.
> 
> Interesting. Some time ago I had a spurious bug on the preempt_notifier
> when starting/stopping lots of guests, but I was never able to reliably 
> reproduce it. I was chasing some other bug, so I did not even considered
> static_key to be broken, but this might actually be the fix for that
> problem.

It could be the same that was reported here:
http://article.gmane.org/gmane.comp.emulators.kvm.devel/154069

Paolo

[tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[tip-bot for Paolo Bonzini]

Commit-ID:  4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff
Gitweb:     http://git.kernel.org/tip/4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff
Author:     Paolo Bonzini <pbonzini@redhat.com>
AuthorDate: Tue, 21 Jun 2016 18:52:17 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 24 Jun 2016 08:23:16 +0200

locking/static_key: Fix concurrent static_key_slow_inc()

The following scenario is possible:

    CPU 1                                   CPU 2
    static_key_slow_inc()
     atomic_inc_not_zero()
      -> key.enabled == 0, no increment
     jump_label_lock()
     atomic_inc_return()
      -> key.enabled == 1 now
                                            static_key_slow_inc()
                                             atomic_inc_not_zero()
                                              -> key.enabled == 1, inc to 2
                                             return
                                            ** static key is wrong!
     jump_label_update()
     jump_label_unlock()

Testing the static key at the point marked by (**) will follow the
wrong path for jumps that have not been patched yet.  This can
actually happen when creating many KVM virtual machines with userspace
LAPIC emulation; just run several copies of the following program:

    #include <fcntl.h>
    #include <unistd.h>
    #include <sys/ioctl.h>
    #include <linux/kvm.h>

    int main(void)
    {
        for (;;) {
            int kvmfd = open("/dev/kvm", O_RDONLY);
            int vmfd = ioctl(kvmfd, KVM_CREATE_VM, 0);
            close(ioctl(vmfd, KVM_CREATE_VCPU, 1));
            close(vmfd);
            close(kvmfd);
        }
        return 0;
    }

Every KVM_CREATE_VCPU ioctl will attempt a static_key_slow_inc() call.
The static key's purpose is to skip NULL pointer checks and indeed one
of the processes eventually dereferences NULL.

As explained in the commit that introduced the bug:

  706249c222f6 ("locking/static_keys: Rework update logic")

jump_label_update() needs key.enabled to be true.  The solution adopted
here is to temporarily make key.enabled == -1, and use go down the
slow path when key.enabled <= 0.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <stable@vger.kernel.org> # v4.3+
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 706249c222f6 ("locking/static_keys: Rework update logic")
Link: http://lkml.kernel.org/r/1466527937-69798-1-git-send-email-pbonzini@redhat.com
[ Small stylistic edits to the changelog and the code. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/jump_label.h | 16 +++++++++++++---
 kernel/jump_label.c        | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 0536524..6890446 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -117,13 +117,18 @@ struct module;
 
 #include <linux/atomic.h>
 
+#ifdef HAVE_JUMP_LABEL
+
 static inline int static_key_count(struct static_key *key)
 {
-	return atomic_read(&key->enabled);
+	/*
+	 * -1 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);
+	return n >= 0 ? n : 1;
 }
 
-#ifdef HAVE_JUMP_LABEL
-
 #define JUMP_TYPE_FALSE	0UL
 #define JUMP_TYPE_TRUE	1UL
 #define JUMP_TYPE_MASK	1UL
@@ -162,6 +167,11 @@ extern void jump_label_apply_nops(struct module *mod);
 
 #else  /* !HAVE_JUMP_LABEL */
 
+static inline int static_key_count(struct static_key *key)
+{
+	return atomic_read(&key->enabled);
+}
+
 static __always_inline void jump_label_init(void)
 {
 	static_key_initialized = true;
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 05254ee..4b353e0 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -58,13 +58,36 @@ static void jump_label_update(struct static_key *key);
 
 void static_key_slow_inc(struct static_key *key)
 {
+	int v, v1;
+
 	STATIC_KEY_CHECK_USE();
-	if (atomic_inc_not_zero(&key->enabled))
-		return;
+
+	/*
+	 * 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
+	 * static_key_enabled(&key) for jumps to be updated properly.
+	 *
+	 * So give a special meaning to negative key->enabled: it sends
+	 * 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.
+	 */
+	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
+		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
+		if (likely(v1 == v))
+			return;
+	}
 
 	jump_label_lock();
-	if (atomic_inc_return(&key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0) {
+		atomic_set(&key->enabled, -1);
 		jump_label_update(key);
+		atomic_set(&key->enabled, 1);
+	} else {
+		atomic_inc(&key->enabled);
+	}
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
@@ -72,6 +95,13 @@ EXPORT_SYMBOL_GPL(static_key_slow_inc);
 static void __static_key_slow_dec(struct static_key *key,
 		unsigned long rate_limit, struct delayed_work *work)
 {
+	/*
+	 * 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,
 		     "jump label: negative count!\n");

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Peter Zijlstra]

On Fri, Jun 24, 2016 at 01:59:06AM -0700, tip-bot for Paolo Bonzini wrote:
> +++ b/kernel/jump_label.c
> @@ -58,13 +58,36 @@ static void jump_label_update(struct static_key *key);
>  
>  void static_key_slow_inc(struct static_key *key)
>  {
> +	int v, v1;
> +
>  	STATIC_KEY_CHECK_USE();
> -	if (atomic_inc_not_zero(&key->enabled))
> -		return;
> +
> +	/*
> +	 * 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
> +	 * static_key_enabled(&key) for jumps to be updated properly.
> +	 *
> +	 * So give a special meaning to negative key->enabled: it sends
> +	 * 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.
> +	 */
> +	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> +		if (likely(v1 == v))
> +			return;
> +	}
>  
>  	jump_label_lock();
> -	if (atomic_inc_return(&key->enabled) == 1)
> +	if (atomic_read(&key->enabled) == 0) {
> +		atomic_set(&key->enabled, -1);
>  		jump_label_update(key);
> +		atomic_set(&key->enabled, 1);
> +	} else {
> +		atomic_inc(&key->enabled);
> +	}
>  	jump_label_unlock();
>  }


So I was recently looking at this again and got all paranoid. Do we want
something like so?

---
 kernel/jump_label.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d11c506a6ac3..69d07e78e48b 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -103,7 +103,7 @@ EXPORT_SYMBOL_GPL(static_key_disable);
 
 void static_key_slow_inc(struct static_key *key)
 {
-	int v, v1;
+	int v;
 
 	STATIC_KEY_CHECK_USE();
 
@@ -119,18 +119,28 @@ void static_key_slow_inc(struct static_key *key)
 	 * so it counts as "enabled" in jump_label_update().  Note that
 	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
 	 */
-	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
-		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
-		if (likely(v1 == v))
+	for (v = atomic_read(&key->enabled); v > 0;) {
+		if (atomic_try_cmpxchg(&key->enabled, &v, v+1))
 			return;
 	}
 
 	cpus_read_lock();
 	jump_label_lock();
-	if (atomic_read(&key->enabled) == 0) {
-		atomic_set(&key->enabled, -1);
+
+	if (atomic_try_cmpxchg(&key->enabled, 0, -1)) {
+		/*
+		 * smp_mb implied, must have -1 before proceeding to change
+		 * text.
+		 */
 		jump_label_update(key);
-		atomic_set(&key->enabled, 1);
+
+		/*
+		 * smp_mb, such that we finish modifying text before enabling
+		 * the fast path. Probably not needed because modifying text is
+		 * likely to serialize everything. Be paranoid.
+		 */
+		smp_mb__before_atomic();
+		atomic_add(2, &key->enabled); /* -1 -> 1 */
 	} else {
 		atomic_inc(&key->enabled);
 	}

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Paolo Bonzini]

On 31/07/2017 11:36, Peter Zijlstra wrote:
> On Fri, Jun 24, 2016 at 01:59:06AM -0700, tip-bot for Paolo Bonzini wrote:
>> +++ b/kernel/jump_label.c
>> @@ -58,13 +58,36 @@ static void jump_label_update(struct static_key *key);
>>  
>>  void static_key_slow_inc(struct static_key *key)
>>  {
>> +	int v, v1;
>> +
>>  	STATIC_KEY_CHECK_USE();
>> -	if (atomic_inc_not_zero(&key->enabled))
>> -		return;
>> +
>> +	/*
>> +	 * 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
>> +	 * static_key_enabled(&key) for jumps to be updated properly.
>> +	 *
>> +	 * So give a special meaning to negative key->enabled: it sends
>> +	 * 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.
>> +	 */
>> +	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
>> +		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
>> +		if (likely(v1 == v))
>> +			return;
>> +	}
>>  
>>  	jump_label_lock();
>> -	if (atomic_inc_return(&key->enabled) == 1)
>> +	if (atomic_read(&key->enabled) == 0) {
>> +		atomic_set(&key->enabled, -1);
>>  		jump_label_update(key);
>> +		atomic_set(&key->enabled, 1);
>> +	} else {
>> +		atomic_inc(&key->enabled);
>> +	}
>>  	jump_label_unlock();
>>  }
> 
> 
> So I was recently looking at this again and got all paranoid. Do we want
> something like so?

Though I agree with the paranoia sentiment, no:

- key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
For this reason the atomic_try_cmpxchg is unnecessary.

- the (implied) smp_mb before jump_label_update is not interesting, but
I don't think it is useful because: 1) during the jump_label_update
there is no correspondence between what static_key_enabled returns and
what the text look like; 2) what would it even be pairing with?

- the smp_mb (though it could be a smp_wmb or atomic_set_release)
initially triggered my paranoia indeed.  But even then, I can't see why
you would need it because there's nothing it pairs with.  Rather, it's
*any use of key->enabled outside jump_label_lock* (meaning: any use of
static_key_enabled and static_key_count outside the core jump_label.c
code) that should be handled with care.

And indeed, while there aren't many, I think two of them are wrong (and
not fixed by your patch):

- include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
 It makes no sense to call it outside cpuset_mutex, and indeed that's
how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
<- rebuild_sched_domains_locked).  The function should be moved inside
kernel/cgroup/cpuset.c since the mutex is static.

- kernel/cgroup/cgroup.c only enables/disables at init, so using
static_key_enabled should be fine.

- kernel/tracepoint.c only manipulates tracepoint static keys under
tracepoints_mutex, and uses static_key_enabled under the same mutex, so
it's fine.

- net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
increment of the static key.  It's racy and maybe we should provide a
new API static_key_enable_forever:

	void static_key_enable_forever(struct static_key *key)
	{
	        STATIC_KEY_CHECK_USE();
	        if (atomic_read(&key->enabled) > 0)
	                return;

	        cpus_read_lock();
	        jump_label_lock();
	        if (atomic_read(&key->enabled) == 0) {
	                atomic_set(&key->enabled, -1);
	                jump_label_update(key);
	                atomic_set(&key->enabled, 1);
	        }
	        jump_label_unlock();
	        cpus_read_unlock();
	}
	EXPORT_SYMBOL_GPL(static_key_enable_forever);

I can prepare a patch if you agree.

Paolo

> ---
>  kernel/jump_label.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index d11c506a6ac3..69d07e78e48b 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -103,7 +103,7 @@ EXPORT_SYMBOL_GPL(static_key_disable);
>  
>  void static_key_slow_inc(struct static_key *key)
>  {
> -	int v, v1;
> +	int v;
>  
>  	STATIC_KEY_CHECK_USE();
>  
> @@ -119,18 +119,28 @@ void static_key_slow_inc(struct static_key *key)
>  	 * so it counts as "enabled" in jump_label_update().  Note that
>  	 * atomic_inc_unless_negative() checks >= 0, so roll our own.
>  	 */
> -	for (v = atomic_read(&key->enabled); v > 0; v = v1) {
> -		v1 = atomic_cmpxchg(&key->enabled, v, v + 1);
> -		if (likely(v1 == v))
> +	for (v = atomic_read(&key->enabled); v > 0;) {
> +		if (atomic_try_cmpxchg(&key->enabled, &v, v+1))
>  			return;
>  	}
>  
>  	cpus_read_lock();
>  	jump_label_lock();
> -	if (atomic_read(&key->enabled) == 0) {
> -		atomic_set(&key->enabled, -1);
> +
> +	if (atomic_try_cmpxchg(&key->enabled, 0, -1)) {
> +		/*
> +		 * smp_mb implied, must have -1 before proceeding to change
> +		 * text.
> +		 */
>  		jump_label_update(key);
> -		atomic_set(&key->enabled, 1);
> +
> +		/*
> +		 * smp_mb, such that we finish modifying text before enabling
> +		 * the fast path. Probably not needed because modifying text is
> +		 * likely to serialize everything. Be paranoid.
> +		 */
> +		smp_mb__before_atomic();
> +		atomic_add(2, &key->enabled); /* -1 -> 1 */
>  	} else {
>  		atomic_inc(&key->enabled);
>  	}
> 

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Peter Zijlstra]

On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
> - key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
> For this reason the atomic_try_cmpxchg is unnecessary.

Agreed, the only reason was the implied barrier.

> - the (implied) smp_mb before jump_label_update is not interesting, but
> I don't think it is useful because: 1) during the jump_label_update
> there is no correspondence between what static_key_enabled returns and
> what the text look like; 2) what would it even be pairing with?

Ah, indeed. So I was worried about the text changes escaping upwards,
but you're right in that there's no harm in that because there's nothing
that cares.

Another inc would see 0 and still serialize on the mutex.

> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
> initially triggered my paranoia indeed.  But even then, I can't see why
> you would need it because there's nothing it pairs with.

So this one would pair with the mb implied by the cmpxchg loop for
inc-if-positive. The ordering being that if we see a positive v, we must
then also see all the text modifications.

So if jump_label_update() were to not fully serialize things, it would
be possible for the v=1 store to appear before the last text changes.
And in that case we'd allow the fast path to complete
static_key_slow_inc() before it was in fact done with changing all text.

Now, I suspect (but did not audit) that anything that changes text must
in fact serialize world, but I wasn't sure.

> Rather, it's *any use of key->enabled outside jump_label_lock*
> (meaning: any use of static_key_enabled and static_key_count outside
> the core jump_label.c code) that should be handled with care.

> And indeed, while there aren't many, I think two of them are wrong (and
> not fixed by your patch):
> 
> - include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
>  It makes no sense to call it outside cpuset_mutex, and indeed that's
> how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
> <- rebuild_sched_domains_locked).  The function should be moved inside
> kernel/cgroup/cpuset.c since the mutex is static.

Dima was poking at that code.

> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
> increment of the static key.  It's racy and maybe we should provide a
> new API static_key_enable_forever:
> 
> 	void static_key_enable_forever(struct static_key *key)
> 	{
> 	        STATIC_KEY_CHECK_USE();
> 	        if (atomic_read(&key->enabled) > 0)
> 	                return;
> 
> 	        cpus_read_lock();
> 	        jump_label_lock();
> 	        if (atomic_read(&key->enabled) == 0) {
> 	                atomic_set(&key->enabled, -1);
> 	                jump_label_update(key);
> 	                atomic_set(&key->enabled, 1);
> 	        }
> 	        jump_label_unlock();
> 	        cpus_read_unlock();
> 	}
> 	EXPORT_SYMBOL_GPL(static_key_enable_forever);
> 
> I can prepare a patch if you agree.

Isn't that what we have static_key_enable() for? Which btw also uses
static_key_count() outside of the mutex.

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Paolo Bonzini]

On 31/07/2017 15:33, Peter Zijlstra wrote:
> On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
>> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
>> initially triggered my paranoia indeed.  But even then, I can't see why
>> you would need it because there's nothing it pairs with.
> 
> So this one would pair with the mb implied by the cmpxchg loop for
> inc-if-positive. The ordering being that if we see a positive v, we must
> then also see all the text modifications.
> 
> So if jump_label_update() were to not fully serialize things, it would
> be possible for the v=1 store to appear before the last text changes.
> And in that case we'd allow the fast path to complete
> static_key_slow_inc() before it was in fact done with changing all text.
> 
> Now, I suspect (but did not audit) that anything that changes text must
> in fact serialize world, but I wasn't sure.

I see.  Then yes, I agree a smp_wmb would be nicer here.

>> Rather, it's *any use of key->enabled outside jump_label_lock*
>> (meaning: any use of static_key_enabled and static_key_count outside
>> the core jump_label.c code) that should be handled with care.
> 
>> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
>> increment of the static key.  It's racy and maybe we should provide a
>> new API static_key_enable_forever:
>>
>> 	void static_key_enable_forever(struct static_key *key)
>> 	{
>> 	        STATIC_KEY_CHECK_USE();
>> 	        if (atomic_read(&key->enabled) > 0)
>> 	                return;
>>
>> 	        cpus_read_lock();
>> 	        jump_label_lock();
>> 	        if (atomic_read(&key->enabled) == 0) {
>> 	                atomic_set(&key->enabled, -1);
>> 	                jump_label_update(key);
>> 	                atomic_set(&key->enabled, 1);
>> 	        }
>> 	        jump_label_unlock();
>> 	        cpus_read_unlock();
>> 	}
>> 	EXPORT_SYMBOL_GPL(static_key_enable_forever);
>>
>> I can prepare a patch if you agree.
> 
> Isn't that what we have static_key_enable() for? Which btw also uses
> static_key_count() outside of the mutex.

Yes, they should be fixed and net/ can then use static_key_enable.

Paolo

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Peter Zijlstra]

On Mon, Jul 31, 2017 at 05:35:40PM +0200, Paolo Bonzini wrote:
> On 31/07/2017 15:33, Peter Zijlstra wrote:
> > On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
> >> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
> >> initially triggered my paranoia indeed.  But even then, I can't see why
> >> you would need it because there's nothing it pairs with.
> > 
> > So this one would pair with the mb implied by the cmpxchg loop for
> > inc-if-positive. The ordering being that if we see a positive v, we must
> > then also see all the text modifications.
> > 
> > So if jump_label_update() were to not fully serialize things, it would
> > be possible for the v=1 store to appear before the last text changes.
> > And in that case we'd allow the fast path to complete
> > static_key_slow_inc() before it was in fact done with changing all text.
> > 
> > Now, I suspect (but did not audit) that anything that changes text must
> > in fact serialize world, but I wasn't sure.
> 
> I see.  Then yes, I agree a smp_wmb would be nicer here.

I'll make it atomic_set_release() and a comment.

> >> Rather, it's *any use of key->enabled outside jump_label_lock*
> >> (meaning: any use of static_key_enabled and static_key_count outside
> >> the core jump_label.c code) that should be handled with care.
> > 
> >> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
> >> increment of the static key.  It's racy and maybe we should provide a
> >> new API static_key_enable_forever:
> >>
> >> 	void static_key_enable_forever(struct static_key *key)
> >> 	{
> >> 	        STATIC_KEY_CHECK_USE();
> >> 	        if (atomic_read(&key->enabled) > 0)
> >> 	                return;
> >>
> >> 	        cpus_read_lock();
> >> 	        jump_label_lock();
> >> 	        if (atomic_read(&key->enabled) == 0) {
> >> 	                atomic_set(&key->enabled, -1);
> >> 	                jump_label_update(key);
> >> 	                atomic_set(&key->enabled, 1);
> >> 	        }
> >> 	        jump_label_unlock();
> >> 	        cpus_read_unlock();
> >> 	}
> >> 	EXPORT_SYMBOL_GPL(static_key_enable_forever);
> >>
> >> I can prepare a patch if you agree.
> > 
> > Isn't that what we have static_key_enable() for? Which btw also uses
> > static_key_count() outside of the mutex.
> 
> Yes, they should be fixed and net/ can then use static_key_enable.

Right, let me try and fix _enable().

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Paolo Bonzini]

> > > Isn't that what we have static_key_enable() for? Which btw also uses
> > > static_key_count() outside of the mutex.
> > 
> > Yes, they should be fixed and net/ can then use static_key_enable.
> 
> Right, let me try and fix _enable().

Here is what I scribbled before leaving the office.  (What was missing:
documentation for how to use static_key_enabled/count, testing).


>From c0bdcc89e26fb16cdc564485232bebcd1e0cc102 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 31 Jul 2017 16:46:04 +0200
Subject: [PATCH 1/3] jump_labels: fix concurrent static_key_enable/disable()

static_key_enable/disable are trying to cap the static key count to
0/1.  However, their use of key->enabled is outside jump_label_lock
so they do not really ensure that.

Rewrite them to do a quick check for an already enabled (respectively,
already disabled) key, and then recheck under the jump label lock.  Unlike
static_key_slow_inc/dec, a failed check under the jump label lock does
not modify key->enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/jump_label.h | 22 +++++++++--------
 kernel/jump_label.c        | 59 +++++++++++++++++++++++++++++-----------------
 2 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 2afd74b9d844..8fc5649476ca 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -234,22 +234,24 @@ static inline int jump_label_apply_nops(struct module *mod)
 
 static inline void static_key_enable(struct static_key *key)
 {
-	int count = static_key_count(key);
-
-	WARN_ON_ONCE(count < 0 || count > 1);
+	STATIC_KEY_CHECK_USE();
 
-	if (!count)
-		static_key_slow_inc(key);
+	if (atomic_read(&key->enabled) != 0) {
+		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
+		return;
+	}
+	atomic_set(&key->enabled, 1);
 }
 
 static inline void static_key_disable(struct static_key *key)
 {
-	int count = static_key_count(key);
-
-	WARN_ON_ONCE(count < 0 || count > 1);
+	STATIC_KEY_CHECK_USE();
 
-	if (count)
-		static_key_slow_dec(key);
+	if (atomic_read(&key->enabled) != 1) {
+		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
+		return;
+	}
+	atomic_set(&key->enabled, 0);
 }
 
 #define STATIC_KEY_INIT_TRUE	{ .enabled = ATOMIC_INIT(1) }
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d11c506a6ac3..47a3e927b87e 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -79,28 +79,6 @@ int static_key_count(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_count);
 
-void static_key_enable(struct static_key *key)
-{
-	int count = static_key_count(key);
-
-	WARN_ON_ONCE(count < 0 || count > 1);
-
-	if (!count)
-		static_key_slow_inc(key);
-}
-EXPORT_SYMBOL_GPL(static_key_enable);
-
-void static_key_disable(struct static_key *key)
-{
-	int count = static_key_count(key);
-
-	WARN_ON_ONCE(count < 0 || count > 1);
-
-	if (count)
-		static_key_slow_dec(key);
-}
-EXPORT_SYMBOL_GPL(static_key_disable);
-
 void static_key_slow_inc(struct static_key *key)
 {
 	int v, v1;
@@ -139,6 +117,43 @@ void static_key_slow_inc(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_slow_inc);
 
+void static_key_enable(struct static_key *key)
+{
+	STATIC_KEY_CHECK_USE();
+	if (atomic_read(&key->enabled) != 0) {
+		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
+		return;
+	}
+
+	cpus_read_lock();
+	jump_label_lock();
+	if (atomic_read(&key->enabled) == 0) {
+		atomic_set(&key->enabled, -1);
+		jump_label_update(key);
+		atomic_set(&key->enabled, 1);
+	}
+	jump_label_unlock();
+	cpus_read_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_enable);
+
+void static_key_disable(struct static_key *key)
+{
+	STATIC_KEY_CHECK_USE();
+	if (atomic_read(&key->enabled) != 1) {
+		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
+		return;
+	}
+
+	cpus_read_lock();
+	jump_label_lock();
+	if (atomic_cmpxchg(&key->enabled, 1, 0))
+		jump_label_update(key);
+	jump_label_unlock();
+	cpus_read_unlock();
+}
+EXPORT_SYMBOL_GPL(static_key_disable);
+
 static void __static_key_slow_dec(struct static_key *key,
 		unsigned long rate_limit, struct delayed_work *work)
 {
-- 
2.13.3


>From 89d89520915bb12fd330069ca8aed32a0c216ab6 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 31 Jul 2017 16:48:05 +0200
Subject: [PATCH 2/3] jump_labels: do not use unserialized static_key_enabled

Any use of key->enabled (that is static_key_enabled and static_key_count)
outside jump_label_lock should handle its own serialization.  The only
two that are not doing so are the UDP encapsulation static keys.  Change
them to use static_key_enable, which now correctly tests key->enabled under
the jump label lock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 net/ipv4/udp.c | 3 +--
 net/ipv6/udp.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b057653ceca9..9b305776fe14 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1811,8 +1811,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 static struct static_key udp_encap_needed __read_mostly;
 void udp_encap_enable(void)
 {
-	if (!static_key_enabled(&udp_encap_needed))
-		static_key_slow_inc(&udp_encap_needed);
+	static_key_enable(&udp_encap_needed);
 }
 EXPORT_SYMBOL(udp_encap_enable);
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 4a3e65626e8b..27057c41d648 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -569,8 +569,7 @@ static __inline__ void udpv6_err(struct sk_buff *skb,
 static struct static_key udpv6_encap_needed __read_mostly;
 void udpv6_encap_enable(void)
 {
-	if (!static_key_enabled(&udpv6_encap_needed))
-		static_key_slow_inc(&udpv6_encap_needed);
+	static_key_enable(&udpv6_encap_needed);
 }
 EXPORT_SYMBOL(udpv6_encap_enable);
 
-- 
2.13.3


>From 7d8f5a373c0357663683197dfc64f20abf31a892 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Mon, 31 Jul 2017 16:52:07 +0200
Subject: [PATCH 3/3] cpuset: make nr_cpusets private

Any use of key->enabled (that is static_key_enabled and static_key_count)
outside jump_label_lock should handle its own serialization.  In the case
of cpusets_enabled_key, the key is always incremented/decremented under
cpuset_mutex, and hence the same rule applies to nr_cpusets.  The rule
*is* respected currently, but the mutex is static so nr_cpusets should
be static too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/cpuset.h | 6 ------
 kernel/cgroup/cpuset.c | 7 +++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
index 119a3f9604b0..cedcc910b7a7 100644
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -24,12 +24,6 @@ static inline bool cpusets_enabled(void)
 	return static_branch_unlikely(&cpusets_enabled_key);
 }
 
-static inline int nr_cpusets(void)
-{
-	/* jump label reference count + the top-level cpuset */
-	return static_key_count(&cpusets_enabled_key.key) + 1;
-}
-
 static inline void cpuset_inc(void)
 {
 	static_branch_inc(&cpusets_enabled_key);
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index ca8376e5008c..6ddca2480276 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -576,6 +576,13 @@ static void update_domain_attr_tree(struct sched_domain_attr *dattr,
 	rcu_read_unlock();
 }
 
+/* Must be called with cpuset_mutex held.  */
+static inline int nr_cpusets(void)
+{
+	/* jump label reference count + the top-level cpuset */
+	return static_key_count(&cpusets_enabled_key.key) + 1;
+}
+
 /*
  * generate_sched_domains()
  *
-- 
2.13.3

Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Dima Zavin]

On Mon, Jul 31, 2017 at 6:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
>> - key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
>> For this reason the atomic_try_cmpxchg is unnecessary.
>
> Agreed, the only reason was the implied barrier.
>
>> - the (implied) smp_mb before jump_label_update is not interesting, but
>> I don't think it is useful because: 1) during the jump_label_update
>> there is no correspondence between what static_key_enabled returns and
>> what the text look like; 2) what would it even be pairing with?
>
> Ah, indeed. So I was worried about the text changes escaping upwards,
> but you're right in that there's no harm in that because there's nothing
> that cares.
>
> Another inc would see 0 and still serialize on the mutex.
>
>> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
>> initially triggered my paranoia indeed.  But even then, I can't see why
>> you would need it because there's nothing it pairs with.
>
> So this one would pair with the mb implied by the cmpxchg loop for
> inc-if-positive. The ordering being that if we see a positive v, we must
> then also see all the text modifications.
>
> So if jump_label_update() were to not fully serialize things, it would
> be possible for the v=1 store to appear before the last text changes.
> And in that case we'd allow the fast path to complete
> static_key_slow_inc() before it was in fact done with changing all text.
>
> Now, I suspect (but did not audit) that anything that changes text must
> in fact serialize world, but I wasn't sure.
>
>> Rather, it's *any use of key->enabled outside jump_label_lock*
>> (meaning: any use of static_key_enabled and static_key_count outside
>> the core jump_label.c code) that should be handled with care.
>
>> And indeed, while there aren't many, I think two of them are wrong (and
>> not fixed by your patch):
>>
>> - include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
>>  It makes no sense to call it outside cpuset_mutex, and indeed that's
>> how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
>> <- rebuild_sched_domains_locked).  The function should be moved inside
>> kernel/cgroup/cpuset.c since the mutex is static.
>
> Dima was poking at that code.

As I understand it, in the case of read_mems_allowed, the value of the
key enabled count itself is not used. We just use the branch rewrite
to avoid seqcount lookups, so only the text changes actually matter
there. We'd still need my fix since the problem was ordering of the
text changes.

Thanks!

--Dima

>
>> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
>> increment of the static key.  It's racy and maybe we should provide a
>> new API static_key_enable_forever:
>>
>>       void static_key_enable_forever(struct static_key *key)
>>       {
>>               STATIC_KEY_CHECK_USE();
>>               if (atomic_read(&key->enabled) > 0)
>>                       return;
>>
>>               cpus_read_lock();
>>               jump_label_lock();
>>               if (atomic_read(&key->enabled) == 0) {
>>                       atomic_set(&key->enabled, -1);
>>                       jump_label_update(key);
>>                       atomic_set(&key->enabled, 1);
>>               }
>>               jump_label_unlock();
>>               cpus_read_unlock();
>>       }
>>       EXPORT_SYMBOL_GPL(static_key_enable_forever);
>>
>> I can prepare a patch if you agree.
>
> Isn't that what we have static_key_enable() for? Which btw also uses
> static_key_count() outside of the mutex.