[PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Roman Kagan]

This patchset contains fixes for two hyperv implementation flaws in KVM
that surfaced when the userspace (QEMU) started to use it more actively.

In both cases more control is delegated from KVM to userspace.

v2 -> v3:
 - add documentation
 - add handling of args[0] to KVM_CAP_HYPERV_SYNIC2

v1 -> v2:
 - add patch 1
 - add capability in patch 2

Roman Kagan (2):
  kvm: x86: hyperv: add KVM_CAP_HYPERV_SYNIC2
  kvm: x86: hyperv: make VP_INDEX managed by userspace

 Documentation/virtual/kvm/api.txt | 18 ++++++++++++
 arch/x86/include/asm/kvm_host.h   |  2 ++
 arch/x86/kvm/hyperv.h             |  2 +-
 include/uapi/linux/kvm.h          |  2 ++
 arch/x86/kvm/hyperv.c             | 62 ++++++++++++++++++++++++---------------
 arch/x86/kvm/x86.c                |  8 ++++-
 6 files changed, 69 insertions(+), 25 deletions(-)

-- 
2.9.4

[PATCH v3 1/2] kvm: x86: hyperv: add KVM_CAP_HYPERV_SYNIC2

[Roman Kagan]

There is a flaw in the Hyper-V SynIC implementation in KVM: when message
page or event flags page is enabled by setting the corresponding msr,
KVM zeroes it out.  This is problematic because on migration the
corresponding MSRs are loaded on the destination, so the content of
those pages is lost.

This went unnoticed so far because the only user of those pages was
in-KVM hyperv synic timers, which could continue working despite that
zeroing.

Newer QEMU uses those pages for Hyper-V VMBus implementation, and
zeroing them breaks the migration.

Besides, in newer QEMU the content of those pages is fully managed by
QEMU, so zeroing them is undesirable even when writing the MSRs from the
guest side.

To support this new scheme, introduce a new capability,
KVM_CAP_HYPERV_SYNIC2, which, when enabled, makes sure that the synic
pages aren't zeroed out in KVM.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v2 -> v3:
 - add documentation
 - add handling of args[0] to KVM_CAP_HYPERV_SYNIC2

 Documentation/virtual/kvm/api.txt |  9 +++++++++
 arch/x86/include/asm/kvm_host.h   |  1 +
 arch/x86/kvm/hyperv.h             |  2 +-
 include/uapi/linux/kvm.h          |  1 +
 arch/x86/kvm/hyperv.c             | 13 +++++++++----
 arch/x86/kvm/x86.c                |  7 ++++++-
 6 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 4029943..b9bd667 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4157,3 +4157,12 @@ Currently the following bits are defined for the device_irq_level bitmap:
 Future versions of kvm may implement additional events. These will get
 indicated by returning a higher number from KVM_CHECK_EXTENSION and will be
 listed above.
+
+8.10 KVM_CAP_HYPERV_SYNIC2
+
+Architectures: x86
+
+This capability enables a newer version of Hyper-V Synthetic interrupt
+controller (SynIC).  The only difference with KVM_CAP_HYPERV_SYNIC is that KVM
+doesn't clear SynIC message and event flags pages when they are enabled by
+writing to the  respective MSRs.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 695605e..10e70e7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -458,6 +458,7 @@ struct kvm_vcpu_hv_synic {
 	DECLARE_BITMAP(auto_eoi_bitmap, 256);
 	DECLARE_BITMAP(vec_bitmap, 256);
 	bool active;
+	bool dont_zero_synic_pages;
 };
 
 /* Hyper-V per vcpu emulation context */
diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index cd11195..12f65fe 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -56,7 +56,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu);
 void kvm_hv_irq_routing_update(struct kvm *kvm);
 int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint);
 void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector);
-int kvm_hv_activate_synic(struct kvm_vcpu *vcpu);
+int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
 
 void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_hv_vcpu_uninit(struct kvm_vcpu *vcpu);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 577429a..193fcf9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -895,6 +895,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_SPAPR_TCE_VFIO 142
 #define KVM_CAP_X86_GUEST_MWAIT 143
 #define KVM_CAP_ARM_USER_IRQ 144
