KVM: MMU: release noslot pfn on the fail path properly

This bug was triggered:

[ 4220.198458] BUG: unable to handle kernel paging request at fffffffffffffffe
[ 4220.203907] IP: [<ffffffff81104d85>] put_page+0xf/0x34
......
[ 4220.237326] Call Trace:
[ 4220.237361]  [<ffffffffa03830d0>] kvm_arch_destroy_vm+0xf9/0x101 [kvm]
[ 4220.237382]  [<ffffffffa036fe53>] kvm_put_kvm+0xcc/0x127 [kvm]
[ 4220.237401]  [<ffffffffa03702bc>] kvm_vcpu_release+0x18/0x1c [kvm]
[ 4220.237407]  [<ffffffff81145425>] __fput+0x111/0x1ed
[ 4220.237411]  [<ffffffff8114550f>] ____fput+0xe/0x10
[ 4220.237418]  [<ffffffff81063511>] task_work_run+0x5d/0x88
[ 4220.237424]  [<ffffffff8104c3f7>] do_exit+0x2bf/0x7ca

The test case:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <fcntl.h>
#include <unistd.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <sys/mman.h>

#include <linux/kvm.h>

#define die(fmt, args...)	do {	\
	printf(fmt, ##args);		\
	exit(-1);} while (0)

static int create_vm(void)

{
	int sys_fd, vm_fd;

	sys_fd = open("/dev/kvm", O_RDWR);
	if (sys_fd < 0)
		die("open /dev/kvm fail.\n");

	vm_fd = ioctl(sys_fd, KVM_CREATE_VM, 0);
	if (vm_fd < 0)
		die("KVM_CREATE_VM fail.\n");

	return vm_fd;
}

static int create_vcpu(int vm_fd)

{
	int vcpu_fd;

	vcpu_fd = ioctl(vm_fd, KVM_CREATE_VCPU, 0);
	if (vcpu_fd < 0)
		die("KVM_CREATE_VCPU ioctl.\n");
	printf("Create vcpu.\n");
	return vcpu_fd;
}

static void *vcpu_thread(void *arg)

{
	int vm_fd = (int)(long)arg;

	create_vcpu(vm_fd);
	return NULL;
}

int main(int argc, char *argv[])

{
	pthread_t thread;
	int vm_fd;

	(void)argc;
	(void)argv;

	vm_fd = create_vm();
	pthread_create(&thread, NULL, vcpu_thread, (void *)(long)vm_fd);
	printf("Exit.\n");
	return 0;
}

It caused by release kvm->arch.ept_identity_map_addr which is the error page.

The parent thread can send KILL signal to the vcpu thread when it was exiting which stops faulting pages and potentially allocating memory. So gfn_to_pfn/gfn_to_page may fail at this time

Fixed by checking the page before it is used

Checking the return of kvm_mmu_get_page is unnecessary since it is guaranteed by memory cache

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte

Introduce mmu_release_pfn_clean to do this kind of thing

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu.c +0 -0

index 399c177..3c10bca 100644
@@ -2432,6 +2432,16 @@ done:
 	return ret;
 }
 

Quoted 20 lines · Xiao Guangrong

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte

Introduce mmu_release_pfn_clean to do this kind of thing

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu.c +0 -0

index 399c177..3c10bca 100644
@@ -2432,6 +2432,16 @@ done:
 	return ret;
 }
 

arch/x86/kvm/mmu.c done: · line 2444

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+/*
+ * The primary user is page fault path which call it to properly
+ * release noslot_pfn.
+ */
+static void mmu_release_pfn_clean(pfn_t pfn)
+{
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
+}
+

Too many APIs, each slightly different. How do I know which one to call?

Please change kvm_release_pfn_*() instead, calling some arch hook (or even #ifdef CONFIG_KVM_HAS_FAST_MMIO) to check for the special case.

Quoted 21 lines · Avi Kivity

Quoted 20 lines · Xiao Guangrong

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte

Introduce mmu_release_pfn_clean to do this kind of thing

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu.c +0 -0

index 399c177..3c10bca 100644
@@ -2432,6 +2432,16 @@ done:
 	return ret;
 }
 

Quoted 12 lines

arch/x86/kvm/mmu.c done: · line 2444

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+/*
+ * The primary user is page fault path which call it to properly
+ * release noslot_pfn.
+ */
+static void mmu_release_pfn_clean(pfn_t pfn)
+{
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
+}
+

Too many APIs, each slightly different. How do I know which one to call?

It is only used in mmu and it is a static function.

Quoted 4 lines

Please change kvm_release_pfn_*() instead, calling some arch hook (or even #ifdef CONFIG_KVM_HAS_FAST_MMIO) to check for the special case.

We only need to call it on page fault path. If we change the common API other x86 components have to suffer from it.

Quoted 35 lines · Xiao Guangrong

Quoted 21 lines · Avi Kivity

Quoted 20 lines · Xiao Guangrong

We can not directly call kvm_release_pfn_clean to release the pfn since we can meet noslot pfn which is used to cache mmio info into spte

Introduce mmu_release_pfn_clean to do this kind of thing

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

arch/x86/kvm/mmu.c +0 -0

index 399c177..3c10bca 100644
@@ -2432,6 +2432,16 @@ done:
 	return ret;
 }
 

Quoted 12 lines

arch/x86/kvm/mmu.c done: · line 2444

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+/*
+ * The primary user is page fault path which call it to properly
+ * release noslot_pfn.
+ */
+static void mmu_release_pfn_clean(pfn_t pfn)
+{
+	if (!is_error_pfn(pfn))
+		kvm_release_pfn_clean(pfn);
+}
+

Too many APIs, each slightly different. How do I know which one to call?

Quoted 1 line

It is only used in mmu and it is a static function.

Still, how do I know which one to call? The name tells me nothing. When I read the code, how do I know if a call is correct or not?

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 2 lines

Please change kvm_release_pfn_*() instead, calling some arch hook (or even #ifdef CONFIG_KVM_HAS_FAST_MMIO) to check for the special case.

We only need to call it on page fault path. If we change the common API other x86 components have to suffer from it.

This way, I have to suffer from it.

btw, what about another approach, to avoid those paths completely? Avoid calling __direct_map() with error_pfn, and jump to a label after kvm_release_pfn_clean() in page_fault(), etc?