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

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Dropping the page refcount early introduces a race condition where a concurrent `guest_memfd` invalidation can trigger a false-positive 'Unexpected RMP fault' kernel warning.
> --
>
> --- Patch [2]: [PATCH v2 2/4] KVM: SEV: Drop page refcount early during RMP fault handling ---
> commit 7d0f356d8592ce1804052b0fdb61fccf3a11ecaa
> Author: Sean Christopherson <[email protected]>
>
> KVM: SEV: Drop page refcount early during RMP fault handling
>
> When handling an RMP fault, KVM retrieves the PFN for a private GPA from
> guest_memfd. This patch drops the page reference immediately after retrieving
> the PFN instead of holding it across the entire handler, and adopts the MMU
> invalidation protocol to guard committing anything based on the PFN.
>
>> 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
>
> [ ... ]
>
>> @@ -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;
>>  	}
>
> [Severity: Medium]
> Since the page reference is dropped before this lookup in
> sev_handle_rmp_fault(), could a concurrent guest_memfd hole-punch drop
> the file's reference to the page and free it?
>
> If the page is freed, the RMP entry would transition to shared/unassigned.
> Could this then cause snp_lookup_rmpentry() to see the page as unassigned
> and incorrectly emit this warning before the mmu_invalidate_retry_gfn()
> check happens?
>

Yes, this can happen. I'll also check for invalidations before
warning in the next revision.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2