+#define KVM_CAP_HYPERV_SYNIC2 145
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ebae57a..a808440 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -221,7 +221,8 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 		synic->version = data;
 		break;
 	case HV_X64_MSR_SIEFP:
-		if (data & HV_SYNIC_SIEFP_ENABLE)
+		if ((data & HV_SYNIC_SIEFP_ENABLE) && !host &&
+		    !synic->dont_zero_synic_pages)
 			if (kvm_clear_guest(vcpu->kvm,
 					    data & PAGE_MASK, PAGE_SIZE)) {
 				ret = 1;
@@ -232,7 +233,8 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
 			synic_exit(synic, msr);
 		break;
 	case HV_X64_MSR_SIMP:
-		if (data & HV_SYNIC_SIMP_ENABLE)
+		if ((data & HV_SYNIC_SIMP_ENABLE) && !host &&
+		    !synic->dont_zero_synic_pages)
 			if (kvm_clear_guest(vcpu->kvm,
 					    data & PAGE_MASK, PAGE_SIZE)) {
 				ret = 1;
@@ -687,14 +689,17 @@ void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu)
 		stimer_init(&hv_vcpu->stimer[i], i);
 }
 
-int kvm_hv_activate_synic(struct kvm_vcpu *vcpu)
+int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
 {
+	struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
+
 	/*
 	 * Hyper-V SynIC auto EOI SINT's are
 	 * not compatible with APICV, so deactivate APICV
 	 */
 	kvm_vcpu_deactivate_apicv(vcpu);
-	vcpu_to_synic(vcpu)->active = true;
+	synic->active = true;
+	synic->dont_zero_synic_pages = dont_zero_synic_pages;
 	return 0;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 87d3cb9..ad5b920 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2661,6 +2661,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_HYPERV_VAPIC:
 	case KVM_CAP_HYPERV_SPIN:
 	case KVM_CAP_HYPERV_SYNIC:
+	case KVM_CAP_HYPERV_SYNIC2:
 	case KVM_CAP_PCI_SEGMENT:
 	case KVM_CAP_DEBUGREGS:
 	case KVM_CAP_X86_ROBUST_SINGLESTEP:
@@ -3384,10 +3385,14 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	switch (cap->cap) {
+	case KVM_CAP_HYPERV_SYNIC2:
+		if (cap->args[0])
+			return -EINVAL;
 	case KVM_CAP_HYPERV_SYNIC:
 		if (!irqchip_in_kernel(vcpu->kvm))
 			return -EINVAL;
-		return kvm_hv_activate_synic(vcpu);
+		return kvm_hv_activate_synic(vcpu, cap->cap ==
+					     KVM_CAP_HYPERV_SYNIC2);
 	default:
 		return -EINVAL;
 	}
-- 
2.9.4

[PATCH v3 2/2] kvm: x86: hyperv: make VP_INDEX managed by userspace

[Roman Kagan]

Hyper-V identifies vCPUs by Virtual Processor Index, which can be
queried via HV_X64_MSR_VP_INDEX msr.  It is defined by the spec as a
sequential number which can't exceed the maximum number of vCPUs per VM.
APIC ids can be sparse and thus aren't a valid replacement for VP
indices.

Current KVM uses its internal vcpu index as VP_INDEX.  However, to make
it predictable and persistent across VM migrations, the userspace has to
control the value of VP_INDEX.

This patch achieves that, by storing vp_index explicitly on vcpu, and
allowing HV_X64_MSR_VP_INDEX to be set from the host side.  For
compatibility it's initialized to KVM vcpu index.  Also a few variables
are renamed to make clear distinction betweed this Hyper-V vp_index and
KVM vcpu_id (== APIC id).  Besides, a new capability,
KVM_CAP_HYPERV_VP_INDEX, is added to allow the userspace to skip
attempting msr writes where unsupported, to avoid spamming error logs.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v2 -> v3:
 - add documentation

 Documentation/virtual/kvm/api.txt |  9 +++++++
 arch/x86/include/asm/kvm_host.h   |  1 +
 include/uapi/linux/kvm.h          |  1 +
 arch/x86/kvm/hyperv.c             | 49 ++++++++++++++++++++++++---------------
 arch/x86/kvm/x86.c                |  1 +
 5 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index b9bd667..ce23207 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4166,3 +4166,12 @@ This capability enables a newer version of Hyper-V Synthetic interrupt
 controller (SynIC).  The only difference with KVM_CAP_HYPERV_SYNIC is that KVM
 doesn't clear SynIC message and event flags pages when they are enabled by
 writing to the  respective MSRs.
+
+8.11 KVM_CAP_HYPERV_VP_INDEX
+
+Architectures: x86
+
+This capability indicates that userspace can load HV_X64_MSR_VP_INDEX msr.  Its
+value is used to denote the target vcpu for a SynIC interrupt.  For
+compatibilty, KVM initializes this msr to KVM's internal vcpu index.  When this
+capability is absent, userspace can still query this msr's value.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 10e70e7..3e654e4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -463,6 +463,7 @@ struct kvm_vcpu_hv_synic {
 
 /* Hyper-V per vcpu emulation context */
 struct kvm_vcpu_hv {
+	u32 vp_index;
 	u64 hv_vapic;
 	s64 runtime_offset;
 	struct kvm_vcpu_hv_synic synic;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 193fcf9..93a1385 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -896,6 +896,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_X86_GUEST_MWAIT 143
 #define KVM_CAP_ARM_USER_IRQ 144
 #define KVM_CAP_HYPERV_SYNIC2 145
+#define KVM_CAP_HYPERV_VP_INDEX 146
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index a808440..8671632 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -106,14 +106,27 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 	return 0;
 }
 
-static struct kvm_vcpu_hv_synic *synic_get(struct kvm *kvm, u32 vcpu_id)
+static struct kvm_vcpu *get_vcpu_by_vpidx(struct kvm *kvm, u32 vpidx)
+{
+	struct kvm_vcpu *vcpu = NULL;
+	int i;
+
+	if (vpidx < KVM_MAX_VCPUS)
+		vcpu = kvm_get_vcpu(kvm, vpidx);
+	if (vcpu && vcpu_to_hv_vcpu(vcpu)->vp_index == vpidx)
+		return vcpu;
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		if (vcpu_to_hv_vcpu(vcpu)->vp_index == vpidx)
+			return vcpu;
+	return NULL;
+}
+
+static struct kvm_vcpu_hv_synic *synic_get(struct kvm *kvm, u32 vpidx)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vcpu_hv_synic *synic;
 
-	if (vcpu_id >= atomic_read(&kvm->online_vcpus))
-		return NULL;
-	vcpu = kvm_get_vcpu(kvm, vcpu_id);
+	vcpu = get_vcpu_by_vpidx(kvm, vpidx);
 	if (!vcpu)
 		return NULL;
 	synic = vcpu_to_synic(vcpu);
@@ -320,11 +333,11 @@ static int synic_set_irq(struct kvm_vcpu_hv_synic *synic, u32 sint)
 	return ret;
 }
 
