Re: [PATCH v10 11/41] KVM: guest_memfd: Ensure pages are not in use before conversion

[Yan Zhao <yan.y.zhao@intel.com>]

On Thu, Aug 13, 2026 at 04:20:05PM -0700, Sean Christopherson wrote:
> On Thu, Aug 13, 2026, Rick P Edgecombe wrote:
> > On Thu, 2026-08-13 at 11:51 -0700, Ackerley Tng wrote:
> > > "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> writes:
> > > 
> > > > On Tue, 2026-08-11 at 10:35 -0700, Ackerley Tng wrote:
> > > > > > > Would like to see what Sean thinks of this. Either way, is it okay to
> > > > > > > follow up after conversions lands?
> > > > > > Let's see what Sean thinks of this :)
> > > > > > I raised this because the issue was encountered by one TDX's stress
> > > > > > selftest.
> > > > > 
> > > > > Which stress selftest is this? I can try running this on my side too.
> > > > 
> > > > We have some selftests that are built on the basic TDX selftests. One just
> > > > hammers the MMU stuff with a bunch of zaps and also weird stuff from the guest.
> > > > It was eventually too much work to try to keep the internal enhancements rebased
> > > 
> > > Would like all the comments we can get on TDX selftests v14 [1]!
> > 
> > I think we had a few. Let me try to round up some more folks.
> > 
> > > 
> > > > nicely so we actually just run an old branch's TDX selftests against newer
> > > > kernels. So the branch is a bit of a pile, and not really suitable for sharing.
> > > > We plan to clean it and upstream it when the path clears. So it would really
> > > > help to get those basic ones upstream. We remain happy to help, so please let us
> > > > know.
> > > 
> > > I guess at this point I'm hoping y'all and Sean are okay that this
> > > conversions series merges, and we let this stress test failure be
> > > handled later. I'll be around to fix things :)
> > > 
> > > I'd say the line of sight to fixing this would be when the KVM MMU only
> > > gets PFNs (and no pages at all) from guest_memfd.
> > 
> > Hmm, I think we shouldn't upstream a uABI that we don't have line of sight to
> > making robust. So it would be good to settle this thread at least.
> 
> This isn't uABI.  You're talking about hitting a race condition between one task
Hmm. Perhaps it is not a uABI issue, since users are allowed to retry. However,
it is hard to convince me that it makes sense to require users to retry a
private-to-shared conversion before a GFN has ever been mapped, given that a
retry is not required when the GFN is currently in use by the guest.

> converting a page and another faulting in the same page.  An NMI, SMI, or IRQ at
> just the right/wrong time, especially on a preemptible kernel, could lead to the
> same test failures, even if KVM drops the refcount "immediately".
Could you elaborate on how an NMI, SMI, or IRQ at just the right/wrong time
could lead to the same test failures?

Do you mean they can cause a fault to be retried? Our test failure is an EAGAIN
returned from a private-to-shared conversion before the page has even been
mapped as private.

> That said, I am 100% in favor of not handing the caller a struct page.  Now that
> the TDX APIs no longer require one, it's more than feasible.  But, we absolutely
> shouldn't just nullify the pointer, we should drop the param entirely.  Not just
> because it's cleaner, but because it also forces an audit of the callers to see
> if they subtly require a refcount (spoiler alert).
Yeah, I also considered dropping the param entirely and was terrified by the
lines of changes :)

If you are in favor of not handing the caller a struct page, the following
changes should also be required on top of your change.

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 8ef16ccf26ce..d5aa197d2cbf 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1613,7 +1613,6 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
 	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
 	struct kvm_pgtable *pgt = s2fd->vcpu->arch.hw_mmu->pgt;
 	unsigned long mmu_seq;
-	struct page *page;
 	struct kvm *kvm = s2fd->vcpu->kvm;
 	void *memcache = NULL;
 	kvm_pfn_t pfn;
@@ -1681,7 +1680,6 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd)
 	}
 
 out_unlock:
-	kvm_release_faultin_page(kvm, page, !!ret, prot & KVM_PGTABLE_PROT_W);
 	kvm_fault_unlock(kvm);
 
 	if ((prot & KVM_PGTABLE_PROT_W) && !ret)
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index c982a6454fc9..43523bb17621 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -1360,7 +1360,7 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
 	bool write_fault, writable;
 	unsigned long mmu_seq;
 	struct vncr_tlb *vt;
-	struct page *page;
+	struct page *page = NULL;
 	u64 va, pfn, gfn;
 	int ret;
 
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 3d3eb8387cd0..c5ba2c8cad74 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -4017,7 +4017,6 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
 	struct kvm *kvm = vcpu->kvm;
 	gfn_t gfn = gpa_to_gfn(gpa);
 	unsigned long mmu_seq;
-	struct page *page;
 	kvm_pfn_t pfn;
 
 	lockdep_assert_held(&svm->sev_es.snp_vmsa_mutex);
@@ -4077,8 +4076,6 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
 	else
 		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
 	read_unlock(&kvm->mmu_lock);
-
-	kvm_release_page_clean(page);
 }
 
 /*
 
> The lone holdout at this point is sev_handle_rmp_fault(), which could end up
> PSMASH-ing a PFN that has since been freed by KVM.  Assuming holding mmu_lock
> while doing RMP operations is ok, something like the below?  Completely untested.
Tested successfully after applying the above fix and the typo correction.

@@ -5073,7 +5070,7 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
 	if (rmp_level == PG_LEVEL_4K)
 		goto out;
 
-	scoped_guard(read_lock)(&kvm->mmu_lock) {
+	scoped_guard(read_lock, &kvm->mmu_lock) {
 		if (mmu_invalidate_retry_gfn(kvm, mmu_seq, gfn))
 			goto out;
 

> As for in-place conversion, this is not a blocker.
Sorry. I didn't intend to block in-place conversion.
I encountered this issue during testing, so reported it.