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 <[email protected]> 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/[email protected]?part=3
