[PATCH v2] KVM/PPC Patch for KVM issue in real mode

Some KVM functions for book3s_hv are called in real mode.
In real mode the top 4 bits of the address space are ignored,
hence an address beginning with 0xc0000000+offset is the
same as 0xd0000000+offset. The issue was observed when
a kvm memslot resolution lead to random values when
access from kvmppc_h_enter(). The issue is hit if the
KVM host is running with a page size of 4K, since
kvzalloc() looks at size < PAGE_SIZE. On systems with
64K the issue is not observed easily, it largely depends
on the size of the structure being allocated.

The proposed fix moves all KVM allocations for book3s_hv
to kzalloc() until all structures used in real mode are
audited. For safety allocations are moved to kmalloc
space. The impact is a large allocation on systems with
4K page size.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 Changelog v2:
   Fix build failures reported by the kbuild test robot
   http://www.spinics.net/lists/kvm/msg141727.html

 arch/powerpc/include/asm/kvm_host.h | 19 +++++++++++++++++++
 include/linux/kvm_host.h            | 11 +++++++++++
 virt/kvm/kvm_main.c                 |  2 +-
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index f15713a..53f5172 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h

@@ -734,6 +734,25 @@ struct kvm_vcpu_arch {
 #define __KVM_HAVE_ARCH_WQP
 #define __KVM_HAVE_CREATE_DEVICE
 
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+#define __KVM_HAVE_ARCH_VZALLOC_OVERRIDE
+
+/*
+ * KVM uses some of these data structures -- the ones
+ * from kvzalloc() in real mode. If the data structure
+ * happens to come from a vmalloc'd range then its access
+ * in real mode will lead to problems due to the aliasing
+ * issue - (top 4 bits are ignore).
+ * A 0xd000+offset will point to a 0xc000+offset in realmode
+ * Hence we want our data structures from come from kmalloc'd
+ * regions, so that we don't have these aliasing issues
+ */
+static inline void *kvm_arch_vzalloc(unsigned long size)
+{
+	return kzalloc(size, GFP_KERNEL);
+}
+#endif
+
 static inline void kvm_arch_hardware_disable(void) {}
 static inline void kvm_arch_hardware_unsetup(void) {}
 static inline void kvm_arch_sync_events(struct kvm *kvm) {}

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 01c0b9c..0c88af5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h

@@ -19,6 +19,7 @@
 #include <linux/preempt.h>
 #include <linux/msi.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 #include <linux/rcupdate.h>
 #include <linux/ratelimit.h>
 #include <linux/err.h>

@@ -793,6 +794,16 @@ static inline bool kvm_arch_has_noncoherent_dma(struct kvm *kvm)
 	return false;
 }
 #endif
+
+#ifdef __KVM_HAVE_ARCH_VZALLOC_OVERRIDE
+static void *kvm_arch_vzalloc(unsigned long size);
+#else
+static inline void *kvm_arch_vzalloc(unsigned long size)
+{
+	return vzalloc(size);
+}
+#endif
+
 #ifdef __KVM_HAVE_ARCH_ASSIGNED_DEVICE
 void kvm_arch_start_assignment(struct kvm *kvm);
 void kvm_arch_end_assignment(struct kvm *kvm);

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index fbf04c0..57e3dca 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c

@@ -689,7 +689,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 void *kvm_kvzalloc(unsigned long size)
 {
 	if (size > PAGE_SIZE)
-		return vzalloc(size);
+		return kvm_arch_vzalloc(size);
 	else
 		return kzalloc(size, GFP_KERNEL);
 }

-- 
2.5.5

Balbir Singh [off-list ref] writes:

Some KVM functions for book3s_hv are called in real mode.
In real mode the top 4 bits of the address space are ignored,
hence an address beginning with 0xc0000000+offset is the
same as 0xd0000000+offset. The issue was observed when
a kvm memslot resolution lead to random values when
access from kvmppc_h_enter(). The issue is hit if the
KVM host is running with a page size of 4K, since
kvzalloc() looks at size < PAGE_SIZE. On systems with
64K the issue is not observed easily, it largely depends
on the size of the structure being allocated.

The proposed fix moves all KVM allocations for book3s_hv
to kzalloc() until all structures used in real mode are
audited. For safety allocations are moved to kmalloc
space. The impact is a large allocation on systems with
4K page size.

We did such access using *real_vmalloc_addr(void *x). So you are
suggesting here is we don't do that for all code path ?

Do you have a stack dump for which you identified the issue ?

do we need that OVERRIDE ? We usually have HAVE_ARCH_KVM_VZALLOC
or just say #ifndef kvm_arch_vzalloc ?

+
+/*
+ * KVM uses some of these data structures -- the ones
+ * from kvzalloc() in real mode. If the data structure
+ * happens to come from a vmalloc'd range then its access
+ * in real mode will lead to problems due to the aliasing
+ * issue - (top 4 bits are ignore).
+ * A 0xd000+offset will point to a 0xc000+offset in realmode
+ * Hence we want our data structures from come from kmalloc'd
+ * regions, so that we don't have these aliasing issues
+ */
+static inline void *kvm_arch_vzalloc(unsigned long size)
+{
+	return kzalloc(size, GFP_KERNEL);
+}
+#endif

....
-aneesh

On 30/11/16 19:35, Aneesh Kumar K.V wrote:

Balbir Singh [off-list ref] writes:

quoted

Some KVM functions for book3s_hv are called in real mode.
In real mode the top 4 bits of the address space are ignored,
hence an address beginning with 0xc0000000+offset is the
same as 0xd0000000+offset. The issue was observed when
a kvm memslot resolution lead to random values when
access from kvmppc_h_enter(). The issue is hit if the
KVM host is running with a page size of 4K, since
kvzalloc() looks at size < PAGE_SIZE. On systems with
64K the issue is not observed easily, it largely depends
on the size of the structure being allocated.

The proposed fix moves all KVM allocations for book3s_hv
to kzalloc() until all structures used in real mode are
audited. For safety allocations are moved to kmalloc
space. The impact is a large allocation on systems with
4K page size.

We did such access using *real_vmalloc_addr(void *x). So you are
suggesting here is we don't do that for all code path ?

Yep.. that is true

Do you have a stack dump for which you identified the issue ?

I found it with kvm_memslots, don't have a stack dump, but
IIRC, I saw it with search_memslots <-- __gfn_to_memslot()

quoted

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 Changelog v2:
   Fix build failures reported by the kbuild test robot
   http://www.spinics.net/lists/kvm/msg141727.html

 arch/powerpc/include/asm/kvm_host.h | 19 +++++++++++++++++++
 include/linux/kvm_host.h            | 11 +++++++++++
 virt/kvm/kvm_main.c                 |  2 +-
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index f15713a..53f5172 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h

@@ -734,6 +734,25 @@ struct kvm_vcpu_arch {
 #define __KVM_HAVE_ARCH_WQP
 #define __KVM_HAVE_CREATE_DEVICE
 
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+#define __KVM_HAVE_ARCH_VZALLOC_OVERRIDE

do we need that OVERRIDE ? We usually have HAVE_ARCH_KVM_VZALLOC
or just say #ifndef kvm_arch_vzalloc ?

I can move __KVM_HAVE_ARCH_VZALLOC_OVERRIDE to
HAVE_ARCH_KVM_VZALLOC_OVERRIDE if it helps with clarity
and convention

Thanks for the review,
Balbir