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

Michael Roth <[email protected]> writes:

> On Thu, Aug 20, 2026 at 03:35:31PM -0700, Ackerley Tng wrote:
>> Michael Roth <[email protected]> writes:
>>
>> > On Thu, Aug 20, 2026 at 07:58:19AM -0700, Ackerley Tng wrote:
>> >> Michael Roth <[email protected]> writes:
>> >>
>> >> > On Tue, Aug 18, 2026 at 09:15:53AM +0000, Ackerley Tng wrote:
>> >> >> From: Sean Christopherson <[email protected]>
>> >> >>
>> >> >> 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.
>

Thanks, I adjusted the commit message to focus on drop refcount + adopt
KVM MMU invalidation protocol in v3, please see v3! Thanks!

> 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 <[email protected]>
>> >> >
>> >> >>
>> >> >> Hence, adopt the MMU invalidation protocol to guard committing anything
>> >> >> based on the PFN.
>> >> >>
>> >> >> Signed-off-by: Sean Christopherson <[email protected]>
>> >> >> Signed-off-by: Ackerley Tng <[email protected]>
>> >> >> ---
>> >> >>  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
>> >> >>