[PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[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 <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 399c177..3c10bca 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2432,6 +2432,16 @@ done:
 	return ret;
 }

+/*
+ * 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);
+}
+
 static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 			 unsigned pt_access, unsigned pte_access,
 			 int user_fault, int write_fault,
@@ -2497,8 +2507,7 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		}
 	}

-	if (!is_error_pfn(pfn))
-		kvm_release_pfn_clean(pfn);
+	mmu_release_pfn_clean(pfn);
 }

 static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)
@@ -2618,7 +2627,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 					      1, ACC_ALL, iterator.sptep);
 			if (!sp) {
 				pgprintk("nonpaging_map: ENOMEM\n");
-				kvm_release_pfn_clean(pfn);
+				mmu_release_pfn_clean(pfn);
 				return -ENOMEM;
 			}

@@ -2882,7 +2891,7 @@ static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, u32 error_code,

 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	mmu_release_pfn_clean(pfn);
 	return 0;
 }

@@ -3350,7 +3359,7 @@ static int tdp_page_fault(struct kvm_vcpu *vcpu, gva_t gpa, u32 error_code,

 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	mmu_release_pfn_clean(pfn);
 	return 0;
 }

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index bf8c42b..f075259 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -544,7 +544,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 out_gpte_changed:
 	if (sp)
 		kvm_mmu_put_page(sp, it.sptep);
-	kvm_release_pfn_clean(pfn);
+	mmu_release_pfn_clean(pfn);
 	return NULL;
 }

@@ -645,7 +645,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, gva_t addr, u32 error_code,

 out_unlock:
 	spin_unlock(&vcpu->kvm->mmu_lock);
-	kvm_release_pfn_clean(pfn);
+	mmu_release_pfn_clean(pfn);
 	return 0;
 }

-- 
1.7.7.6

[PATCH 2/3] KVM: fix release error page

[Xiao Guangrong]

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/vmx.c |   19 ++++++++++++++++---
 arch/x86/kvm/x86.c |   13 ++++++++++---
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 248c2b4..ccea20a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3590,6 +3590,7 @@ static void seg_setup(int seg)

 static int alloc_apic_access_page(struct kvm *kvm)
 {
+	struct page *page;
 	struct kvm_userspace_memory_region kvm_userspace_mem;
 	int r = 0;

@@ -3604,7 +3605,13 @@ static int alloc_apic_access_page(struct kvm *kvm)
 	if (r)
 		goto out;

-	kvm->arch.apic_access_page = gfn_to_page(kvm, 0xfee00);
+	page = gfn_to_page(kvm, 0xfee00);
+	if (is_error_page(page)) {
+		r = -EFAULT;
+		goto out;
+	}
+
+	kvm->arch.apic_access_page = page;
 out:
 	mutex_unlock(&kvm->slots_lock);
 	return r;
@@ -3612,6 +3619,7 @@ out:

 static int alloc_identity_pagetable(struct kvm *kvm)
 {
+	struct page *page;
 	struct kvm_userspace_memory_region kvm_userspace_mem;
 	int r = 0;

@@ -3627,8 +3635,13 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 	if (r)
 		goto out;

-	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
+	page = gfn_to_page(kvm, kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
+	if (is_error_page(page)) {
+		r = -EFAULT;
+		goto out;
+	}
+
+	kvm->arch.ept_identity_pagetable = page;
 out:
 	mutex_unlock(&kvm->slots_lock);
 	return r;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d8fba22..d44edaa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5101,17 +5101,20 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
 			!kvm_event_needs_reinjection(vcpu);
 }

-static void vapic_enter(struct kvm_vcpu *vcpu)
+static int vapic_enter(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct page *page;

 	if (!apic || !apic->vapic_addr)
-		return;
+		return 0;

 	page = gfn_to_page(vcpu->kvm, apic->vapic_addr >> PAGE_SHIFT);
+	if (is_error_page(page))
+		return -EFAULT;

 	vcpu->arch.apic->vapic_page = page;
+	return 0;
 }

 static void vapic_exit(struct kvm_vcpu *vcpu)
@@ -5418,7 +5421,11 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 	}

 	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
-	vapic_enter(vcpu);
+	r = vapic_enter(vcpu);
+	if (r) {
+		srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
+		return r;
+	}

 	r = 1;
 	while (r > 0) {
-- 
1.7.7.6

Re: [PATCH 2/3] KVM: fix release error page

[Avi Kivity]

On 09/07/2012 09:14 AM, Xiao Guangrong wrote:
> 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
> 

Thanks, applied to master for 3.6.


-- 
error compiling committee.c: too many arguments to function

[PATCH 3/3] KVM: MMU: remove unnecessary check

[Xiao Guangrong]

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

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 3c10bca..98cf4bf 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2625,11 +2625,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
 					      iterator.level - 1,
 					      1, ACC_ALL, iterator.sptep);
-			if (!sp) {
-				pgprintk("nonpaging_map: ENOMEM\n");
-				mmu_release_pfn_clean(pfn);
-				return -ENOMEM;
-			}

 			mmu_spte_set(iterator.sptep,
 				     __pa(sp->spt)
-- 
1.7.7.6

Re: [PATCH 3/3] KVM: MMU: remove unnecessary check

[Avi Kivity]

On 09/07/2012 09:15 AM, Xiao Guangrong wrote:
> Checking the return of kvm_mmu_get_page is unnecessary since it is
> guaranteed by memory cache
> 


Thanks, applied.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[Avi Kivity]

On 09/07/2012 09:13 AM, Xiao Guangrong wrote:
> 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 <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
>  arch/x86/kvm/paging_tmpl.h |    4 ++--
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 399c177..3c10bca 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2432,6 +2432,16 @@ done:
>  	return ret;
>  }
> 
> +/*
> + * 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.




-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[Xiao Guangrong]

On 09/10/2012 04:22 PM, Avi Kivity wrote:
> On 09/07/2012 09:13 AM, Xiao Guangrong wrote:
>> 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 <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
>>  arch/x86/kvm/paging_tmpl.h |    4 ++--
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 399c177..3c10bca 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2432,6 +2432,16 @@ done:
>>  	return ret;
>>  }
>>
>> +/*
>> + * 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.

> 
> 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.

Re: [PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[Avi Kivity]

On 09/10/2012 11:37 AM, Xiao Guangrong wrote:
> On 09/10/2012 04:22 PM, Avi Kivity wrote:
>> On 09/07/2012 09:13 AM, Xiao Guangrong wrote:
>>> 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 <xiaoguangrong@linux.vnet.ibm.com>
>>> ---
>>>  arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
>>>  arch/x86/kvm/paging_tmpl.h |    4 ++--
>>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>> index 399c177..3c10bca 100644
>>> --- a/arch/x86/kvm/mmu.c
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -2432,6 +2432,16 @@ done:
>>>  	return ret;
>>>  }
>>>
>>> +/*
>>> + * 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.

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?

> 
>> 
>> 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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] KVM: MMU: release noslot pfn on the fail path properly

[Xiao Guangrong]

On 09/10/2012 05:02 PM, Avi Kivity wrote:
> On 09/10/2012 11:37 AM, Xiao Guangrong wrote:
>> On 09/10/2012 04:22 PM, Avi Kivity wrote:
>>> On 09/07/2012 09:13 AM, Xiao Guangrong wrote:
>>>> 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 <xiaoguangrong@linux.vnet.ibm.com>
>>>> ---
>>>>  arch/x86/kvm/mmu.c         |   19 ++++++++++++++-----
>>>>  arch/x86/kvm/paging_tmpl.h |    4 ++--
>>>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>>>> index 399c177..3c10bca 100644
>>>> --- a/arch/x86/kvm/mmu.c
>>>> +++ b/arch/x86/kvm/mmu.c
>>>> @@ -2432,6 +2432,16 @@ done:
>>>>  	return ret;
>>>>  }
>>>>
>>>> +/*
>>>> + * 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.
> 
> 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?
> 
>>
>>>
>>> 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.

Sorry. :(

> 
> 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?

I will try it.