Messages in this thread | | | Date | Mon, 13 Dec 2021 15:28:11 +0100 | Subject | Re: [PATCH MANUALSEL 5.15 6/9] KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN | From | Paolo Bonzini <> |
| |
On 12/13/21 15:19, Sasha Levin wrote: > From: Vitaly Kuznetsov <vkuznets@redhat.com> > > [ Upstream commit feb627e8d6f69c9a319fe279710959efb3eba873 ] > > Commit 63f5a1909f9e ("KVM: x86: Alert userspace that KVM_SET_CPUID{,2} > after KVM_RUN is broken") officially deprecated KVM_SET_CPUID{,2} ioctls > after first successful KVM_RUN and promissed to make this sequence forbiden > in 5.16. It's time to fulfil the promise. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Message-Id: <20211122175818.608220-3-vkuznets@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Sasha Levin <sashal@kernel.org> > --- > arch/x86/kvm/mmu/mmu.c | 28 +++++++++++----------------- > arch/x86/kvm/x86.c | 19 +++++++++++++++++++ > 2 files changed, 30 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 0a88cb4f731f4..31392e492ce5e 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -5022,6 +5022,14 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu) > /* > * Invalidate all MMU roles to force them to reinitialize as CPUID > * information is factored into reserved bit calculations. > + * > + * Correctly handling multiple vCPU models with respect to paging and > + * physical address properties) in a single VM would require tracking > + * all relevant CPUID information in kvm_mmu_page_role. That is very > + * undesirable as it would increase the memory requirements for > + * gfn_track (see struct kvm_mmu_page_role comments). For now that > + * problem is swept under the rug; KVM's CPUID API is horrific and > + * it's all but impossible to solve it without introducing a new API. > */ > vcpu->arch.root_mmu.mmu_role.ext.valid = 0; > vcpu->arch.guest_mmu.mmu_role.ext.valid = 0; > @@ -5029,24 +5037,10 @@ void kvm_mmu_after_set_cpuid(struct kvm_vcpu *vcpu) > kvm_mmu_reset_context(vcpu); > > /* > - * KVM does not correctly handle changing guest CPUID after KVM_RUN, as > - * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't > - * tracked in kvm_mmu_page_role. As a result, KVM may miss guest page > - * faults due to reusing SPs/SPTEs. Alert userspace, but otherwise > - * sweep the problem under the rug. > - * > - * KVM's horrific CPUID ABI makes the problem all but impossible to > - * solve, as correctly handling multiple vCPU models (with respect to > - * paging and physical address properties) in a single VM would require > - * tracking all relevant CPUID information in kvm_mmu_page_role. That > - * is very undesirable as it would double the memory requirements for > - * gfn_track (see struct kvm_mmu_page_role comments), and in practice > - * no sane VMM mucks with the core vCPU model on the fly. > + * Changing guest CPUID after KVM_RUN is forbidden, see the comment in > + * kvm_arch_vcpu_ioctl(). > */ > - if (vcpu->arch.last_vmentry_cpu != -1) { > - pr_warn_ratelimited("KVM: KVM_SET_CPUID{,2} after KVM_RUN may cause guest instability\n"); > - pr_warn_ratelimited("KVM: KVM_SET_CPUID{,2} will fail after KVM_RUN starting with Linux 5.16\n"); > - } > + KVM_BUG_ON(vcpu->arch.last_vmentry_cpu != -1, vcpu->kvm); > } > > void kvm_mmu_reset_context(struct kvm_vcpu *vcpu) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b7aa845f7beee..2a584070833f9 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5088,6 +5088,17 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > struct kvm_cpuid __user *cpuid_arg = argp; > struct kvm_cpuid cpuid; > > + /* > + * KVM does not correctly handle changing guest CPUID after KVM_RUN, as > + * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't > + * tracked in kvm_mmu_page_role. As a result, KVM may miss guest page > + * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with > + * the core vCPU model on the fly, so fail. > + */ > + r = -EINVAL; > + if (vcpu->arch.last_vmentry_cpu != -1) > + goto out; > + > r = -EFAULT; > if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid))) > goto out; > @@ -5098,6 +5109,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > struct kvm_cpuid2 __user *cpuid_arg = argp; > struct kvm_cpuid2 cpuid; > > + /* > + * KVM_SET_CPUID{,2} after KVM_RUN is forbidded, see the comment in > + * KVM_SET_CPUID case above. > + */ > + r = -EINVAL; > + if (vcpu->arch.last_vmentry_cpu != -1) > + goto out; > + > r = -EFAULT; > if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid))) > goto out; >
NACK
the error says "starting with Linux 5.16" for a reason... :)
Paolo
| |