Re: [PATCH v3 3/8] KVM: SVM: Hold SRCU while reloading guest-owned VMSAs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: SVM: redundant nested srcu_read_lock() in __sev_snp_reload_vmsa()
--

commit c628b3f34be1a24c7bbd7ced36d3f7d15af73940
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: SVM: Hold SRCU while reloading guest-owned VMSAs

This commit acquires a KVM SRCU read lock in __sev_snp_reload_vmsa() around
the memslot and guest_memfd lookup through VMCB installation.

>     KVM: SVM: Hold SRCU while reloading guest-owned VMSAs
>     
>     Take a dedicated KVM SRCU read lock around the guest-owned VMSA memslot
>     and guest_memfd lookup.  This makes the reload path self-contained and
>     prevents it from racing a userspace memslot replacement.
>     
>     Keep the lock through VMCB installation, and release it on all lookup
>     failure paths.
>     
>     Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c0ae34871be90..5bfe23790ba46 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -4057,6 +4057,7 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
>  	gfn_t gfn = gpa_to_gfn(gpa);
>  	unsigned long mmu_seq;
>  	kvm_pfn_t pfn;
> +	int idx;
>  
>  	lockdep_assert_held(&svm->sev_es.snp_vmsa_mutex);
>  
> @@ -4088,9 +4089,10 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
>  	if (!VALID_PAGE(gpa))
>  		return;
>  
> -	slot = gfn_to_memslot(vcpu->kvm, gfn);
> +	idx = srcu_read_lock(&kvm->srcu);
> +	slot = gfn_to_memslot(kvm, gfn);
>  	if (!slot)
> -		return;
> +		goto out_unlock;
>  
>  	mmu_seq = kvm->mmu_invalidate_seq;
>  	smp_rmb();
> @@ -4099,8 +4101,8 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
>  	 * The new VMSA will be private memory guest memory, so retrieve the
>  	 * PFN from the gmem backend.
>  	 */
> -	if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, NULL))
> -		return;
> +	if (kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, NULL))
> +		goto out_unlock;
>  
>  	read_lock(&kvm->mmu_lock);
>  	/*
> @@ -4115,6 +4117,9 @@ 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);
> +
> +out_unlock:
> +	srcu_read_unlock(&kvm->srcu, idx);
>  }

[Severity: Medium]
Is taking a dedicated srcu_read_lock(&kvm->srcu) in __sev_snp_reload_vmsa()
necessary, and is the commit message accurate that this path previously raced
with userspace memslot replacement?

gfn_to_memslot() calls kvm_memslots() -> __kvm_memslots(), which already
requires kvm->srcu (or kvm->slots_lock) to be held:

include/linux/kvm_host.h:__kvm_memslots() {
	return srcu_dereference_check(kvm->memslots[as_id], &kvm->srcu,
			lockdep_is_held(&kvm->slots_lock) ||
			!refcount_read(&kvm->users_count));
}

Looking at all callers of __sev_snp_reload_vmsa(), kvm->srcu is already held
via kvm_vcpu_srcu_read_lock(vcpu):

1. sev_snp_reload_vmsa() is only called from vcpu_enter_guest() when
   servicing KVM_REQ_VMSA_PAGE_RELOAD:

arch/x86/kvm/x86.c:vcpu_enter_guest() {
		if (kvm_check_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, vcpu)) {
			kvm_vcpu_reset(vcpu, true);
			if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE) {
				r = 1;
				goto out;
			}
		}
		if (kvm_check_request(KVM_REQ_VMSA_PAGE_RELOAD, vcpu))
			kvm_x86_call(reload_vmsa)(vcpu);
}

2. sev_snp_init_protected_guest_state() is only called from
   sev_init_vmcb(svm, init_event) when init_event is true, which is reached
   via svm_vcpu_reset(vcpu, true) <- kvm_vcpu_reset(vcpu, true) from
   vcpu_enter_guest() (servicing KVM_REQ_UPDATE_PROTECTED_GUEST_STATE) and
   from kvm_apic_accept_events() (all callers of which hold kvm->srcu).

Even in the later commit 58d63c955c7c ("KVM: SEV: Add SNP vCPU state get and
set commands"), snp_set_vcpu_state() introduces its own helper,
sev_snp_install_guest_vmsa(), rather than calling __sev_snp_reload_vmsa().

Could this nested srcu_read_lock() and srcu_read_unlock() in
__sev_snp_reload_vmsa() be dropped, or replaced with an assertion that
kvm->srcu is held?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102114137.1198982-1-joro@8bytes.org?part=3