linux-kernel - Re: [PATCH v4 06/13] KVM: x86/mmu: Bypass __handle_changed

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 26 Jun 2023 14:37:40 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Vipin Sharma <vipinsh@...gle.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Matlack <dmatlack@...gle.com>,
        Ben Gardon <bgardon@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v4 06/13] KVM: x86/mmu: Bypass __handle_changed_spte()
 when clearing TDP MMU dirty bits

On Sun, Jun 25, 2023, Like Xu wrote:
> On 22/3/2023 6:00 am, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 950c5d23ecee..467931c43968 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -517,7 +517,6 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> >    *	    threads that might be modifying SPTEs.
> >    *
> >    * Handle bookkeeping that might result from the modification of a SPTE.
> > - * This function must be called for all TDP SPTE modifications.
> >    */
> >   static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> >   				  u64 old_spte, u64 new_spte, int level,
> > @@ -1689,8 +1688,10 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
> >   							iter.old_spte, dbit,
> >   							iter.level);
> > -		__handle_changed_spte(kvm, iter.as_id, iter.gfn, iter.old_spte,
> > -				      iter.old_spte & ~dbit, iter.level, false);
> > +		trace_kvm_tdp_mmu_spte_changed(iter.as_id, iter.gfn, iter.level,
> 
> Here the first parameter "kvm" is no longer used in this context.
> 
> Please help confirm that for clear_dirty_pt_masked(), should the "struct kvm
> *kvm" parameter be cleared from the list of incoming parameters ?

Hmm, there's only one caller, so keeping @kvm around "just in case" probably
doesn't make sense, e.g. adding it back so that we could do KVM_BUG_ON() in the
future wouldn't require much churn.

That said, I'm tempted to move the lockdep so that it's more obvious why it's safe
for clear_dirty_pt_masked() to use the non-atomic (for non-volatile SPTEs)
tdp_mmu_clear_spte_bits() helper.  for_each_tdp_mmu_root() does its own lockdep,
so the only "loss" in lockdep coverage is if the list is completely empty.

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 512163d52194..0b4f03bef70e 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1600,6 +1600,8 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
                                                   shadow_dirty_mask;
        struct tdp_iter iter;
 
+       lockdep_assert_held_write(&kvm->mmu_lock);
+
        rcu_read_lock();
 
        tdp_root_for_each_leaf_pte(iter, root, gfn + __ffs(mask),
@@ -1646,7 +1648,6 @@ void kvm_tdp_mmu_clear_dirty_pt_masked(struct kvm *kvm,
 {
        struct kvm_mmu_page *root;
 
-       lockdep_assert_held_write(&kvm->mmu_lock);
        for_each_tdp_mmu_root(kvm, root, slot->as_id)
                clear_dirty_pt_masked(kvm, root, gfn, mask, wrprot);
 }