vVMX: Cleanup partial vCPU initialization

[PATCH] vVMX: Cleanup partial vCPU initialization

Posted by Ross Lagerwall 5 days, 3 hours ago

If nested vCPU initialization fails, cleanup the allocated memory since
it is no longer handled by the caller.

Fixes: c47984aabead ("nvmx: implement support for MSR bitmaps")
Fixes: f5bdb4aaa165 ("x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM")
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e4f3a5fe4c71..cf9aecb4c3e4 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -53,6 +53,13 @@ void nvmx_cpu_dead(unsigned int cpu)
     XFREE(per_cpu(vvmcs_buf, cpu));
 }
 
+static void vcpu_relinquish_resources(struct vcpu *v)
+{
+    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+
+    FREE_XENHEAP_PAGE(nvmx->msr_merged);
+}
+
 int cf_check nvmx_vcpu_initialise(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -89,7 +96,7 @@ int cf_check nvmx_vcpu_initialise(struct vcpu *v)
         if ( !vmread_bitmap )
         {
             gdprintk(XENLOG_ERR, "nest: allocation for vmread bitmap failed\n");
-            return -ENOMEM;
+            goto err;
         }
         v->arch.hvm.vmx.vmread_bitmap = vmread_bitmap;
 
@@ -99,7 +106,7 @@ int cf_check nvmx_vcpu_initialise(struct vcpu *v)
         if ( !vmwrite_bitmap )
         {
             gdprintk(XENLOG_ERR, "nest: allocation for vmwrite bitmap failed\n");
-            return -ENOMEM;
+            goto err;
         }
         v->arch.hvm.vmx.vmwrite_bitmap = vmwrite_bitmap;
 
@@ -124,7 +131,7 @@ int cf_check nvmx_vcpu_initialise(struct vcpu *v)
     {
         nvmx->msr_merged = alloc_xenheap_page();
         if ( !nvmx->msr_merged )
-            return -ENOMEM;
+            goto err;
     }
 
     nvmx->ept.enabled = 0;
@@ -139,6 +146,11 @@ int cf_check nvmx_vcpu_initialise(struct vcpu *v)
     nvmx->msrbitmap = NULL;
     INIT_LIST_HEAD(&nvmx->launched_list);
     return 0;
+
+ err:
+    nvmx_vcpu_destroy(v);
+    vcpu_relinquish_resources(v);
+    return -ENOMEM;
 }
  
 void cf_check nvmx_vcpu_destroy(struct vcpu *v)
@@ -183,13 +195,6 @@ void cf_check nvmx_vcpu_destroy(struct vcpu *v)
     }
 }
 
-static void vcpu_relinquish_resources(struct vcpu *v)
-{
-    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
-
-    FREE_XENHEAP_PAGE(nvmx->msr_merged);
-}
-
 void cf_check nvmx_domain_relinquish_resources(struct domain *d)
 {
     struct vcpu *v;
-- 
2.51.0

Re: [PATCH] vVMX: Cleanup partial vCPU initialization

Posted by Andrew Cooper 5 days, 1 hour ago

On 24/10/2025 5:20 pm, Ross Lagerwall wrote:
> If nested vCPU initialization fails, cleanup the allocated memory since
> it is no longer handled by the caller.
>
> Fixes: c47984aabead ("nvmx: implement support for MSR bitmaps")
> Fixes: f5bdb4aaa165 ("x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM")
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

AFAICT, this is because of hvm_vcpu_initialise() not calling
nestedhvm_vcpu_destroy() if nestedhvm_vcpu_initialise() fails?

I think this is an improvement in the short term, but we really need to
fix our object lifecycle.  Relatedly, I should dust off the fault-ttl
series because it would be able to find this failure automatically.

~Andrew

Re: [PATCH] vVMX: Cleanup partial vCPU initialization

Posted by Ross Lagerwall 2 days, 9 hours ago

On 10/24/25 7:08 PM, Andrew Cooper wrote:
> On 24/10/2025 5:20 pm, Ross Lagerwall wrote:
>> If nested vCPU initialization fails, cleanup the allocated memory since
>> it is no longer handled by the caller.
>>
>> Fixes: c47984aabead ("nvmx: implement support for MSR bitmaps")
>> Fixes: f5bdb4aaa165 ("x86/hvm: Obsolete the use of HVM_PARAM_NESTEDHVM")
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> AFAICT, this is because of hvm_vcpu_initialise() not calling
> nestedhvm_vcpu_destroy() if nestedhvm_vcpu_initialise() fails?
> 
> I think this is an improvement in the short term, but we really need to
> fix our object lifecycle.  Relatedly, I should dust off the fault-ttl
> series because it would be able to find this failure automatically.
> 
> ~Andrew

Having hvm_vcpu_initialise() call nestedhvm_vcpu_destroy() would be one 
way of fixing it (albeit it is complicated because some cleanup is also 
done in vcpu_relinquish_resources()). But IMO generally a partial 
failure of an initialization function should be handled up internally 
rather than expecting the caller to deal with it.

Ross