[PATCH] KVM: coalesced_mmio: NULLify the pointers before freeing ring page and dev

[PATCH] KVM: coalesced_mmio: NULLify the pointers before freeing ring page and dev

[Takuya Yoshikawa]

kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
ring page and dev even after it has freed them.

This may trigger problems, e.g., if we call kvm_coalesced_mmio_free() in
kvm_destroy_vm() or kvm_vm_ioctl_register_coalesced_mmio() afterward.

This patch avoids such problems by NULLifying the pointers.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 virt/kvm/coalesced_mmio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 5169736..11776b7 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
 	return ret;
 
 out_free_dev:
+	kvm->coalesced_mmio_dev = NULL;
 	kfree(dev);
 out_free_page:
+	kvm->coalesced_mmio_ring = NULL;
 	__free_page(page);
 out_err:
 	return ret;
-- 
1.6.3.3

Re: [PATCH] KVM: coalesced_mmio: NULLify the pointers before freeing ring page and dev

[Wei Yongjun]

Takuya Yoshikawa wrote:
> kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
> ring page and dev even after it has freed them.
>
> This may trigger problems, e.g., if we call kvm_coalesced_mmio_free() in
> kvm_destroy_vm() or kvm_vm_ioctl_register_coalesced_mmio() afterward.
>
> This patch avoids such problems by NULLifying the pointers.
>   

After this patch, I think we also need to do some check in
kvm_vcpu_fault() for coalesced_mmio_ring, since the coalesced_mmio
may not be init correctly. This is other issue, so I will send a
new patch for this.

> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  virt/kvm/coalesced_mmio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5169736..11776b7 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
>  	return ret;
>  
>  out_free_dev:
> +	kvm->coalesced_mmio_dev = NULL;
>  	kfree(dev);
>  out_free_page:
> +	kvm->coalesced_mmio_ring = NULL;
>  	__free_page(page);
>  out_err:
>  	return ret;
>   

Re: [PATCH] KVM: coalesced_mmio: NULLify the pointers before freeing ring page and dev

[Takuya Yoshikawa]

Wei Yongjun wrote:
> Takuya Yoshikawa wrote:
>> kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
>> ring page and dev even after it has freed them.
>>
>> This may trigger problems, e.g., if we call kvm_coalesced_mmio_free() in
>> kvm_destroy_vm() or kvm_vm_ioctl_register_coalesced_mmio() afterward.
>>
>> This patch avoids such problems by NULLifying the pointers.
>>   
> 
> After this patch, I think we also need to do some check in
> kvm_vcpu_fault() for coalesced_mmio_ring, since the coalesced_mmio
> may not be init correctly. This is other issue, so I will send a
> new patch for this.

Eh, thanks.

> 
>> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>> ---
>>  virt/kvm/coalesced_mmio.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
>> index 5169736..11776b7 100644
>> --- a/virt/kvm/coalesced_mmio.c
>> +++ b/virt/kvm/coalesced_mmio.c
>> @@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
>>  	return ret;
>>  
>>  out_free_dev:
>> +	kvm->coalesced_mmio_dev = NULL;
>>  	kfree(dev);
>>  out_free_page:
>> +	kvm->coalesced_mmio_ring = NULL;
>>  	__free_page(page);
>>  out_err:
>>  	return ret;
>>   
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] KVM: fix to not use NULL kvm->coalesced_mmio_ring in kvm_vcpu_fault()

[Wei Yongjun]

If coalesced_mmio init fail, the kvm->coalesced_mmio_ring will be set
to NULL. If so, we should return VM_FAULT_SIGBUS in kvm_vcpu_fault()
even if vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 virt/kvm/kvm_main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e758ef7..0e06a6d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1253,7 +1253,8 @@ static int kvm_vcpu_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		page = virt_to_page(vcpu->arch.pio_data);
 #endif
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
-	else if (vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET)
+	else if (vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET &&
+		 vcpu->kvm->coalesced_mmio_ring)
 		page = virt_to_page(vcpu->kvm->coalesced_mmio_ring);
 #endif
 	else
-- 
1.6.3.3

Re: [PATCH] KVM: fix to not use NULL kvm->coalesced_mmio_ring in kvm_vcpu_fault()

[Takuya Yoshikawa]

