linux-kernel - Re: [PATCH] KVM: X86: fix tlb_flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YK/y3QgSg+aYk9Z+@google.com>
Date:   Thu, 27 May 2021 19:28:29 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org,
        Lai Jiangshan <laijs@...ux.alibaba.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org
Subject: Re: [PATCH] KVM: X86: fix tlb_flush_guest()

On Thu, May 27, 2021, Sean Christopherson wrote:
> > KVM_REQ_MMU_RELOAD is overkill, nuking the shadow page tables will completely
> > offset the performance gains of the paravirtualized flush.

Argh, I take that back.  The PV KVM_VCPU_FLUSH_TLB flag doesn't distinguish
between flushing a specific mm and flushing the entire TLB.  The HyperV usage
(via KVM_REQ) also throws everything into a single bucket.  A full RELOAD still
isn't necessary as KVM just needs to sync all roots, not blast them away.  For
previous roots, KVM doesn't have a mechanism to defer the sync, so the immediate
fix will need to unload those roots.

And looking at KVM's other flows, __kvm_mmu_new_pgd() and kvm_set_cr3() are also
broken with respect to previous roots.  E.g. if the guest does a MOV CR3 that
flushes the entire TLB, followed by a MOV CR3 with PCID_NOFLUSH=1, KVM will fail
to sync the MMU on the second flush even though the guest can technically rely
on the first MOV CR3 to have synchronized any previous changes relative to the
fisrt MOV CR3.

Lai, if it's ok with you, I'll massage this patch as discussed and fold it into
a larger series to fix the other bugs and do additional cleanup/improvements.

> > I believe the minimal fix is:
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 81ab3b8f22e5..b0072063f9bf 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3072,6 +3072,9 @@ static void kvm_vcpu_flush_tlb_all(struct kvm_vcpu *vcpu)
> >  static void kvm_vcpu_flush_tlb_guest(struct kvm_vcpu *vcpu)
> >  {
> >         ++vcpu->stat.tlb_flush;
> > +
> > +       if (!tdp_enabled)
> > +               kvm_mmu_sync_roots(vcpu);
> >         static_call(kvm_x86_tlb_flush_guest)(vcpu);
> >  }