Thanks for putting this together!
On Tue, Apr 25, 2017 at 12:03 PM, David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxx>
> Fixes: 1df6ddede10a ("KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP")
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
> 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
>
