linux-kernel - Re: [PATCH] KVM: x86/tdp_mmu: Trigger the callback only when an interesting change

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: <ZvbB6s6MYZ2dmQxr@google.com>
Date: Fri, 27 Sep 2024 07:32:10 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: Isaku Yamahata <isaku.yamahata@...el.com>, kvm@...r.kernel.org, sagis@...gle.com, 
	chao.gao@...el.com, pbonzini@...hat.com, rick.p.edgecombe@...el.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/tdp_mmu: Trigger the callback only when an
 interesting change

On Fri, Sep 27, 2024, Sean Christopherson wrote:
> On Thu, Sep 26, 2024, Yan Zhao wrote:
> > On Thu, Sep 12, 2024 at 05:07:57PM -0700, Sean Christopherson wrote:
> > > On Thu, Sep 12, 2024, Isaku Yamahata wrote:
> > > Right now, the fixes for make_spte() are sitting toward the end of the massive
> > > kvm_follow_pfn() rework (80+ patches and counting), but despite the size, I am
> > > fairly confident that series can land in 6.13 (lots and lots of small patches).
> > > 
> > > ---
> > > Author:     Sean Christopherson <seanjc@...gle.com>
> > > AuthorDate: Thu Sep 12 16:23:21 2024 -0700
> > > Commit:     Sean Christopherson <seanjc@...gle.com>
> > > CommitDate: Thu Sep 12 16:35:06 2024 -0700
> > > 
> > >     KVM: x86/mmu: Flush TLBs if resolving a TDP MMU fault clears W or D bits
> > >     
> > >     Do a remote TLB flush if installing a leaf SPTE overwrites an existing
> > >     leaf SPTE (with the same target pfn) and clears the Writable bit or the
> > >     Dirty bit.  KVM isn't _supposed_ to clear Writable or Dirty bits in such
> > >     a scenario, but make_spte() has a flaw where it will fail to set the Dirty
> > >     if the existing SPTE is writable.
> > >     
> > >     E.g. if two vCPUs race to handle faults, the KVM will install a W=1,D=1
> > >     SPTE for the first vCPU, and then overwrite it with a W=1,D=0 SPTE for the
> > >     second vCPU.  If the first vCPU (or another vCPU) accesses memory using
> > >     the W=1,D=1 SPTE, i.e. creates a writable, dirty TLB entry, and that is
> > >     the only SPTE that is dirty at the time of the next relevant clearing of
> > >     the dirty logs, then clear_dirty_gfn_range() will not modify any SPTEs
> > >     because it sees the D=0 SPTE, and thus will complete the clearing of the
> > >     dirty logs without performing a TLB flush.
> > But it looks that kvm_flush_remote_tlbs_memslot() will always be invoked no
> > matter clear_dirty_gfn_range() finds a D bit or not.
> 
> Oh, right, I forgot about that.  I'll tweak the changelog to call that out before
> posting.  Hmm, and I'll drop the Cc: stable@ too, as commit b64d740ea7dd ("kvm:
> x86: mmu: Always flush TLBs when enabling dirty logging") was a bug fix, i.e. if
> anything should be backported it's that commit.

Actually, a better idea.  I think it makes sense to fully commit to not flushing
when overwriting SPTEs, and instead rely on the dirty logging logic to do a remote
TLB flush.

E.g. on top of this change in the mega-series is a cleanup to unify the TDP MMU
and shadow MMU logic for clearing Writable and Dirty bits, with this comment
(which is a massaged version of an existing comment for mmu_spte_update()):

/*
 * Whenever an MMU-writable SPTE is overwritten with a read-only SPTE, remote
 * TLBs must be flushed.  Otherwise write-protecting the gfn may find a read-
 * only SPTE, even though the writable SPTE might be cached in a CPU's TLB.
 *
 * Remote TLBs also need to be flushed if the Dirty bit is cleared, as false
 * negatives are not acceptable, e.g. if KVM is using D-bit based PML on VMX.
 *
 * Don't flush if the Accessed bit is cleared, as access tracking tolerates
 * false negatives, and the one path that does care about TLB flushes,
 * kvm_mmu_notifier_clear_flush_young(), uses mmu_spte_update_no_track().
 *
 * Note, this logic only applies to leaf SPTEs.  The caller is responsible for
 * determining whether or not a TLB flush is required when modifying a shadow-
 * present non-leaf SPTE.
 */

But that comment is was made stale by commit b64d740ea7dd.  And looking through
the dirty logging logic, KVM (luckily? thankfully?) flushes based on whether or
not dirty bitmap/ring entries are reaped, not based on whether or not SPTEs were
modified.