[PATCH] kvm: Check irqchip mode before assign irqfd

[PATCH] kvm: Check irqchip mode before assign irqfd

[Peter Xu]

When assigning kvm irqfd we didn't check the irqchip mode but we allow
KVM_IRQFD to succeed with all the irqchip modes.  However it does not
make much sense to create irqfd even without the kernel chips.  Let's
provide a arch-dependent helper to check whether a specific irqfd is
allowed by the arch.  At least for x86, it should make sense to check:

- when irqchip mode is NONE, all irqfds should be disallowed, and,

- when irqchip mode is SPLIT, irqfds that are with resamplefd should
  be disallowed.

For either of the case, previously we'll silently ignore the irq or
the irq ack event if the irqchip mode is incorrect.  However that can
cause misterious guest behaviors and it can be hard to triage.  Let's
fail KVM_IRQFD even earlier to detect these incorrect configurations.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Radim Krčmář <rkrcmar@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/irq.c | 7 +++++++
 arch/x86/kvm/irq.h | 1 +
 virt/kvm/eventfd.c | 9 +++++++++
 3 files changed, 17 insertions(+)

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index faa264822cee..007bc654f928 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -172,3 +172,10 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
 	__kvm_migrate_apic_timer(vcpu);
 	__kvm_migrate_pit_timer(vcpu);
 }
+
+bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
+{
+	bool resample = args->flags & KVM_IRQFD_FLAG_RESAMPLE;
+
+	return resample ? irqchip_kernel(kvm) : irqchip_in_kernel(kvm);
+}
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index d5005cc26521..fd210cdd4983 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -114,6 +114,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return mode != KVM_IRQCHIP_NONE;
 }
 
+bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args);
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 001aeda4c154..3972a9564c76 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -44,6 +44,12 @@
 
 static struct workqueue_struct *irqfd_cleanup_wq;
 
+bool __attribute__((weak))
+kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
+{
+	return true;
+}
+
 static void
 irqfd_inject(struct work_struct *work)
 {
@@ -297,6 +303,9 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 	if (!kvm_arch_intc_initialized(kvm))
 		return -EAGAIN;
 
+	if (!kvm_arch_irqfd_allowed(kvm, args))
+		return -EINVAL;
+
 	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL_ACCOUNT);
 	if (!irqfd)
 		return -ENOMEM;
-- 
2.17.1

Re: [PATCH] kvm: Check irqchip mode before assign irqfd

[Peter Xu]

On Sun, May 05, 2019 at 04:56:42PM +0800, Peter Xu wrote:
> When assigning kvm irqfd we didn't check the irqchip mode but we allow
> KVM_IRQFD to succeed with all the irqchip modes.  However it does not
> make much sense to create irqfd even without the kernel chips.  Let's
> provide a arch-dependent helper to check whether a specific irqfd is
> allowed by the arch.  At least for x86, it should make sense to check:
> 
> - when irqchip mode is NONE, all irqfds should be disallowed, and,
> 
> - when irqchip mode is SPLIT, irqfds that are with resamplefd should
>   be disallowed.
> 
> For either of the case, previously we'll silently ignore the irq or
> the irq ack event if the irqchip mode is incorrect.  However that can
> cause misterious guest behaviors and it can be hard to triage.  Let's
> fail KVM_IRQFD even earlier to detect these incorrect configurations.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Radim Krčmář <rkrcmar@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Note: haven't tested, but IIUC QEMU's vfio will naturally fall back to
no-irqfd mode (actually virtio seems to also have this but virtio
should not be affected after all) if the KVM_IRQFD ioctl failed so I
feel like this patch could also at least fix the broken guests
reported besides any future fixes from QEMU side on the issue:

https://bugs.launchpad.net/qemu/+bug/1826422

Thanks,

-- 
Peter Xu

Re: [PATCH] kvm: Check irqchip mode before assign irqfd

[Peter Xu]

