Re: [PATCH 3/3] KVM: x86: hyper-v: Implement HvCallRestorePartitionTime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] x86/kvm: Race condition in kvm_vcpu_apply_tsc_generation() causes state corruption
- [High] x86/kvm: Pre-mature matched TSC count increment permits double-counting
- [Medium] x86/kvm: VM-wide cur_tsc_offset overwritten in per-vCPU loop
--

commit 3619ce01761243ff0d6db55e73b6eefb9f3cc10b
Author: Mushahid Hussain <hmushi@amazon.co.uk>
Subject: KVM: x86: hyper-v: Implement HvCallRestorePartitionTime

A nested Hyper-V guest can resume from hibernation and use
HvCallRestorePartitionTime to sync its TSC and reference counter. This commit
implements handling for the hypercall in KVM, stepping both clocks in a
single update. It updates kvm_set_clock_and_tsc() and issues requests to
write the TSC offset for each vCPU.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 19666a80240a5..c0865468b02e7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -4354,13 +4354,85 @@ static int kvm_vm_ioctl_get_clock(struct kvm *kvm, void __user *argp)
>  	return 0;
>  }
>  
> -static void __kvm_set_clock(struct kvm *kvm, struct kvm_clock_data *data)
> +/* Must precede pvclock_update_vm_gtod_copy(), which reads the matched count. */
> +static void kvm_open_tsc_generation(struct kvm *kvm, u64 guest_tsc)
> +{
> +	struct kvm_arch *ka = &kvm->arch;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
> +	lockdep_assert_held(&ka->tsc_write_lock);
> +
> +	ka->cur_tsc_generation++;
> +	ka->cur_tsc_write = guest_tsc;
> +	ka->last_tsc_write = guest_tsc;
> +	ka->nr_vcpus_matched_tsc = atomic_read(&kvm->online_vcpus) - 1;

[Severity: High]
Is it safe to pre-increment the matched count globally before the per-vCPU
updates complete?

If a vCPU executes kvm_synchronize_tsc() before processing the asynchronous
KVM_REQ_WRITE_TSC_OFFSET, it might see a stale generation, match it, and
increment ka->nr_vcpus_matched_tsc.

Since the count is pre-incremented here, double-counting could cause
the total to reach or exceed online_vcpus, permanently breaking the KVM
masterclock validation (which checks if matched_tsc + 1 == online_vcpus).

> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (vcpu->arch.guest_tsc_protected)
> +			continue;
> +		ka->last_tsc_khz = vcpu->arch.virtual_tsc_khz;
> +		ka->last_tsc_scaling_ratio = vcpu->arch.l1_tsc_scaling_ratio;
> +		break;
> +	}
> +}
> +
> +/* Every vCPU reads @guest_tsc at host TSC @host_tsc. */
> +static void kvm_set_tsc_generation(struct kvm *kvm, u64 host_tsc,
> +				   u64 guest_tsc, u64 ns)
> +{
> +	struct kvm_arch *ka = &kvm->arch;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
> +
> +	lockdep_assert_held(&ka->tsc_write_lock);
> +
> +	ka->cur_tsc_nsec = ns;
> +	ka->last_tsc_nsec = ns;
> +	ka->restore_host_tsc = host_tsc;
> +	ka->restore_guest_tsc = guest_tsc;
> +	ka->restore_tsc_nsec = ns;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (vcpu->arch.guest_tsc_protected)
> +			continue;
> +
> +		ka->cur_tsc_offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc,
> +							       guest_tsc);
> +		ka->last_tsc_offset = ka->cur_tsc_offset;

[Severity: Medium]
Could this loop repeatedly overwrite the VM-wide offset, leaving it configured
for the last vCPU?

If vCPUs have different TSC scaling ratios (which can be set via
KVM_SET_TSC_KHZ), the reference frequency is bound to the first valid vCPU in
kvm_open_tsc_generation() above, while the offset here ends up bound to the
last vCPU, breaking consistency.

> +		kvm_make_request(KVM_REQ_WRITE_TSC_OFFSET, vcpu);
> +	}
> +}
> +
> +/* Runs on the vCPU; only the owner writes its TSC offset. */
> +static void kvm_vcpu_apply_tsc_generation(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_arch *ka = &vcpu->kvm->arch;
> +	unsigned long flags;
> +	u64 offset;
> +
> +	raw_spin_lock_irqsave(&ka->tsc_write_lock, flags);
> +	offset = kvm_compute_l1_tsc_offset(vcpu, ka->restore_host_tsc,
> +					   ka->restore_guest_tsc);
> +	vcpu->arch.last_guest_tsc = ka->restore_guest_tsc;
> +	vcpu->arch.this_tsc_generation = ka->cur_tsc_generation;

[Severity: High]
Could a race condition corrupt state tracking here?

If a concurrent write to MSR_IA32_TSC bumps ka->cur_tsc_generation while
KVM_REQ_WRITE_TSC_OFFSET is queued, the vCPU processing the request will copy
the newer generation counter here while applying the stale ka->restore_*
offset data.

This would leave the vCPU claiming synchronization with the new generation
while actually holding old data, breaking KVM masterclock and TSC
synchronization.

> +	vcpu->arch.this_tsc_nsec = ka->restore_tsc_nsec;
> +	vcpu->arch.this_tsc_write = ka->restore_guest_tsc;
> +	raw_spin_unlock_irqrestore(&ka->tsc_write_lock, flags);
> +
> +	kvm_vcpu_write_tsc_offset(vcpu, offset);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005192431.87317-1-hmushi@amazon.co.uk?part=3