-int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint)
+int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vpidx, u32 sint)
 {
 	struct kvm_vcpu_hv_synic *synic;
 
-	synic = synic_get(kvm, vcpu_id);
+	synic = synic_get(kvm, vpidx);
 	if (!synic)
 		return -EINVAL;
 
@@ -343,11 +356,11 @@ void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector)
 			kvm_hv_notify_acked_sint(vcpu, i);
 }
 
-static int kvm_hv_set_sint_gsi(struct kvm *kvm, u32 vcpu_id, u32 sint, int gsi)
+static int kvm_hv_set_sint_gsi(struct kvm *kvm, u32 vpidx, u32 sint, int gsi)
 {
 	struct kvm_vcpu_hv_synic *synic;
 
-	synic = synic_get(kvm, vcpu_id);
+	synic = synic_get(kvm, vpidx);
 	if (!synic)
 		return -EINVAL;
 
@@ -682,6 +695,8 @@ void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu)
 	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
 	int i;
 
+	hv_vcpu->vp_index = kvm_vcpu_get_idx(vcpu);
+
 	synic_init(&hv_vcpu->synic);
 
 	bitmap_zero(hv_vcpu->stimer_pending_bitmap, HV_SYNIC_STIMER_COUNT);
@@ -983,6 +998,11 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
 	struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
 
 	switch (msr) {
+	case HV_X64_MSR_VP_INDEX:
+		if (!host)
+			return 1;
+		hv->vp_index = (u32)data;
+		break;
 	case HV_X64_MSR_APIC_ASSIST_PAGE: {
 		u64 gfn;
 		unsigned long addr;
@@ -1094,18 +1114,9 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
 
 	switch (msr) {
-	case HV_X64_MSR_VP_INDEX: {
-		int r;
-		struct kvm_vcpu *v;
-
-		kvm_for_each_vcpu(r, v, vcpu->kvm) {
-			if (v == vcpu) {
-				data = r;
-				break;
-			}
-		}
+	case HV_X64_MSR_VP_INDEX:
+		data = hv->vp_index;
 		break;
-	}
 	case HV_X64_MSR_EOI:
 		return kvm_hv_vapic_msr_read(vcpu, APIC_EOI, pdata);
 	case HV_X64_MSR_ICR:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ad5b920..01781d3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2662,6 +2662,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_HYPERV_SPIN:
 	case KVM_CAP_HYPERV_SYNIC:
 	case KVM_CAP_HYPERV_SYNIC2:
+	case KVM_CAP_HYPERV_VP_INDEX:
 	case KVM_CAP_PCI_SEGMENT:
 	case KVM_CAP_DEBUGREGS:
 	case KVM_CAP_X86_ROBUST_SINGLESTEP:
-- 
2.9.4

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Paolo Bonzini]

On 22/06/2017 15:51, Roman Kagan wrote:
> This patchset contains fixes for two hyperv implementation flaws in KVM
> that surfaced when the userspace (QEMU) started to use it more actively.
> 
> In both cases more control is delegated from KVM to userspace.
> 
> v2 -> v3:
>  - add documentation
>  - add handling of args[0] to KVM_CAP_HYPERV_SYNIC2
> 
> v1 -> v2:
>  - add patch 1
>  - add capability in patch 2
> 
> Roman Kagan (2):
>   kvm: x86: hyperv: add KVM_CAP_HYPERV_SYNIC2
>   kvm: x86: hyperv: make VP_INDEX managed by userspace
> 
>  Documentation/virtual/kvm/api.txt | 18 ++++++++++++
>  arch/x86/include/asm/kvm_host.h   |  2 ++
>  arch/x86/kvm/hyperv.h             |  2 +-
>  include/uapi/linux/kvm.h          |  2 ++
>  arch/x86/kvm/hyperv.c             | 62 ++++++++++++++++++++++++---------------
>  arch/x86/kvm/x86.c                |  8 ++++-
>  6 files changed, 69 insertions(+), 25 deletions(-)
> 

Looks good, thanks.

Paolo

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Roman Kagan]

On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/06/2017 15:51, Roman Kagan wrote:
> > This patchset contains fixes for two hyperv implementation flaws in KVM
> > that surfaced when the userspace (QEMU) started to use it more actively.
> > 
> > In both cases more control is delegated from KVM to userspace.
> > 
> > v2 -> v3:
> >  - add documentation
> >  - add handling of args[0] to KVM_CAP_HYPERV_SYNIC2
> > 
> > v1 -> v2:
> >  - add patch 1
> >  - add capability in patch 2
> > 
> > Roman Kagan (2):
> >   kvm: x86: hyperv: add KVM_CAP_HYPERV_SYNIC2
> >   kvm: x86: hyperv: make VP_INDEX managed by userspace
> > 
> >  Documentation/virtual/kvm/api.txt | 18 ++++++++++++
> >  arch/x86/include/asm/kvm_host.h   |  2 ++
> >  arch/x86/kvm/hyperv.h             |  2 +-
> >  include/uapi/linux/kvm.h          |  2 ++
> >  arch/x86/kvm/hyperv.c             | 62 ++++++++++++++++++++++++---------------
> >  arch/x86/kvm/x86.c                |  8 ++++-
> >  6 files changed, 69 insertions(+), 25 deletions(-)
> > 
> 
> Looks good, thanks.

Are there still any problems with this series?
I don't see it in kvm queue, so presumably it wasn't accepted...

Thanks,
Roman.

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Radim Krčmář]