Wei Yongjun wrote:
> If coalesced_mmio init fail, the kvm->coalesced_mmio_ring will be set
> to NULL. If so, we should return VM_FAULT_SIGBUS in kvm_vcpu_fault()
> even if vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
>  virt/kvm/kvm_main.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e758ef7..0e06a6d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1253,7 +1253,8 @@ static int kvm_vcpu_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  		page = virt_to_page(vcpu->arch.pio_data);
>  #endif
>  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
> -	else if (vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET)
> +	else if (vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET &&
> +		 vcpu->kvm->coalesced_mmio_ring)
>  		page = virt_to_page(vcpu->kvm->coalesced_mmio_ring);
>  #endif
>  	else

Btw, I am not certain if we can continue the normal path even if
kvm_coalesced_mmio_init() fails.

[PATCH -v2] KVM: fix kvm_coalesced_mmio_init()'s error handling

[Takuya Yoshikawa]

This version may be better.

Thanks,
  Takuya

===
kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
ring page and dev even after it has freed them.

Also, if this function fails, though it must be rare, it seems to be
suggesting the system's serious state.

This patch changes the error handling for this function to fix these issues.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 virt/kvm/coalesced_mmio.c |    2 ++
 virt/kvm/kvm_main.c       |    4 +++-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 5169736..11776b7 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
 	return ret;
 
 out_free_dev:
+	kvm->coalesced_mmio_dev = NULL;
 	kfree(dev);
 out_free_page:
+	kvm->coalesced_mmio_ring = NULL;
 	__free_page(page);
 out_err:
 	return ret;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e758ef7..9e72067 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -419,7 +419,9 @@ static struct kvm *kvm_create_vm(void)
 	list_add(&kvm->vm_list, &vm_list);
 	spin_unlock(&kvm_lock);
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
-	kvm_coalesced_mmio_init(kvm);
+	r = kvm_coalesced_mmio_init(kvm);
+	if (r < 0)
+		goto out_err;
 #endif
 out:
 	return kvm;
-- 
1.6.3.3

Re: [PATCH -v2] KVM: fix kvm_coalesced_mmio_init()'s error handling

[Wei Yongjun]

Takuya Yoshikawa wrote:
> This version may be better.
>
> Thanks,
>   Takuya
>
> ===
> kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
> ring page and dev even after it has freed them.
>
> Also, if this function fails, though it must be rare, it seems to be
> suggesting the system's serious state.
>
> This patch changes the error handling for this function to fix these issues.
>   

We must also unregister mmu_notifier in the error path.

> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  virt/kvm/coalesced_mmio.c |    2 ++
>  virt/kvm/kvm_main.c       |    4 +++-
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5169736..11776b7 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
>  	return ret;
>  
>  out_free_dev:
> +	kvm->coalesced_mmio_dev = NULL;
>  	kfree(dev);
>  out_free_page:
> +	kvm->coalesced_mmio_ring = NULL;
>  	__free_page(page);
>  out_err:
>  	return ret;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e758ef7..9e72067 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -419,7 +419,9 @@ static struct kvm *kvm_create_vm(void)
>  	list_add(&kvm->vm_list, &vm_list);
>  	spin_unlock(&kvm_lock);
>  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
> -	kvm_coalesced_mmio_init(kvm);
> +	r = kvm_coalesced_mmio_init(kvm);
> +	if (r < 0)
> +		goto out_err;
>  #endif
>  out:
>  	return kvm;
>   

Re: [PATCH -v2] KVM: fix kvm_coalesced_mmio_init()'s error handling

[Takuya Yoshikawa]

Wei Yongjun wrote:
> Takuya Yoshikawa wrote:
>> This version may be better.
>>
>> Thanks,
>>   Takuya
>>
>> ===
>> kvm_coalesced_mmio_init() keeps to hold the addresses of a coalesced mmio
>> ring page and dev even after it has freed them.
>>
>> Also, if this function fails, though it must be rare, it seems to be
>> suggesting the system's serious state.
>>
>> This patch changes the error handling for this function to fix these issues.
>>   
> 
> We must also unregister mmu_notifier in the error path.

Oh, sorry.

