linux-kernel - Re: [PATCH v4 18/30] KVM: x86/mmu: Zap only TDP MMU leafs in kvm_zap_gfn

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: <Yj9XE/oeQXBp2Ryg@google.com>
Date:   Sat, 26 Mar 2022 18:10:27 +0000
From:   Mingwei Zhang <mizhang@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, kvm <kvm@...r.kernel.org>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        David Hildenbrand <david@...hat.com>,
        David Matlack <dmatlack@...gle.com>,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [PATCH v4 18/30] KVM: x86/mmu: Zap only TDP MMU leafs in
 kvm_zap_gfn_range()

On Fri, Mar 25, 2022, Sean Christopherson wrote:
> On Sun, Mar 13, 2022, Mingwei Zhang wrote:
> > On Thu, Mar 3, 2022 at 11:39 AM Paolo Bonzini <pbonzini@...hat.com> wrote:
> > > @@ -898,13 +879,13 @@ static bool zap_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
> > >   * SPTEs have been cleared and a TLB flush is needed before releasing the
> > >   * MMU lock.
> > >   */
> > > -bool __kvm_tdp_mmu_zap_gfn_range(struct kvm *kvm, int as_id, gfn_t start,
> > > -                                gfn_t end, bool can_yield, bool flush)
> > > +bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, int as_id, gfn_t start, gfn_t end,
> > > +                          bool can_yield, bool flush)
> > >  {
> > >         struct kvm_mmu_page *root;
> > >
> > >         for_each_tdp_mmu_root_yield_safe(kvm, root, as_id)
> > > -               flush = zap_gfn_range(kvm, root, start, end, can_yield, flush);
> > > +               flush = tdp_mmu_zap_leafs(kvm, root, start, end, can_yield, false);
> > 
> > hmm, I think we might have to be very careful here. If we only zap
> > leafs, then there could be side effects. For instance, the code in
> > disallowed_hugepage_adjust() may not work as intended. If you check
> > the following condition in arch/x86/kvm/mmu/mmu.c:2918
> > 
> > if (cur_level > PG_LEVEL_4K &&
> >     cur_level == fault->goal_level &&
> >     is_shadow_present_pte(spte) &&
> >     !is_large_pte(spte)) {
> > 
> > If we previously use 4K mappings in this range due to various reasons
> > (dirty logging etc), then afterwards, we zap the range. Then the guest
> > touches a 4K and now we should map the range with whatever the maximum
> > level we can for the guest.
> > 
> > However, if we just zap only the leafs, then when the code comes to
> > the above location, is_shadow_present_pte(spte) will return true,
> > since the spte is a non-leaf (say a regular PMD entry). The whole if
> > statement will be true, then we never allow remapping guest memory
> > with huge pages.
> 
> But that's at worst a performance issue, and arguably working as intended.  The
> zap in this case is never due to the _guest_ unmapping the pfn, so odds are good
> the guest will want to map back in the same pfns with the same permissions.
> Zapping shadow pages so that the guest can maybe create a hugepage may end up
> being a lot of extra work for no benefit.  Or it may be a net positive.  Either
> way, it's not a functional issue.

This should be a performance bug instead of a functional one. But it
does affect both dirty logging (before Ben's early page promotion) and
our demand paging. So I proposed the fix in here:

https://lore.kernel.org/lkml/20220323184915.1335049-2-mizhang@google.com/T/#me78d50ffac33f4f418432f7b171c50630414ef28

If we see memory corruptions, I bet it could only be that we miss some
TLB flushes, since this patch series is basically trying to avoid
immediate TLB flushing by simply changing ASID (assigning new root).

To debug, maybe force the TLB flushes after zap_gfn_range and see if the
problem still exist?