Re: [PATCH v2 2/4] KVM: SEV: Drop page refcount early during RMP fault handling

[Michael Roth]

On Thu, Aug 20, 2026 at 03:35:31PM -0700, Ackerley Tng wrote:
> Michael Roth <michael.roth@xxxxxxx> writes:
> 
> > On Thu, Aug 20, 2026 at 07:58:19AM -0700, Ackerley Tng wrote:
> >> Michael Roth <michael.roth@xxxxxxx> writes:
> >>
> >> > On Tue, Aug 18, 2026 at 09:15:53AM +0000, Ackerley Tng wrote:
> >> >> From: Sean Christopherson <seanjc@xxxxxxxxxx>
> >> >>
> >> >> When handling an RMP fault, KVM retrieves the PFN for a private GPA from
> >> >> guest_memfd.
> >> >>
> >> >> Drop the page reference immediately after retrieving the PFN instead of
> >> >> holding it across the entire handler so that the later patch can follow up
> >> >> with completely not returning refcounted pages from kvm_gmem_get_pfn().
> >> >
> >> > Regarding this point:
> >> >
> >> >>
> >> >> On a first look, existing RMP table handling (psmash and checking for
> >> >> errors) might seem like it works fine, since truncation of the page from
> >> >> guest_memfd would have called rmp_make_shared() and removed the PFN from
> >> >> the RMP table. However, that is insufficient since a freed page may already
> >> >> be used in a different SNP VM.
> >> >
> >> > In the code this patch is applied on top of, I think the kvm_gmem_get_pfn()
> >> > ref is enough to avoid the reused-by-another-SNP-VM scenario until after
> >> > caller releases the ref, so I think the above explanation should be adjusted
> >> > to also be preparatory for "the later patch".
> >> >
> >>
> >> I think this sequence of events is possible:
> >>
> >> CPU 0: sev_handle_rmp_fault()
> >> CPU 0:   kvm_gmem_get_pfn()
> >> CPU 0:     filemap_invalidate_lock()
> >> CPU 0:     refcount++
> >> CPU 0:     filemap_invalidate_unlock()
> >> CPU 0:     refcount-- <<== because kvm_release_page_unused(page);
> >
> > Prior to this patch, the kvm_release_page_unused() isn't done until
> > after the snp_lookup_rmpentry()/snp_rmptable_psmash().
> >
> > It's possible CPU 1 truncate sequence below can run concurrently
> > after filemap_invalidate_unlock() above, but even though it calls
> > free_folio(), which might zap the RMP entry, the filemap code will
> > only put the refs that it has on the folio so it wouldn't actually
> > get freed back to the buggy allocator, so it doesn't seem like the
> > psmash-another-guest scenario is reachable. I could certainly be
> > misreading things though.
> >
> 
> Ah I see what you mean. I think we mean the same thing, let me add to
> the commit message that I meant after dropping the refcount early. Does
> this help?
> 
>   The filemap_invalidate_lock() is already dropped in kvm_gmem_get_pfn()
>   before returning to sev_handle_rmp_fault(). After dropping the
>   refcount earlier with kvm_release_page_unused(), these scenarios are
>   possible:
> 
>   1. Since the filemap_invalidate_lock() is dropped, the page can be
>      truncated (or in future, converted), and the RMP entry is now
>      shared.
> 
>      In this case, existing RMP table handling (psmash and checking for
>      errors) would be sufficient. On finding a shared entry, psmashing
>      would fail gracefully and no warning would be emitted.
> 
>   2. The page is truncated and freed, and then re-allocated to another
>      SNP VM. The RMP entry is now assigned, but to another SNP VM.
> 
>      To address this, adopt the MMU invalidation protocol to guard
>      psmashing.

This reads kinda weird to me, as if with #2 we're documenting a "bug" that
this patch fixes, but the bug would only exist if we partially applied the
bits of this patch the drops the ref counts earlier and left out the
bits of the patch that introduce the mmu notifier logic that replaces it.

I think with patch 1 applied (which covers the
psmash-a-now-shared-entry case while retaining the original refcount
logic), the only thing this patch is doing is replacing the elevated
refcount logic with the MMU invalidation logic as prep for dropping
reliance of refcounts entirely.

I think if the wording was simplified to just say something to that effect
it would make it clearer that this and patch #3 are prep for #4, and that
patch #1 is the only patch here that would make potentially make sense for
a downstream to backport without the other bits.

-Mike

