On Tuesday, March 14, 2023 2:40 AM, Isaku Yamahata wrote:
> > I had a patch to fix a bug here, maybe you can take it:
> >
> > kvm_x86_vendor_init copies vt_x86_ops to kvm_x86_ops.
> > vt_x86_ops.vm_size needs to be updated before calling
> > kvm_x86_vendor_init so that kvm_x86_ops can get the correct vm_size.
>
> Thanks for catching it. With your patch, vm_size is always max(sizeof struct
> kvm_vmx, sizeof strut kvm_tdx) even when the admin sets kvm_intel.tdx=true
> and tdx is disabled by error.
This seems to be another bug, where enable_tdx should be set to false if tdx
fails to be enabled:
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index d6a9f705a2a1..ef1e794efb97 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -4552,6 +4552,7 @@ static int tdx_module_setup(void)
ret = tdx_enable();
if (ret) {
+ enable_tdx = false;
pr_info("Failed to initialize TDX module.\n");
return ret;
}
>
> option 1: Ignore such waste. Your patch. The difference is small and it's only
> the error case. Locally I have the following values.
> sizeof(struct kvm_vmx) = 44576
> sizeof(struct vcpu_vmx) = 10432
> sizeof(struct kvm_tdx)= 44632
> sizeof(struct vcpu_tdx) = 8192
> I suspect the actual allocation size for struct kvm is same. That's
> the reason why I didn't hit problem.
No, the actual allocation size isn't same.
You didn’t get error notices because the kvm_tdx fields are still located
in the same page as that allocated for kvm_vmx, though that part of memory
isn’t explicitly allocated. If kvm_tdx size grows and crosses the page boundary,
you will see unexpected faults. Or if this part of memory (illegally used by
kvm_tdx) later happens to be given to other kernel components to use, you
would see random errors somewhere.
>
> option 2: Explicitly update vm_size after kvm_x86_vendor_init()
> struct kvm_x86_ops isn't exported. It would be ugly.
>
> option 3: Allow setup_hardware() to update vm_size.
> setup_hardware(void) => setup_hardware(unsigned int *vm_size)
> It's confusing because kvm_x86_ops.vm_size is already initialized.
>
> Let's go with option 1(your patch).
Sounds good.
