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

[Ackerley Tng]

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);

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
>>