| Message ID | 20170425190306.20392-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks for putting this together! On Tue, Apr 25, 2017 at 12:03 PM, David Hildenbrand <david@redhat.com> wrote: > We needed the lock to avoid racing with creation of the irqchip on x86. As > kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock > might be held for a longer time. > > Let's introduce an arch specific callback to check if we can actually > add irq routes. For x86, all we have to do is check if we have an > irqchip in the kernel. We don't need kvm->lock at that point as the > irqchip is marked as inititalized only when actually fully created. > > Reported-by: Steve Rutherford <srutherford@google.com> > Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP") > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 1 - > arch/x86/kvm/irq.h | 2 +- > arch/x86/kvm/irq_comm.c | 15 +++++++++------ > arch/x86/kvm/x86.c | 11 +---------- > include/linux/kvm_host.h | 5 +++++ > virt/kvm/kvm_main.c | 5 ++--- > 6 files changed, 18 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 2cc5ec7..d962fa9 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -726,7 +726,6 @@ struct kvm_hv { > > enum kvm_irqchip_mode { > KVM_IRQCHIP_NONE, > - KVM_IRQCHIP_INIT_IN_PROGRESS, /* temporarily set during creation */ > KVM_IRQCHIP_KERNEL, /* created with KVM_CREATE_IRQCHIP */ > KVM_IRQCHIP_SPLIT, /* created with KVM_CAP_SPLIT_IRQCHIP */ > }; > diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h > index 0edd22c..d5005cc 100644 > --- a/arch/x86/kvm/irq.h > +++ b/arch/x86/kvm/irq.h > @@ -111,7 +111,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm) > > /* Matches smp_wmb() when setting irqchip_mode */ > smp_rmb(); > - return mode > KVM_IRQCHIP_INIT_IN_PROGRESS; > + return mode != KVM_IRQCHIP_NONE; > } > > void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c > index 4517a4c..3cc3b2d 100644 > --- a/arch/x86/kvm/irq_comm.c > +++ b/arch/x86/kvm/irq_comm.c > @@ -274,16 +274,19 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin, > srcu_read_unlock(&kvm->irq_srcu, idx); > } > > +bool kvm_arch_can_set_irq_routing(struct kvm *kvm) > +{ > + return irqchip_in_kernel(kvm); > +} > + > int kvm_set_routing_entry(struct kvm *kvm, > struct kvm_kernel_irq_routing_entry *e, > const struct kvm_irq_routing_entry *ue) > { > - /* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */ > - if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE) > - return -EINVAL; > - > - /* Matches smp_wmb() when setting irqchip_mode */ > - smp_rmb(); > + /* We can't check irqchip_in_kernel() here as some callers are > + * currently inititalizing the irqchip. Other callers should therefore > + * check kvm_arch_can_set_irq_routing() before calling this function. > + */ > switch (ue->type) { > case KVM_IRQ_ROUTING_IRQCHIP: > if (irqchip_split(kvm)) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 34bf64f..eae0e9c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3911,14 +3911,9 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > goto split_irqchip_unlock; > if (kvm->created_vcpus) > goto split_irqchip_unlock; > - kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; > r = kvm_setup_empty_irq_routing(kvm); > - if (r) { > - kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; > - /* Pairs with smp_rmb() when reading irqchip_mode */ > - smp_wmb(); > + if (r) > goto split_irqchip_unlock; > - } > /* Pairs with irqchip_in_kernel. */ > smp_wmb(); > kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT; > @@ -4004,12 +3999,8 @@ long kvm_arch_vm_ioctl(struct file *filp, > goto create_irqchip_unlock; > } > > - kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; > r = kvm_setup_default_irq_routing(kvm); > if (r) { > - kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; > - /* Pairs with smp_rmb() when reading irqchip_mode */ > - smp_wmb(); > kvm_ioapic_destroy(kvm); > kvm_pic_destroy(kvm); > goto create_irqchip_unlock; > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 397b7b5..a9921b1 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu); > #ifdef __KVM_HAVE_IOAPIC > void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm); > void kvm_arch_post_irq_routing_update(struct kvm *kvm); > +bool kvm_arch_can_set_irq_routing(struct kvm *kvm); > #else > static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) > { > @@ -511,6 +512,10 @@ static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) > static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) > { > } > +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) > +{ > + return true; > +} > #endif > > #ifdef CONFIG_HAVE_KVM_IRQFD > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 357e67c..0a37ba4 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3054,6 +3054,8 @@ static long kvm_vm_ioctl(struct file *filp, > if (copy_from_user(&routing, argp, sizeof(routing))) > goto out; > r = -EINVAL; > + if (!kvm_arch_can_set_irq_routing(kvm)) > + goto out; > if (routing.nr > KVM_MAX_IRQ_ROUTES) > goto out; > if (routing.flags) > @@ -3069,11 +3071,8 @@ static long kvm_vm_ioctl(struct file *filp, > routing.nr * sizeof(*entries))) > goto out_free_irq_routing; > } > - /* avoid races with KVM_CREATE_IRQCHIP on x86 */ > - mutex_lock(&kvm->lock); > r = kvm_set_irq_routing(kvm, entries, routing.nr, > routing.flags); > - mutex_unlock(&kvm->lock); > out_free_irq_routing: > vfree(entries); > break; > -- > 2.9.3 >
2017-04-25 21:03+0200, David Hildenbrand: > We needed the lock to avoid racing with creation of the irqchip on x86. As > kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock > might be held for a longer time. > > Let's introduce an arch specific callback to check if we can actually > add irq routes. For x86, all we have to do is check if we have an > irqchip in the kernel. We don't need kvm->lock at that point as the > irqchip is marked as inititalized only when actually fully created. > > Reported-by: Steve Rutherford <srutherford@google.com> > Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP") > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 6 files changed, 18 insertions(+), 21 deletions(-) Nice! Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu); > #ifdef __KVM_HAVE_IOAPIC > void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm); > void kvm_arch_post_irq_routing_update(struct kvm *kvm); > +bool kvm_arch_can_set_irq_routing(struct kvm *kvm); (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak linking would probably be cleaner for a slow path.) > #else > static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) > { > @@ -511,6 +512,10 @@ static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) > static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) > { > } > +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) > +{ > + return true; > +} > #endif >
On 25.04.2017 21:59, Radim Krčmář wrote: > 2017-04-25 21:03+0200, David Hildenbrand: >> We needed the lock to avoid racing with creation of the irqchip on x86. As >> kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock >> might be held for a longer time. >> >> Let's introduce an arch specific callback to check if we can actually >> add irq routes. For x86, all we have to do is check if we have an >> irqchip in the kernel. We don't need kvm->lock at that point as the >> irqchip is marked as inititalized only when actually fully created. >> >> Reported-by: Steve Rutherford <srutherford@google.com> >> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP") >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> 6 files changed, 18 insertions(+), 21 deletions(-) > > Nice! > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu); >> #ifdef __KVM_HAVE_IOAPIC >> void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm); >> void kvm_arch_post_irq_routing_update(struct kvm *kvm); >> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm); > > (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak > linking would probably be cleaner for a slow path.) > >> #else >> static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) >> { >> @@ -511,6 +512,10 @@ static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) >> static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) >> { >> } >> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) >> +{ >> + return true; >> +} >> #endif >> Makes sense, shall I resend or can you fix that up when applying? Thanks!
Hi David,
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on next-20170424]
[cannot apply to v4.11-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/David-Hildenbrand/KVM-x86-don-t-hold-kvm-lock-in-KVM_SET_GSI_ROUTING/20170426-171503
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: powerpc-currituck_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
In file included from arch/powerpc/include/asm/kvm_ppc.h:30:0,
from arch/powerpc/kernel/smp.c:41:
>> include/linux/kvm_host.h:515:13: error: 'kvm_arch_can_set_irq_routing' defined but not used [-Werror=unused-function]
static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/kvm_arch_can_set_irq_routing +515 include/linux/kvm_host.h
509 static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm)
510 {
511 }
512 static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm)
513 {
514 }
> 515 static bool kvm_arch_can_set_irq_routing(struct kvm *kvm)
516 {
517 return true;
518 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
2017-04-26 20:33+0800, kbuild test robot: > Hi David, > > [auto build test ERROR on kvm/linux-next] > [also build test ERROR on next-20170424] > [cannot apply to v4.11-rc8] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/David-Hildenbrand/KVM-x86-don-t-hold-kvm-lock-in-KVM_SET_GSI_ROUTING/20170426-171503 > base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next > config: powerpc-currituck_defconfig (attached as .config) > compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 > reproduce: > wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=powerpc > > All errors (new ones prefixed by >>): > > In file included from arch/powerpc/include/asm/kvm_ppc.h:30:0, > from arch/powerpc/kernel/smp.c:41: > >> include/linux/kvm_host.h:515:13: error: 'kvm_arch_can_set_irq_routing' defined but not used [-Werror=unused-function] > static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > vim +/kvm_arch_can_set_irq_routing +515 include/linux/kvm_host.h > > 509 static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) > 510 { > 511 } > 512 static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) > 513 { > 514 } > > 515 static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) Oh, we need both static and inline to supress the warning ... > 516 { > 517 return true; > 518 } > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
2017-04-26 10:40+0200, David Hildenbrand: > On 25.04.2017 21:59, Radim Krčmář wrote: >> 2017-04-25 21:03+0200, David Hildenbrand: >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu); >>> #ifdef __KVM_HAVE_IOAPIC >>> void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm); >>> void kvm_arch_post_irq_routing_update(struct kvm *kvm); >>> +bool kvm_arch_can_set_irq_routing(struct kvm *kvm); >> >> (A nitpick: it might be useful even without __KVM_HAVE_IOAPIC so weak >> linking would probably be cleaner for a slow path.) >> >>> #else >>> static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) >>> { >>> @@ -511,6 +512,10 @@ static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) >>> static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) >>> { >>> } >>> +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) >>> +{ >>> + return true; >>> +} >>> #endif >>> > > Makes sense, shall I resend or can you fix that up when applying? Please send a v2, it's going to be automatically compile-tested on all arches that way. :)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 2cc5ec7..d962fa9 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -726,7 +726,6 @@ struct kvm_hv { enum kvm_irqchip_mode { KVM_IRQCHIP_NONE, - KVM_IRQCHIP_INIT_IN_PROGRESS, /* temporarily set during creation */ KVM_IRQCHIP_KERNEL, /* created with KVM_CREATE_IRQCHIP */ KVM_IRQCHIP_SPLIT, /* created with KVM_CAP_SPLIT_IRQCHIP */ }; diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h index 0edd22c..d5005cc 100644 --- a/arch/x86/kvm/irq.h +++ b/arch/x86/kvm/irq.h @@ -111,7 +111,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm) /* Matches smp_wmb() when setting irqchip_mode */ smp_rmb(); - return mode > KVM_IRQCHIP_INIT_IN_PROGRESS; + return mode != KVM_IRQCHIP_NONE; } void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c index 4517a4c..3cc3b2d 100644 --- a/arch/x86/kvm/irq_comm.c +++ b/arch/x86/kvm/irq_comm.c @@ -274,16 +274,19 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, unsigned irqchip, unsigned pin, srcu_read_unlock(&kvm->irq_srcu, idx); } +bool kvm_arch_can_set_irq_routing(struct kvm *kvm) +{ + return irqchip_in_kernel(kvm); +} + int kvm_set_routing_entry(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, const struct kvm_irq_routing_entry *ue) { - /* also allow creation of routes during KVM_IRQCHIP_INIT_IN_PROGRESS */ - if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE) - return -EINVAL; - - /* Matches smp_wmb() when setting irqchip_mode */ - smp_rmb(); + /* We can't check irqchip_in_kernel() here as some callers are + * currently inititalizing the irqchip. Other callers should therefore + * check kvm_arch_can_set_irq_routing() before calling this function. + */ switch (ue->type) { case KVM_IRQ_ROUTING_IRQCHIP: if (irqchip_split(kvm)) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 34bf64f..eae0e9c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3911,14 +3911,9 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, goto split_irqchip_unlock; if (kvm->created_vcpus) goto split_irqchip_unlock; - kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; r = kvm_setup_empty_irq_routing(kvm); - if (r) { - kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; - /* Pairs with smp_rmb() when reading irqchip_mode */ - smp_wmb(); + if (r) goto split_irqchip_unlock; - } /* Pairs with irqchip_in_kernel. */ smp_wmb(); kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT; @@ -4004,12 +3999,8 @@ long kvm_arch_vm_ioctl(struct file *filp, goto create_irqchip_unlock; } - kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; r = kvm_setup_default_irq_routing(kvm); if (r) { - kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; - /* Pairs with smp_rmb() when reading irqchip_mode */ - smp_wmb(); kvm_ioapic_destroy(kvm); kvm_pic_destroy(kvm); goto create_irqchip_unlock; diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 397b7b5..a9921b1 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -504,6 +504,7 @@ void vcpu_put(struct kvm_vcpu *vcpu); #ifdef __KVM_HAVE_IOAPIC void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm); void kvm_arch_post_irq_routing_update(struct kvm *kvm); +bool kvm_arch_can_set_irq_routing(struct kvm *kvm); #else static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) { @@ -511,6 +512,10 @@ static inline void kvm_arch_post_irq_ack_notifier_list_update(struct kvm *kvm) static inline void kvm_arch_post_irq_routing_update(struct kvm *kvm) { } +static bool kvm_arch_can_set_irq_routing(struct kvm *kvm) +{ + return true; +} #endif #ifdef CONFIG_HAVE_KVM_IRQFD diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 357e67c..0a37ba4 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3054,6 +3054,8 @@ static long kvm_vm_ioctl(struct file *filp, if (copy_from_user(&routing, argp, sizeof(routing))) goto out; r = -EINVAL; + if (!kvm_arch_can_set_irq_routing(kvm)) + goto out; if (routing.nr > KVM_MAX_IRQ_ROUTES) goto out; if (routing.flags) @@ -3069,11 +3071,8 @@ static long kvm_vm_ioctl(struct file *filp, routing.nr * sizeof(*entries))) goto out_free_irq_routing; } - /* avoid races with KVM_CREATE_IRQCHIP on x86 */ - mutex_lock(&kvm->lock); r = kvm_set_irq_routing(kvm, entries, routing.nr, routing.flags); - mutex_unlock(&kvm->lock); out_free_irq_routing: vfree(entries); break;
We needed the lock to avoid racing with creation of the irqchip on x86. As kvm_set_irq_routing() calls srcu_synchronize_expedited(), this lock might be held for a longer time. Let's introduce an arch specific callback to check if we can actually add irq routes. For x86, all we have to do is check if we have an irqchip in the kernel. We don't need kvm->lock at that point as the irqchip is marked as inititalized only when actually fully created. Reported-by: Steve Rutherford <srutherford@google.com> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP") Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/irq.h | 2 +- arch/x86/kvm/irq_comm.c | 15 +++++++++------ arch/x86/kvm/x86.c | 11 +---------- include/linux/kvm_host.h | 5 +++++ virt/kvm/kvm_main.c | 5 ++--- 6 files changed, 18 insertions(+), 21 deletions(-)