2017-07-13 18:29+0300, Roman Kagan:
> On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > On 22/06/2017 15:51, Roman Kagan wrote:
> > Looks good, thanks.
> 
> Are there still any problems with this series?
> I don't see it in kvm queue, so presumably it wasn't accepted...

No, the problem was on my side.  Queing it for the end of this merge
window.  Thanks for the ping.

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Radim Krčmář]

2017-07-13 17:45+0200, Radim Krčmář:
> 2017-07-13 18:29+0300, Roman Kagan:
> > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > Looks good, thanks.
> > 
> > Are there still any problems with this series?
> > I don't see it in kvm queue, so presumably it wasn't accepted...
> 
> No, the problem was on my side.  Queing it for the end of this merge
> window.  Thanks for the ping.

And took it out after hitting a bug:  we're asking for the VP_INDEX before the
VCPU is in kvm->vcpus[], but the index is its position in that array.
I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().

The error:

kernel BUG at ./include/linux/kvm_host.h:523!
invalid opcode: 0000 [#1] SMP
Modules linked in: kvm_amd(OE) kvm(OE) irqbypass(E) xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack libcrc32c tun bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables sunrpc snd_hda_codec_realtek snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm snd_timer snd sky2 parport_serial ppdev parport_pc joydev parport shpchp sp5100_tco soundcore i2c_piix4 wmi k10temp acpi_cpufreq amdkfd amd_iommu_v2 radeon i2c_algo_bit drm_kms_helper uas serio_raw usb_storage ttm pata_atiixp ata_generic pata_acpi drm pata_jmicron [last unloaded: irqbypass]
CPU: 0 PID: 8274 Comm: CPU 0/KVM Tainted: G           OE   4.12.0+ #1
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS 080014  03/07/2008
task: ffff8fe5a4658000 task.stack: ffffb8408211c000
RIP: 0010:kvm_hv_vcpu_init+0x1bd/0x1c0 [kvm]
RSP: 0018:ffffb8408211fcf8 EFLAGS: 00010246
RAX: ffff8fe625800000 RBX: ffff8fe622128000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000080000000 RDI: ffff8fe622128000
RBP: ffffb8408211fd18 R08: ffff8fe6a6fde960 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffffffffff
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  00007fd284c55700(0000) GS:ffff8fe6a6e00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005598739115b0 CR3: 00000002201ce000 CR4: 00000000000006f0
Call Trace:
 ? kvm_arch_vcpu_init+0x1d8/0x270 [kvm]
 kvm_vcpu_init+0xcb/0x110 [kvm]
 svm_create_vcpu+0x4a/0x390 [kvm_amd]
 kvm_arch_vcpu_create+0x3e/0x60 [kvm]
 kvm_vm_ioctl+0x1ff/0x8f0 [kvm]
 ? __lock_acquire+0x31f/0x13b0
 ? sched_clock+0x9/0x10
 ? debug_lockdep_rcu_enabled+0x1d/0x30
 do_vfs_ioctl+0xa6/0x6c0
 SyS_ioctl+0x79/0x90
 entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x7fd2934005c7
RSP: 002b:00007fd284c54808 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007fd284c55700 RCX: 00007fd2934005c7
RDX: 0000000000000000 RSI: 000000000000ae41 RDI: 000000000000000e
RBP: 00007ffe60da4210 R08: 000055f38306f5d0 R09: 000055f385dc4000
R10: 000055f38367ab20 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffe60da4230 R14: 00007fd284c559c0 R15: 0000000000000000
Code: ff 00 00 00 00 48 c7 83 b4 fe ff ff 00 00 00 00 89 83 a4 fe ff ff 41 83 ff 04 0f 85 4a ff ff ff 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 90 0f 1f 44 00 00 55 48 89 e5 41 54 53 41 89 f4 48 89 fb 
RIP: kvm_hv_vcpu_init+0x1bd/0x1c0 [kvm] RSP: ffffb8408211fcf8

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Radim Krčmář]