> 
>> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
>> ---
>>  virt/kvm/coalesced_mmio.c |    2 ++
>>  virt/kvm/kvm_main.c       |    4 +++-
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
>> index 5169736..11776b7 100644
>> --- a/virt/kvm/coalesced_mmio.c
>> +++ b/virt/kvm/coalesced_mmio.c
>> @@ -119,8 +119,10 @@ int kvm_coalesced_mmio_init(struct kvm *kvm)
>>  	return ret;
>>  
>>  out_free_dev:
>> +	kvm->coalesced_mmio_dev = NULL;
>>  	kfree(dev);
>>  out_free_page:
>> +	kvm->coalesced_mmio_ring = NULL;
>>  	__free_page(page);
>>  out_err:
>>  	return ret;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index e758ef7..9e72067 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -419,7 +419,9 @@ static struct kvm *kvm_create_vm(void)
>>  	list_add(&kvm->vm_list, &vm_list);
>>  	spin_unlock(&kvm_lock);
>>  #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
>> -	kvm_coalesced_mmio_init(kvm);
>> +	r = kvm_coalesced_mmio_init(kvm);
>> +	if (r < 0)
>> +		goto out_err;
>>  #endif
>>  out:
>>  	return kvm;
>>   

[PATCH -v3 1/2] KVM: introduce kvm_uninit_mmu_notifier()

[Takuya Yoshikawa]

We have kvm_init_mmu_notifier() for registering mmu notifier.
This patch makes the counterpart for unregister.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 virt/kvm/kvm_main.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e758ef7..64b792c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -359,6 +359,11 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
 	return mmu_notifier_register(&kvm->mmu_notifier, current->mm);
 }
 
+static void kvm_uninit_mmu_notifier(struct kvm *kvm)
+{
+	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
+}
+
 #else  /* !(CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER) */
 
 static int kvm_init_mmu_notifier(struct kvm *kvm)
@@ -366,6 +371,11 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
 	return 0;
 }
 
+static void kvm_uninit_mmu_notifier(struct kvm *kvm)
+{
+	kvm_arch_flush_shadow(kvm);
+}
+
 #endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */
 
 static struct kvm *kvm_create_vm(void)
@@ -485,11 +495,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	for (i = 0; i < KVM_NR_BUSES; i++)
 		kvm_io_bus_destroy(kvm->buses[i]);
 	kvm_coalesced_mmio_free(kvm);
-#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
-	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
-#else
-	kvm_arch_flush_shadow(kvm);
-#endif
+	kvm_uninit_mmu_notifier(kvm);
 	kvm_arch_destroy_vm(kvm);
 	hardware_disable_all();
 	mmdrop(mm);
-- 
1.6.3.3

Re: [PATCH -v3 1/2] KVM: introduce kvm_uninit_mmu_notifier()

[Takuya Yoshikawa]

I made this patch for the coalesced mmio's error handling work.

Though, I wanted to finish this work, I have no time this week any more.
I'll do the remaining part next week, sorry.


Takuya Yoshikawa wrote:
> We have kvm_init_mmu_notifier() for registering mmu notifier.
> This patch makes the counterpart for unregister.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  virt/kvm/kvm_main.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e758ef7..64b792c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -359,6 +359,11 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
>  	return mmu_notifier_register(&kvm->mmu_notifier, current->mm);
>  }
>  
> +static void kvm_uninit_mmu_notifier(struct kvm *kvm)
> +{
> +	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
> +}
> +
>  #else  /* !(CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER) */
>  
>  static int kvm_init_mmu_notifier(struct kvm *kvm)
> @@ -366,6 +371,11 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
>  	return 0;
>  }
>  
> +static void kvm_uninit_mmu_notifier(struct kvm *kvm)
> +{
> +	kvm_arch_flush_shadow(kvm);
> +}
> +
>  #endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */
>  
>  static struct kvm *kvm_create_vm(void)
> @@ -485,11 +495,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  	for (i = 0; i < KVM_NR_BUSES; i++)
>  		kvm_io_bus_destroy(kvm->buses[i]);
>  	kvm_coalesced_mmio_free(kvm);
> -#if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
> -	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
> -#else
> -	kvm_arch_flush_shadow(kvm);
> -#endif
> +	kvm_uninit_mmu_notifier(kvm);
>  	kvm_arch_destroy_vm(kvm);
>  	hardware_disable_all();
>  	mmdrop(mm);