| Message ID | 20240423165328.2853870-1-seanjc@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: x86: Fix supported VM_TYPES caps | expand |
On 4/24/2024 12:53 AM, Sean Christopherson wrote: > Fix a goof where KVM fails to re-initialize the set of supported VM types, > resulting in KVM overreporting the set of supported types when a vendor > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > supported. Hah, this reminds me of the bug of msrs_to_save[] and etc. 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") The series looks good to me. With v2 to move the reset of kvm_cap and set the hardcoded caps earlier, Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> > Fix a similar long-standing bug with supported_mce_cap that is much less > benign, and then harden against us making the same mistake in the future. > > Sean Christopherson (3): > KVM: x86: Fully re-initialize supported_vm_types on vendor module load > KVM: x86: Fully re-initialize supported_mce_cap on vendor module load > KVM: x86: Explicitly zero kvm_caps during vendor module load > > arch/x86/kvm/x86.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > > base-commit: a96cb3bf390eebfead5fc7a2092f8452a7997d1b
On Thu, Apr 25, 2024, Xiaoyao Li wrote: > On 4/24/2024 12:53 AM, Sean Christopherson wrote: > > Fix a goof where KVM fails to re-initialize the set of supported VM types, > > resulting in KVM overreporting the set of supported types when a vendor > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > > supported. > > Hah, this reminds me of the bug of msrs_to_save[] and etc. > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") Yeah, and we had the same bug with allow_smaller_maxphyaddr 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload") If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful for userspace, I would more seriously consider pursuing that in advance of multi-KVM[*]. Because having KVM be fully self-contained has some *really* nice properties, e.g. eliminates this entire class of bugs, eliminates a huge pile of exports, etc. : > Since the symbols in the new module are invisible outside, I recommend: : > new kvm_intel.ko = kvm_intel.ko + kvm.ko : > new kvm_amd.ko = kvm_amd.ko + kvm.ko : : Yeah, Paolo also suggested this at LPC. [*] https://lore.kernel.org/all/ZWYtDGH5p4RpGYBw@google.com
On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote: > On Thu, Apr 25, 2024, Xiaoyao Li wrote: > > On 4/24/2024 12:53 AM, Sean Christopherson wrote: > > > Fix a goof where KVM fails to re-initialize the set of supported VM types, > > > resulting in KVM overreporting the set of supported types when a vendor > > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > > > supported. > > > > Hah, this reminds me of the bug of msrs_to_save[] and etc. > > > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") > > Yeah, and we had the same bug with allow_smaller_maxphyaddr > > 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload") > > If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful > for userspace, > Do we have any real side effects for _userspace_ here? > I would more seriously consider pursuing that in advance of > multi-KVM[*]. Because having KVM be fully self-contained has some *really* nice > properties, e.g. eliminates this entire class of bugs, eliminates a huge pile of > exports, etc. > > : > Since the symbols in the new module are invisible outside, I recommend: > : > new kvm_intel.ko = kvm_intel.ko + kvm.ko > : > new kvm_amd.ko = kvm_amd.ko + kvm.ko > : > : Yeah, Paolo also suggested this at LPC. > > [*] https://lore.kernel.org/all/ZWYtDGH5p4RpGYBw@google.com > +1. This makes life a lot easier.
On Fri, Apr 26, 2024, Kai Huang wrote: > On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote: > > On Thu, Apr 25, 2024, Xiaoyao Li wrote: > > > On 4/24/2024 12:53 AM, Sean Christopherson wrote: > > > > Fix a goof where KVM fails to re-initialize the set of supported VM types, > > > > resulting in KVM overreporting the set of supported types when a vendor > > > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > > > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > > > > supported. > > > > > > Hah, this reminds me of the bug of msrs_to_save[] and etc. > > > > > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") > > > > Yeah, and we had the same bug with allow_smaller_maxphyaddr > > > > 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload") > > > > If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful > > for userspace, > > > > Do we have any real side effects for _userspace_ here? kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the big problem. E.g. params from the kernel command line for kvm.??? will become ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK params don't supporting such aliasing/links. I don't think there are any deal breakers, but I don't expect it to Just Work either.
On 27/04/2024 3:47 am, Sean Christopherson wrote: > On Fri, Apr 26, 2024, Kai Huang wrote: >> On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote: >>> On Thu, Apr 25, 2024, Xiaoyao Li wrote: >>>> On 4/24/2024 12:53 AM, Sean Christopherson wrote: >>>>> Fix a goof where KVM fails to re-initialize the set of supported VM types, >>>>> resulting in KVM overreporting the set of supported types when a vendor >>>>> module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, >>>>> reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as >>>>> supported. >>>> >>>> Hah, this reminds me of the bug of msrs_to_save[] and etc. >>>> >>>> 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") >>> >>> Yeah, and we had the same bug with allow_smaller_maxphyaddr >>> >>> 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload") >>> >>> If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful >>> for userspace, >>> >> >> Do we have any real side effects for _userspace_ here? > > kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the > big problem. E.g. params from the kernel command line for kvm.??? will become > ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create > a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK > params don't supporting such aliasing/links. > > I don't think there are any deal breakers, but I don't expect it to Just Work either. Perhaps we can make the kvm.ko as a dummy module which only keeps the module parameters for backward compatibility?
On Mon, Apr 29, 2024, Kai Huang wrote: > On 27/04/2024 3:47 am, Sean Christopherson wrote: > > On Fri, Apr 26, 2024, Kai Huang wrote: > > > On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote: > > > > On Thu, Apr 25, 2024, Xiaoyao Li wrote: > > > > > On 4/24/2024 12:53 AM, Sean Christopherson wrote: > > > > > > Fix a goof where KVM fails to re-initialize the set of supported VM types, > > > > > > resulting in KVM overreporting the set of supported types when a vendor > > > > > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > > > > > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > > > > > > supported. > > > > > > > > > > Hah, this reminds me of the bug of msrs_to_save[] and etc. > > > > > > > > > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists") > > > > > > > > Yeah, and we had the same bug with allow_smaller_maxphyaddr > > > > > > > > 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload") > > > > > > > > If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful > > > > for userspace, > > > > > > > > > > Do we have any real side effects for _userspace_ here? > > > > kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the > > big problem. E.g. params from the kernel command line for kvm.??? will become > > ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create > > a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK > > params don't supporting such aliasing/links. > > > > I don't think there are any deal breakers, but I don't expect it to Just Work either. > > Perhaps we can make the kvm.ko as a dummy module which only keeps the module > parameters for backward compatibility? Keeping parameters in a dummy kvm.ko would largely defeat the purpose of linking everything into vendor modules, i.e. would make it possible for the parameters to hold a stale value.
Queued, thanks. Paolo
On Mon, Apr 29, 2024 at 5:56 PM Sean Christopherson <seanjc@google.com> wrote: > > Perhaps we can make the kvm.ko as a dummy module which only keeps the module > > parameters for backward compatibility? > > Keeping parameters in a dummy kvm.ko would largely defeat the purpose of linking > everything into vendor modules, i.e. would make it possible for the parameters to > hold a stale value. We have the following read-write params: parm: nx_huge_pages:bool parm: nx_huge_pages_recovery_ratio:uint parm: nx_huge_pages_recovery_period_ms:uint parm: flush_on_reuse:bool parm: ignore_msrs:bool parm: report_ignored_msrs:bool parm: min_timer_period_us:uint parm: tsc_tolerance_ppm:uint parm: lapic_timer_advance_ns:int parm: force_emulation_prefix:int parm: pi_inject_timer:bint parm: eager_page_split:bool parm: halt_poll_ns:uint parm: halt_poll_ns_grow:uint parm: halt_poll_ns_grow_start:uint parm: halt_poll_ns_shrink:uint Vendor modules do not muck with them (the only one that is exported is report_ignored_msrs for which permanency is obviously harmless). And the following read-only params: parm: tdp_mmu:bool parm: mmio_caching:bool parm: kvmclock_periodic_sync:bool parm: vector_hashing:bool parm: enable_vmware_backdoor:bool parm: enable_pmu:bool parm: mitigate_smt_rsb:bool The only really bad one is tdp_mmu, which can change depending on the ept/npt parameters of kvm-intel/kvm-amd; everything else is okay to have in a common module. mitigate_smt_rsb could (should?) move to kvm-amd.ko if the modules were unified with kvm.ko as a dummy one. Paolo
On Tue, Apr 23, 2024 at 6:53 PM Sean Christopherson <seanjc@google.com> wrote: > > Fix a goof where KVM fails to re-initialize the set of supported VM types, > resulting in KVM overreporting the set of supported types when a vendor > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko, > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as > supported. > > Fix a similar long-standing bug with supported_mce_cap that is much less > benign, and then harden against us making the same mistake in the future. > > Sean Christopherson (3): > KVM: x86: Fully re-initialize supported_vm_types on vendor module load > KVM: x86: Fully re-initialize supported_mce_cap on vendor module load > KVM: x86: Explicitly zero kvm_caps during vendor module load Applied, thanks. Paolo