Patch "locking/static_key: Fix concurrent static_key_slow_inc()" has been added to the 4.6-stable tree
Subsystems (2)
This is a note to let you know that I've just added the patch titled
locking/static_key: Fix concurrent static_key_slow_inc()
to the 4.6-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
locking-static_key-fix-concurrent-static_key_slow_inc.patch
and it can be found in the queue-4.6 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
Quoted 1 line
From
4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ffMon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Tue, 21 Jun 2016 18:52:17 +0200
Subject: locking/static_key: Fix concurrent static_key_slow_inc()
From: Paolo Bonzini <[email protected]>
commit 4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff upstream.
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.
--- include/linux/jump_label.h | 16 +++++++++++++--- kernel/jump_label.c | 36 +++++++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 6 deletions(-)
include/linux/jump_label.h +13 -3
@@ -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#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;
kernel/jump_label.c +33 -3
@@ -58,13 +58,36 @@ static void jump_label_update(struct stavoid 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" );
Patches currently in stable-queue which might be from [email protected] are
queue-4.6/locking-static_key-fix-concurrent-static_key_slow_inc.patch