Re: [PATCH] static_key: fix concurrent static_key_slow_inc

[Peter Zijlstra <peterz@infradead.org>]

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).