lkml.org 
[lkml]   [2023]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module init/exit functions
    Date
    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.
    \
     
     \ /
      Last update: 2023-03-27 00:57    [W:3.302 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site