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

On Thu, Aug 13, 2026 at 04:20:05PM -0700, Sean Christopherson wrote:

On Thu, Aug 13, 2026, Rick P Edgecombe wrote:

quoted

On Thu, 2026-08-13 at 11:51 -0700, Ackerley Tng wrote:

quoted

"Edgecombe, Rick P" [off-list ref] writes:

quoted

On Tue, 2026-08-11 at 10:35 -0700, Ackerley Tng wrote:

quoted

quoted

quoted

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.

quoted

quoted

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.