linux-kernel - Re: [PATCH v9 04/11] KVM: x86/mmu: Relax locking for kvm_test_age_gfn() and kvm_age

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: <Z60bhK96JnKIgqZQ@google.com>
Date: Wed, 12 Feb 2025 14:07:00 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, David Matlack <dmatlack@...gle.com>, 
	David Rientjes <rientjes@...gle.com>, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Wei Xu <weixugc@...gle.com>, Yu Zhao <yuzhao@...gle.com>, 
	Axel Rasmussen <axelrasmussen@...gle.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 04/11] KVM: x86/mmu: Relax locking for
 kvm_test_age_gfn() and kvm_age_gfn()

On Tue, Feb 04, 2025, James Houghton wrote:
> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> index 22551e2f1d00..e984b440c0f0 100644
> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -142,8 +142,14 @@ bool spte_has_volatile_bits(u64 spte)
>  		return true;
>  
>  	if (spte_ad_enabled(spte)) {
> -		if (!(spte & shadow_accessed_mask) ||
> -		    (is_writable_pte(spte) && !(spte & shadow_dirty_mask)))
> +		/*
> +		 * Do not check the Accessed bit. It can be set (by the CPU)
> +		 * and cleared (by kvm_tdp_mmu_age_spte()) without holding

When possible, don't reference functions by name in comments.  There are situations
where it's unavoidable, e.g. when calling out memory barrier pairs, but for cases
like this, it inevitably leads to stale code.

> +		 * the mmu_lock, but when clearing the Accessed bit, we do
> +		 * not invalidate the TLB, so we can already miss Accessed bit

No "we" in comments or changelog.

> +		 * updates.
> +		 */
> +		if (is_writable_pte(spte) && !(spte & shadow_dirty_mask))
>  			return true;

This 100% belongs in a separate prepatory patch.  And if it's moved to a separate
patch, then the rename can/should happen at the same time.

And with the Accessed check gone, and looking forward to the below change, I think
this is the perfect opportunity to streamline the final check to:

	return spte_ad_enabled(spte) && is_writable_pte(spte) &&
	       !(spte & shadow_dirty_mask);

No need to send another version, I'll move things around when applying.

Also, as discussed off-list I'm 99% certain that with the lockless aging, KVM
must atomically update A/D-disabled SPTEs, as the SPTE can be access-tracked and
restored outside of mmu_lock.  E.g. a task that holds mmu_lock and is clearing
the writable bit needs to update it atomically to avoid its change from being
lost.

I'll slot this is in:

--
From: Sean Christopherson <seanjc@...gle.com>
Date: Wed, 12 Feb 2025 12:58:39 -0800
Subject: [PATCH 03/10] KVM: x86/mmu: Always update A/D-disable SPTEs
 atomically

In anticipation of aging SPTEs outside of mmu_lock, force A/D-disabled
SPTEs to be updated atomically, as aging A/D-disabled SPTEs will mark them
for access-tracking outside of mmu_lock.  Coupled with restoring access-
tracked SPTEs in the fast page fault handler, the end result is that
A/D-disable SPTEs will be volatile at all times.

Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/mmu/spte.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 9663ba867178..0f9f47b4ab0e 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -141,8 +141,11 @@ bool spte_needs_atomic_update(u64 spte)
 	if (!is_writable_pte(spte) && is_mmu_writable_spte(spte))
 		return true;

-	/* Access-tracked SPTEs can be restored by KVM's fast page fault handler. */
-	if (is_access_track_spte(spte))
+	/*
+	 * A/D-disabled SPTEs can be access-tracked by aging, and access-tracked
+	 * SPTEs can be restored by KVM's fast page fault handler.
+	 */
+	if (!spte_ad_enabled(spte))
 		return true;

 	/*
@@ -151,8 +154,7 @@ bool spte_needs_atomic_update(u64 spte)
 	 * invalidate TLBs when aging SPTEs, and so it's safe to clobber the
 	 * Accessed bit (and rare in practice).
 	 */
-	return spte_ad_enabled(spte) && is_writable_pte(spte) &&
-	       !(spte & shadow_dirty_mask);
+	return is_writable_pte(spte) && !(spte & shadow_dirty_mask);
 }

 bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
--