2017-07-13 18:38+0200, Radim Krčmář:
> 2017-07-13 17:45+0200, Radim Krčmář:
> > 2017-07-13 18:29+0300, Roman Kagan:
> > > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > > Looks good, thanks.
> > > 
> > > Are there still any problems with this series?
> > > I don't see it in kvm queue, so presumably it wasn't accepted...
> > 
> > No, the problem was on my side.  Queing it for the end of this merge
> > window.  Thanks for the ping.
> 
> And took it out after hitting a bug:  we're asking for the VP_INDEX before the
> VCPU is in kvm->vcpus[], but the index is its position in that array.
> I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().

Ugh, definitely no, we're not under kvm->lock there.
Assigning the VP_INDEX in kvm_arch_vcpu_postcreate() would be doable,
but adding a new callback before exposing the VCPU fd is probably safer.

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Roman Kagan]

On Thu, Jul 13, 2017 at 06:38:29PM +0200, Radim Krčmář wrote:
> 2017-07-13 17:45+0200, Radim Krčmář:
> > 2017-07-13 18:29+0300, Roman Kagan:
> > > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > > Looks good, thanks.
> > > 
> > > Are there still any problems with this series?
> > > I don't see it in kvm queue, so presumably it wasn't accepted...
> > 
> > No, the problem was on my side.  Queing it for the end of this merge
> > window.  Thanks for the ping.
> 
> And took it out after hitting a bug:  we're asking for the VP_INDEX before the
> VCPU is in kvm->vcpus[], but the index is its position in that array.
> I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().
> 
> The error:
> 
> kernel BUG at ./include/linux/kvm_host.h:523!
> invalid opcode: 0000 [#1] SMP
> Modules linked in: kvm_amd(OE) kvm(OE) irqbypass(E) xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack libcrc32c tun bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables sunrpc snd_hda_codec_realtek snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm snd_timer snd sky2 parport_serial ppdev parport_pc joydev parport shpchp sp5100_tco soundcore i2c_piix4 wmi k10temp acpi_cpufreq amdkfd amd_iommu_v2 radeon i2c_algo_bit drm_kms_helper uas serio_raw usb_storage ttm pata_atiixp ata_generic pata_acpi drm pata_jmicron [last unloaded: irqbypass]
> CPU: 0 PID: 8274 Comm: CPU 0/KVM Tainted: G           OE   4.12.0+ #1
> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS 080014  03/07/2008
> task: ffff8fe5a4658000 task.stack: ffffb8408211c000
> RIP: 0010:kvm_hv_vcpu_init+0x1bd/0x1c0 [kvm]
> RSP: 0018:ffffb8408211fcf8 EFLAGS: 00010246
> RAX: ffff8fe625800000 RBX: ffff8fe622128000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 0000000080000000 RDI: ffff8fe622128000
> RBP: ffffb8408211fd18 R08: ffff8fe6a6fde960 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffffffffff
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> FS:  00007fd284c55700(0000) GS:ffff8fe6a6e00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00005598739115b0 CR3: 00000002201ce000 CR4: 00000000000006f0
> Call Trace:
>  ? kvm_arch_vcpu_init+0x1d8/0x270 [kvm]
>  kvm_vcpu_init+0xcb/0x110 [kvm]
>  svm_create_vcpu+0x4a/0x390 [kvm_amd]
>  kvm_arch_vcpu_create+0x3e/0x60 [kvm]
>  kvm_vm_ioctl+0x1ff/0x8f0 [kvm]
>  ? __lock_acquire+0x31f/0x13b0
>  ? sched_clock+0x9/0x10
>  ? debug_lockdep_rcu_enabled+0x1d/0x30
>  do_vfs_ioctl+0xa6/0x6c0
>  SyS_ioctl+0x79/0x90
>  entry_SYSCALL_64_fastpath+0x1f/0xbe
> RIP: 0033:0x7fd2934005c7
> RSP: 002b:00007fd284c54808 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007fd284c55700 RCX: 00007fd2934005c7
> RDX: 0000000000000000 RSI: 000000000000ae41 RDI: 000000000000000e
> RBP: 00007ffe60da4210 R08: 000055f38306f5d0 R09: 000055f385dc4000
> R10: 000055f38367ab20 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007ffe60da4230 R14: 00007fd284c559c0 R15: 0000000000000000
> Code: ff 00 00 00 00 48 c7 83 b4 fe ff ff 00 00 00 00 89 83 a4 fe ff ff 41 83 ff 04 0f 85 4a ff ff ff 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 90 0f 1f 44 00 00 55 48 89 e5 41 54 53 41 89 f4 48 89 fb 
> RIP: kvm_hv_vcpu_init+0x1bd/0x1c0 [kvm] RSP: ffffb8408211fcf8

Hmm, I wonder how I've never hit this...

Thanks for catching,
Roman.

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Roman Kagan]

