Re: [PATCH v13 03/26] KVM: x86: Replace nr_vcpus_matched_tsc count with all_vcpus_matched_tsc bool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KVM: x86: race clearing matched TSC state in
  kvm_arch_vcpu_postcreate()
--

commit 0772c4b5400b34e15b8efeba1e351501ea33af4a
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Replace nr_vcpus_matched_tsc count with all_vcpus_matched_tsc bool

This commit replaces read-time comparisons of nr_vcpus_matched_tsc against
online_vcpus with a boolean all_vcpus_matched_tsc maintained under
tsc_write_lock, initializing it to true at VM creation and clearing it in
kvm_arch_vcpu_postcreate() before synchronizing the new vCPU.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 12f465040efc2..1bb0136042a65 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -9584,8 +9589,38 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  {
> -	if (mutex_lock_killable(&vcpu->mutex))
> +	struct kvm_arch *ka = &vcpu->kvm->arch;
> +	unsigned long flags;
> +
> +	/*
> +	 * The new vCPU is already counted in online_vcpus but has not
> +	 * been synchronized. Clear the cached matched state before
> +	 * trying the mutex: the old read-time comparison failed
> +	 * conservatively false throughout this window, and userspace
> +	 * can hold it open across a whole first KVM_RUN by taking
> +	 * vcpu->mutex before we do. The synchronization below (or any
> +	 * racing host-initiated TSC write on the already-live fd)
> +	 * re-latches the flags. A protected guest TSC never
> +	 * participates in offset matching, so for it there is nothing
> +	 * to clear — and no later sync to re-latch.
> +	 */
> +	if (!vcpu->arch.guest_tsc_protected) {
> +		raw_spin_lock_irqsave(&ka->tsc_write_lock, flags);
> +		WRITE_ONCE(ka->all_vcpus_matched_tsc, false);
> +		raw_spin_unlock_irqrestore(&ka->tsc_write_lock, flags);
> +	}
> +
> +	if (mutex_lock_killable(&vcpu->mutex)) {
> +		/*
> +		 * Aborted by a fatal signal: the synchronization never
> +		 * ran. Propagate whatever the matched state now is (a
> +		 * racing TSC write may have re-latched it), so the
> +		 * master clock does not stay in its previous mode with
> +		 * running siblings keeping PVCLOCK_TSC_STABLE_BIT.
> +		 */
> +		kvm_update_masterclock(vcpu->kvm);
> +		return;
> +	}

[Severity: Medium]
Can clearing ka->all_vcpus_matched_tsc here race with vCPU ioctls on the
newly created fd, or leave ka->use_master_clock stale while waiting for
vcpu->mutex?

In kvm_vm_ioctl_create_vcpu(), create_vcpu_fd(), atomic_inc(&kvm->online_vcpus),
and mutex_unlock(&vcpu->mutex) all run before kvm_arch_vcpu_postcreate():

CPU0                                    CPU1
kvm_vm_ioctl_create_vcpu()
  create_vcpu_fd(vcpu)
  atomic_inc(&kvm->online_vcpus)
  mutex_unlock(&vcpu->mutex)
                                        kvm_vcpu_ioctl(KVM_SET_MSRS)
                                          mutex_lock(&vcpu->mutex)
                                          ...
                                          __kvm_synchronize_tsc()
                                            ka->nr_vcpus_matched_tsc++
                                            vcpu->arch.this_tsc_generation =
                                              ka->cur_tsc_generation
                                            ka->all_vcpus_matched_tsc = true
  kvm_arch_vcpu_postcreate()
    raw_spin_lock_irqsave(&ka->tsc_write_lock)
    WRITE_ONCE(ka->all_vcpus_matched_tsc, false) <-- clobbers matched state
    raw_spin_unlock_irqrestore(&ka->tsc_write_lock)
    mutex_lock_killable(&vcpu->mutex)   <-- aborted by fatal signal
    kvm_update_masterclock()            <-- disables masterclock permanently

Because vcpu->arch.this_tsc_generation already matches
ka->cur_tsc_generation after CPU1's write, even if mutex_lock_killable()
succeeds, a subsequent matching sync in postcreate will not increment
nr_vcpus_matched_tsc again (and in the later commit "KVM: x86: Allow KVM
master clock mode when TSCs are offset from each other", a non-matching
offset write on CPU1 followed by the clear on CPU0 leaves
ka->all_vcpus_matched_freq false).

Additionally, when another thread takes vcpu->mutex for KVM_RUN before
postcreate does, clearing ka->all_vcpus_matched_tsc without calling
kvm_update_masterclock() leaves ka->use_master_clock true across that
entire first KVM_RUN (and even if CPU1 runs kvm_guest_time_update() before
CPU0 acquires tsc_write_lock, both ka->all_vcpus_matched_tsc and
ka->use_master_clock are still true).

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