Re: [PATCH v11 07/23] KVM: x86: Avoid redundant masterclock updates from multiple vCPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: x86: race in kvm_update_masterclock() on MCLOCK_INPROGRESS and TSC page
--

commit 568cc2c30a5c5633833d9fd527d1659f817b9d25
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Avoid redundant masterclock updates from multiple vCPUs

This commit avoids redundant masterclock updates when multiple vCPUs process
KVM_REQ_MASTERCLOCK_UPDATE by checking under tsc_write_lock whether the
requesting vCPU's request is still pending and clearing the request across
all vCPUs once a single updater proceeds.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e3618d82c099c..77bd3e4bfdcd7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1712,10 +1712,39 @@ static void kvm_make_mclock_inprogress_request(struct kvm *kvm)
>  	kvm_make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS);
>  }
>  
> -static void __kvm_start_pvclock_update(struct kvm *kvm)
> +static void kvm_clear_mclock_inprogress_request(struct kvm *kvm)
>  {
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +}
> +
> +static bool __kvm_start_pvclock_update(struct kvm *kvm, struct kvm_vcpu *requesting_vcpu)
> +{
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
>  	raw_spin_lock_irq(&kvm->arch.tsc_write_lock);
> +
> +	/*
> +	 * If another vCPU already did the update while we were waiting
> +	 * for the lock, our request will have been cleared. Bail out.
> +	 */
> +	if (requesting_vcpu &&
> +	    !kvm_test_request(KVM_REQ_MASTERCLOCK_UPDATE, requesting_vcpu)) {
> +		kvm_clear_mclock_inprogress_request(kvm);
> +		raw_spin_unlock_irq(&kvm->arch.tsc_write_lock);
> +		return false;
> +	}
> +
> +	/* The update is VM-wide; prevent other vCPUs from redoing it. */
> +	kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_clear_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
> +
>  	write_seqcount_begin(&kvm->arch.pvclock_sc);
> +	return true;
>  }
>  
>  static void kvm_start_pvclock_update(struct kvm *kvm)
> @@ -1723,7 +1752,7 @@ static void kvm_start_pvclock_update(struct kvm *kvm)
>  	kvm_make_mclock_inprogress_request(kvm);
>  
>  	/* no guest entries from this point */
> -	__kvm_start_pvclock_update(kvm);
> +	__kvm_start_pvclock_update(kvm, NULL);
>  }
>  
>  static void kvm_end_pvclock_update(struct kvm *kvm)
> @@ -1732,22 +1761,25 @@ static void kvm_end_pvclock_update(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	unsigned long i;
>  
> -	write_seqcount_end(&ka->pvclock_sc);
> -	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
>  
>  	/* guest entries allowed */
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
> +	kvm_clear_mclock_inprogress_request(kvm);
> +
> +	write_seqcount_end(&ka->pvclock_sc);
> +	raw_spin_unlock_irq(&ka->tsc_write_lock);
>  }
>  
> -static void kvm_update_masterclock(struct kvm *kvm)
> +static void kvm_update_masterclock(struct kvm *kvm, struct kvm_vcpu *vcpu)
>  {
>  	kvm_hv_request_tsc_page_update(kvm);
> -	kvm_start_pvclock_update(kvm);
> -	pvclock_update_vm_gtod_copy(kvm);
> -	kvm_end_pvclock_update(kvm);
> +	kvm_make_mclock_inprogress_request(kvm);
> +
> +	if (__kvm_start_pvclock_update(kvm, vcpu)) {
> +		pvclock_update_vm_gtod_copy(kvm);
> +		kvm_end_pvclock_update(kvm);
> +	}
>  }

[Severity: High]
Can calling kvm_hv_request_tsc_page_update() and
kvm_make_mclock_inprogress_request() in kvm_update_masterclock() before
checking whether KVM_REQ_MASTERCLOCK_UPDATE is still pending cause races on
KVM_REQ_MCLOCK_INPROGRESS and hv->hv_tsc_page_status?