> 
> > The psmash-a-now-shared-entry scenario because of a race with the
> > VMM/truncate path seems possible prior to this patch, but that's
> > pretty similar to the psmash-a-now-shared-entry because of a race
> > with the guest scenario and which would generate spurious warning
> > messages to console, and that would be similarly addressed via
> > patch 1 I think. I'm not sure
> >
> 
> Yup. I like your psmash-another-guest vs psmash-a-now-shared-entry
> classification.
> 
> > -Mike
> >
> >>
> >> CPU 1: truncate()
> >> CPU 1:   filemap_invalidate_lock()
> >> CPU 1:   refcount--
> >> CPU 1:     kvm_gmem_free_folio()
> >> CPU 1:       sev_gmem_make_shared()
> >> CPU 1:     folio is freed
> >> CPU 1:   filemap_invalidate_unlock()
> >>
> >> CPU 2: in some other SNP VM,
> >> CPU 2: kvm_gmem_get_pfn() gets the freed folio
> >> CPU 2:   sev_gmem_make_private()
> >>
> >> CPU 0:   snp_lookup_rmpentry(pfn, &assigned, &rmp_level);
> >>            <<== it's assigned but to some other SNP VM
> >> CPU 0:   snp_rmptable_psmash(pfn);
> >>            <<== this psmash would be smashing in some other SNP VM
> >>
> >> And hence I think we do need to check for invalidations using the MMU
> >> invalidation protocol.
> >>
> >> > Other than that:
> >> >
> >> > Reviewed-by: Michael Roth <michael.roth@xxxxxxx>
> >> >
> >> >>
> >> >> Hence, adopt the MMU invalidation protocol to guard committing anything
> >> >> based on the PFN.
> >> >>
> >> >> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> >> >> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> >> >> ---
> >> >>  arch/x86/kvm/svm/sev.c | 39 ++++++++++++++++++++++++---------------
> >> >>  1 file changed, 24 insertions(+), 15 deletions(-)
> >> >>
> >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >> >> index b2738362a928b..b34b11d7f8fad 100644
> >> >> --- a/arch/x86/kvm/svm/sev.c
> >> >> +++ b/arch/x86/kvm/svm/sev.c
> >> >> @@ -5003,6 +5003,7 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
> >> >>  	struct kvm_memory_slot *slot;
> >> >>  	struct kvm *kvm = vcpu->kvm;
> >> >>  	int order, rmp_level, ret;
> >> >> +	unsigned long mmu_seq;
> >> >>  	struct page *page;
> >> >>  	bool assigned;
> >> >>  	kvm_pfn_t pfn;
> >> >> @@ -5030,18 +5031,22 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
> >> >>  		return;
> >> >>  	}
> >> >>
> >> >> +	mmu_seq = kvm->mmu_invalidate_seq;
> >> >> +	smp_rmb();
> >> >> +
> >> >>  	ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &page, &order);
> >> >>  	if (ret) {
> >> >>  		pr_warn_ratelimited("SEV: Unexpected RMP fault, no backing page for private GPA 0x%llx\n",
> >> >>  				    gpa);
> >> >>  		return;
> >> >>  	}
> >> >> +	kvm_release_page_unused(page);
> >> >>
> >> >>  	ret = snp_lookup_rmpentry(pfn, &assigned, &rmp_level);
> >> >>  	if (ret || !assigned) {
> >> >>  		pr_warn_ratelimited("SEV: Unexpected RMP fault, no assigned RMP entry found for GPA 0x%llx PFN 0x%llx error %d\n",
> >> >>  				    gpa, pfn, ret);
> >> >> -		goto out_no_trace;
> >> >> +		return;
> >> >>  	}
> >> >>
> >> >>  	/*
> >> >> @@ -5069,27 +5074,31 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
> >> >>  	if (rmp_level == PG_LEVEL_4K)
> >> >>  		goto out;
> >> >>
> >> >> -	ret = snp_rmptable_psmash(pfn);
> >> >> -	if (ret) {
> >> >> -		/*
> >> >> -		 * Look it up again. If it's 4K now then the PSMASH may have
> >> >> -		 * raced with another process and the issue has already resolved
> >> >> -		 * itself. If it's not assigned, then this must have raced with
> >> >> -		 * another process that made this page shared.
> >> >> -		 */
> >> >> -		if (!snp_lookup_rmpentry(pfn, &assigned, &rmp_level) &&
> >> >> -		    ((assigned && rmp_level == PG_LEVEL_4K) || !assigned))
> >> >> +	scoped_guard(read_lock, &kvm->mmu_lock) {
> >> >> +		if (mmu_invalidate_retry_gfn(kvm, mmu_seq, gfn))
> >> >>  			goto out;
> >> >>
> >> >> -		pr_warn_ratelimited("SEV: Unable to split RMP entry for GPA 0x%llx PFN 0x%llx ret %d\n",
> >> >> -				    gpa, pfn, ret);
> >> >> +		ret = snp_rmptable_psmash(pfn);
> >> >> +		if (ret) {
> >> >> +			/*
> >> >> +			 * Look it up again. If it's 4K now then the PSMASH may
> >> >> +			 * have raced with another process and the issue has
> >> >> +			 * already resolved itself. If it's not assigned, then
> >> >> +			 * this must have raced with another process that made
> >> >> +			 * this page shared.
> >> >> +			 */
> >> >> +			if (!snp_lookup_rmpentry(pfn, &assigned, &rmp_level) &&
> >> >> +			    ((assigned && rmp_level == PG_LEVEL_4K) || !assigned))
> >> >> +				goto out;
> >> >> +
> >> >> +			pr_warn_ratelimited("SEV: Unable to split RMP entry for GPA 0x%llx PFN 0x%llx ret %d\n",
> >> >> +					    gpa, pfn, ret);
> >> >> +		}
> >> >>  	}
> >> >>
> >> >>  	kvm_zap_gfn_range(kvm, gfn, gfn + PTRS_PER_PMD);
> >> >>  out:
> >> >>  	trace_kvm_rmp_fault(vcpu, gpa, pfn, error_code, rmp_level, ret);
> >> >> -out_no_trace:
> >> >> -	kvm_release_page_unused(page);
> >> >>  }
> >> >>
> >> >>  static bool is_pfn_range_shared(kvm_pfn_t start, kvm_pfn_t end)
> >> >>
> >> >> --
> >> >> 2.55.0.699.gb54405d56f-goog
> >> >>