On Thu, Jul 13, 2017 at 06:41:29PM +0200, Radim Krčmář wrote:
> 2017-07-13 18:38+0200, Radim Krčmář:
> > 2017-07-13 17:45+0200, Radim Krčmář:
> > > 2017-07-13 18:29+0300, Roman Kagan:
> > > > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > > > Looks good, thanks.
> > > > 
> > > > Are there still any problems with this series?
> > > > I don't see it in kvm queue, so presumably it wasn't accepted...
> > > 
> > > No, the problem was on my side.  Queing it for the end of this merge
> > > window.  Thanks for the ping.
> > 
> > And took it out after hitting a bug:  we're asking for the VP_INDEX before the
> > VCPU is in kvm->vcpus[], but the index is its position in that array.
> > I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().
> 
> Ugh, definitely no, we're not under kvm->lock there.
> Assigning the VP_INDEX in kvm_arch_vcpu_postcreate() would be doable,
> but adding a new callback before exposing the VCPU fd is probably safer.

But kvm_arch_vcpu_postcreate() *is* the last thing that's called before
returning the VCPU fd.  Isn't it the right place to do it?  Or do you
mean a dedicated kvm_hv_vcpu_postcreate() call rather than open-coding
it there?

Roman,
fixing his scripts not to ignore errors from insmod when smoke-testing
kernel modules (facepalm)...

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Radim Krčmář]