On Sun, May 05, 2019 at 05:20:22PM +0800, Peter Xu wrote:
> On Sun, May 05, 2019 at 04:56:42PM +0800, Peter Xu wrote:
> > When assigning kvm irqfd we didn't check the irqchip mode but we allow
> > KVM_IRQFD to succeed with all the irqchip modes.  However it does not
> > make much sense to create irqfd even without the kernel chips.  Let's
> > provide a arch-dependent helper to check whether a specific irqfd is
> > allowed by the arch.  At least for x86, it should make sense to check:
> > 
> > - when irqchip mode is NONE, all irqfds should be disallowed, and,
> > 
> > - when irqchip mode is SPLIT, irqfds that are with resamplefd should
> >   be disallowed.
> > 
> > For either of the case, previously we'll silently ignore the irq or
> > the irq ack event if the irqchip mode is incorrect.  However that can
> > cause misterious guest behaviors and it can be hard to triage.  Let's
> > fail KVM_IRQFD even earlier to detect these incorrect configurations.
> > 
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Radim Krčmář <rkrcmar@redhat.com>
> > CC: Alex Williamson <alex.williamson@redhat.com>
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Note: haven't tested, but IIUC QEMU's vfio will naturally fall back to
> no-irqfd mode (actually virtio seems to also have this but virtio
> should not be affected after all) if the KVM_IRQFD ioctl failed so I
> feel like this patch could also at least fix the broken guests
> reported besides any future fixes from QEMU side on the issue:
> 
> https://bugs.launchpad.net/qemu/+bug/1826422

Ping - Paolo, should patch still worth to have before the complete fix
between split irqchip & resamplefds?

Thanks,

-- 
Peter Xu

Re: [PATCH] kvm: Check irqchip mode before assign irqfd

[Paolo Bonzini]

On 05/05/19 10:56, Peter Xu wrote:
> When assigning kvm irqfd we didn't check the irqchip mode but we allow
> KVM_IRQFD to succeed with all the irqchip modes.  However it does not
> make much sense to create irqfd even without the kernel chips.  Let's
> provide a arch-dependent helper to check whether a specific irqfd is
> allowed by the arch.  At least for x86, it should make sense to check:
> 
> - when irqchip mode is NONE, all irqfds should be disallowed, and,
> 
> - when irqchip mode is SPLIT, irqfds that are with resamplefd should
>   be disallowed.
> 
> For either of the case, previously we'll silently ignore the irq or
> the irq ack event if the irqchip mode is incorrect.  However that can
> cause misterious guest behaviors and it can be hard to triage.  Let's
> fail KVM_IRQFD even earlier to detect these incorrect configurations.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Radim Krčmář <rkrcmar@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/irq.c | 7 +++++++
>  arch/x86/kvm/irq.h | 1 +
>  virt/kvm/eventfd.c | 9 +++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index faa264822cee..007bc654f928 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -172,3 +172,10 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
>  	__kvm_migrate_apic_timer(vcpu);
>  	__kvm_migrate_pit_timer(vcpu);
>  }
> +
> +bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
> +{
> +	bool resample = args->flags & KVM_IRQFD_FLAG_RESAMPLE;
> +
> +	return resample ? irqchip_kernel(kvm) : irqchip_in_kernel(kvm);
> +}
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index d5005cc26521..fd210cdd4983 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -114,6 +114,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return mode != KVM_IRQCHIP_NONE;
>  }
>  
> +bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args);
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
>  void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 001aeda4c154..3972a9564c76 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -44,6 +44,12 @@
>  
>  static struct workqueue_struct *irqfd_cleanup_wq;
>  
> +bool __attribute__((weak))
> +kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
> +{
> +	return true;
> +}
> +
>  static void
>  irqfd_inject(struct work_struct *work)
>  {
> @@ -297,6 +303,9 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
>  	if (!kvm_arch_intc_initialized(kvm))
>  		return -EAGAIN;
>  
> +	if (!kvm_arch_irqfd_allowed(kvm, args))
> +		return -EINVAL;
> +
>  	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL_ACCOUNT);
>  	if (!irqfd)
>  		return -ENOMEM;
> 

Queued, thanks.

Paolo