In vcpu_enter_guest(), KVM_REQ_MASTERCLOCK_UPDATE is checked with the
non-clearing kvm_test_request() before calling kvm_update_masterclock().
When the first vCPU enters kvm_update_masterclock() and calls
kvm_make_mclock_inprogress_request(kvm), it kicks all other running vCPUs out
of guest mode before acquiring kvm->arch.tsc_write_lock and clearing
KVM_REQ_MASTERCLOCK_UPDATE. Those kicked vCPUs re-enter vcpu_enter_guest(),
still observe KVM_REQ_MASTERCLOCK_UPDATE set, and also call
kvm_update_masterclock(), broadcasting additional rounds of VM-wide
KVM_REQ_MCLOCK_INPROGRESS kicks before bailing out in
__kvm_start_pvclock_update().

When a losing vCPU (vcpu1) acquires tsc_write_lock in
__kvm_start_pvclock_update() after the winning vCPU (vcpu0) has finished,
!kvm_test_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu1) is true and the bail-out
path unconditionally clears KVM_REQ_MCLOCK_INPROGRESS on all vCPUs:

arch/x86/kvm/x86.c:__kvm_start_pvclock_update() {
	if (requesting_vcpu &&
	    !kvm_test_request(KVM_REQ_MASTERCLOCK_UPDATE, requesting_vcpu)) {
		kvm_clear_mclock_inprogress_request(kvm);
		raw_spin_unlock_irq(&kvm->arch.tsc_write_lock);
		return false;
	}
}

If another updater (such as kvm_vm_ioctl_set_clock() via
kvm_start_pvclock_update(), kvm_hyperv_tsc_notifier(),
kvm_arch_vcpu_postcreate(), or another vCPU handling a per-vCPU
KVM_REQ_MASTERCLOCK_UPDATE from kvm_synchronize_tsc(),
kvm_write_system_time(), or kvm_hv_set_msr_pw()) has already called
kvm_make_mclock_inprogress_request(kvm) and is waiting on tsc_write_lock (or
running hyperv_stop_tsc_emulation()), vcpu1's bail-out clears
KVM_REQ_MCLOCK_INPROGRESS across all vCPUs in the VM (and similarly
kvm_end_pvclock_update() clears KVM_REQ_MCLOCK_INPROGRESS even if another
caller has already set it while spinning on tsc_write_lock). Other vCPUs can
then enter IN_GUEST_MODE while the second update is in progress. Because
kvm_end_pvclock_update() sets KVM_REQ_CLOCK_UPDATE with kvm_make_request()
without kicking vCPUs, vCPUs that entered guest mode early will not be kicked
and can run with stale pvclock state.

Similarly, because kvm_hv_request_tsc_page_update(kvm) runs before
__kvm_start_pvclock_update() checks KVM_REQ_MASTERCLOCK_UPDATE, two races on
hv->hv_tsc_page_status can occur:

1. If vcpu0 and vcpu1 both call kvm_hv_request_tsc_page_update(kvm), vcpu0
completes the update and runs kvm_guest_time_update() ->
kvm_hv_setup_tsc_page() (setting hv->hv_tsc_page_status = HV_TSC_PAGE_SET),
and a new KVM_REQ_MASTERCLOCK_UPDATE is raised before vcpu1 acquires
tsc_write_lock, vcpu1 will see KVM_REQ_MASTERCLOCK_UPDATE set and perform the
second masterclock update without kvm_hv_request_tsc_page_update(kvm) being
called for it. In kvm_hv_setup_tsc_page(), HV_TSC_PAGE_SET causes an early
return, leaving the Hyper-V TSC page stale.

2. If no second update occurs, a lagging vcpu1 that runs
kvm_hv_request_tsc_page_update(kvm) after vcpu0 has already completed
kvm_hv_setup_tsc_page() resets hv_tsc_page_status from HV_TSC_PAGE_SET back
to HV_TSC_PAGE_HOST_CHANGED, bails out in __kvm_start_pvclock_update(), and
then rewrites tsc_sequence and the shared Hyper-V TSC page in
kvm_hv_setup_tsc_page() while other vCPUs are already running in guest mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105154955.2525419-1-dwmw2@infradead.org?part=7