2017-07-13 21:15+0300, Roman Kagan:
> On Thu, Jul 13, 2017 at 06:41:29PM +0200, Radim Krčmář wrote:
> > 2017-07-13 18:38+0200, Radim Krčmář:
> > > 2017-07-13 17:45+0200, Radim Krčmář:
> > > > 2017-07-13 18:29+0300, Roman Kagan:
> > > > > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > > > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > > > > Looks good, thanks.
> > > > > 
> > > > > Are there still any problems with this series?
> > > > > I don't see it in kvm queue, so presumably it wasn't accepted...
> > > > 
> > > > No, the problem was on my side.  Queing it for the end of this merge
> > > > window.  Thanks for the ping.
> > > 
> > > And took it out after hitting a bug:  we're asking for the VP_INDEX before the
> > > VCPU is in kvm->vcpus[], but the index is its position in that array.
> > > I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().
> > 
> > Ugh, definitely no, we're not under kvm->lock there.
> > Assigning the VP_INDEX in kvm_arch_vcpu_postcreate() would be doable,
> > but adding a new callback before exposing the VCPU fd is probably safer.
> 
> But kvm_arch_vcpu_postcreate() *is* the last thing that's called before
> returning the VCPU fd.  Isn't it the right place to do it?  Or do you
> mean a dedicated kvm_hv_vcpu_postcreate() call rather than open-coding
> it there?

It is, I just didn't want to guarantee that nothing bad can happen

 1) after create_vcpu_fd() -- another thread can already access the fd
    (getting its number just takes some guess-work)

 2) after atomic_inc(&kvm->online_vcpus) -- the VCPU's vp_index is still
    invalid 0, but someone (get_vcpu_by_vpidx()) might look at it

I'd prefer to reuse kvm_arch_vcpu_postcreate() if it looks ok to you.
(Adding a new call only after all sane solutions have failed.)

> Roman,
> fixing his scripts not to ignore errors from insmod when smoke-testing
> kernel modules (facepalm)...

:) tests for tests for ...

Re: [PATCH v3 0/2] kvm: x86: hyperv: fix userspace interaction flaws

[Roman Kagan]

On Thu, Jul 13, 2017 at 08:52:38PM +0200, Radim Krčmář wrote:
> 2017-07-13 21:15+0300, Roman Kagan:
> > On Thu, Jul 13, 2017 at 06:41:29PM +0200, Radim Krčmář wrote:
> > > 2017-07-13 18:38+0200, Radim Krčmář:
> > > > 2017-07-13 17:45+0200, Radim Krčmář:
> > > > > 2017-07-13 18:29+0300, Roman Kagan:
> > > > > > On Fri, Jun 23, 2017 at 12:54:25PM +0200, Paolo Bonzini wrote:
> > > > > > > On 22/06/2017 15:51, Roman Kagan wrote:
> > > > > > > Looks good, thanks.
> > > > > > 
> > > > > > Are there still any problems with this series?
> > > > > > I don't see it in kvm queue, so presumably it wasn't accepted...
> > > > > 
> > > > > No, the problem was on my side.  Queing it for the end of this merge
> > > > > window.  Thanks for the ping.
> > > > 
> > > > And took it out after hitting a bug:  we're asking for the VP_INDEX before the
> > > > VCPU is in kvm->vcpus[], but the index is its position in that array.
> > > > I think we can just use kvm->online_vcpus instead of kvm_vcpu_get_idx().
> > > 
> > > Ugh, definitely no, we're not under kvm->lock there.
> > > Assigning the VP_INDEX in kvm_arch_vcpu_postcreate() would be doable,
> > > but adding a new callback before exposing the VCPU fd is probably safer.
> > 
> > But kvm_arch_vcpu_postcreate() *is* the last thing that's called before
> > returning the VCPU fd.  Isn't it the right place to do it?  Or do you
> > mean a dedicated kvm_hv_vcpu_postcreate() call rather than open-coding
> > it there?
> 
> It is, I just didn't want to guarantee that nothing bad can happen
> 
>  1) after create_vcpu_fd() -- another thread can already access the fd
>     (getting its number just takes some guess-work)
> 
>  2) after atomic_inc(&kvm->online_vcpus) -- the VCPU's vp_index is still
>     invalid 0, but someone (get_vcpu_by_vpidx()) might look at it

Well, this whole patch is to allow the userspace to set vp_index, under
the assumption that nothing in the kernel will use it until the
userspace has a chance to do so.  This initialization is a fallback for
the case when the userspace is too old and doesn't explicitly control
vp_index.

So the risks you mention are hypthetical only.

> I'd prefer to reuse kvm_arch_vcpu_postcreate() if it looks ok to you.
> (Adding a new call only after all sane solutions have failed.)

Great then, will adjust the patch and respin.

Thanks,
Roman.