Re: [PATCH v12 14/24] KVM: x86: Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: x86: stale masterclock state read in
  kvm_vcpu_ioctl_get_clock_guest()
- [Medium] KVM: x86: mismatched TSC scaling in KVM_[GS]ET_CLOCK_GUEST
  for protected TSC
--

commit c242a430a092692493307282dff57c9d0a945477
Author: Jack Allister <jalliste@amazon.com>

KVM: x86: Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration

This commit adds the KVM_GET_CLOCK_GUEST and KVM_SET_CLOCK_GUEST vCPU ioctls
and KVM_CAP_CLOCK_GUEST capability to save and restore the guest pvclock
relationship to the guest TSC across migration.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4afafe7ed8b8a..00e6b2bdaffb5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -3674,6 +3677,197 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +#ifdef CONFIG_X86_64
> +static int kvm_vcpu_ioctl_get_clock_guest(struct kvm_vcpu *v, void __user *argp)
> +{
> +	struct pvclock_vcpu_time_info hv_clock = {};
> +	struct kvm_arch *ka = &v->kvm->arch;
> +	bool use_master_clock;
> +	unsigned int seq;
> +
> +	/*
> +	 * Construct the pvclock purely from the master clock state. The
> +	 * master mul/shift are computed for the guest TSC frequency, which
> +	 * in master clock mode is the frequency of every vCPU; only the
> +	 * tsc_timestamp is per-vCPU, translating the master snapshot into
> +	 * this vCPU's TSC domain via its scaling ratio and offset.
> +	 *
> +	 * Note, this deliberately does NOT invoke kvm_guest_time_update(),
> +	 * which would write the guest's pvclock pages: dirtying guest
> +	 * memory after the final KVM_RUN would break post-copy migration
> +	 * flows. The pvclock returned here describes the same clock as the
> +	 * guest-visible copy (the same linear function of the guest TSC),
> +	 * anchored at the current master clock snapshot.
> +	 *
> +	 * Translating the snapshot through the vCPU's *current* scaling
> +	 * ratio and offset is correct even if they changed after the
> +	 * snapshot was taken: the snapshot anchors a host TSC value, not
> +	 * a guest one. A frequency change whose masterclock update is
> +	 * still pending does leave stale mul/shift here, but that is the
> +	 * paused-VM migration path reading a frequency userspace itself
> +	 * has just changed — and the result fails safe, as the same-vCPU
> +	 * KVM_SET_CLOCK_GUEST on the destination rejects a mul/shift
> +	 * inconsistent with the vCPU's frequency with -ERANGE.
> +	 */
> +	do {
> +		seq = read_seqcount_begin(&ka->pvclock_sc);
> +
> +		use_master_clock = ka->use_master_clock;
> +		if (!use_master_clock)
> +			continue;
> +
> +		hv_clock.tsc_timestamp = kvm_read_l1_tsc(v, ka->master_cycle_now);
> +		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
> +		hv_clock.tsc_shift = ka->master_tsc_shift;
> +		hv_clock.tsc_to_system_mul = ka->master_tsc_mul;
> +		hv_clock.flags = READ_ONCE(ka->all_vcpus_matched_tsc) ?
> +				 PVCLOCK_TSC_STABLE_BIT : 0;
> +	} while (read_seqcount_retry(&ka->pvclock_sc, seq));

[Severity: High]
Can kvm_vcpu_ioctl_get_clock_guest() return stale or corrupted pvclock data
when a KVM_REQ_MASTERCLOCK_UPDATE is pending?

Events that invalidate or change the master clock (such as a host suspend
and resume with a backwards TSC, a host clocksource change away from TSC, or
a per-vCPU KVM_SET_TSC_KHZ) only queue KVM_REQ_MASTERCLOCK_UPDATE on vCPUs
without updating ka->use_master_clock, ka->master_cycle_now, or
ka->master_tsc_mul and ka->master_tsc_shift until vcpu_enter_guest() runs.

If the host suspends and resumes while the VM is paused,
kvm_arch_enable_virtualization_cpu() sets ka->backwards_tsc_observed, adds
vcpu->arch.tsc_offset_adjustment, and requests KVM_REQ_MASTERCLOCK_UPDATE.
When userspace then calls KVM_GET_CLOCK_GUEST, vcpu_load() ->
kvm_arch_vcpu_load() applies tsc_offset_adjustment to v->arch.l1_tsc_offset
before kvm_vcpu_ioctl_get_clock_guest() runs:

arch/x86/kvm/x86.c:kvm_arch_vcpu_load() {
	...
	if (unlikely(vcpu->arch.tsc_offset_adjustment)) {
		adjust_tsc_offset_host(vcpu, vcpu->arch.tsc_offset_adjustment);
		vcpu->arch.tsc_offset_adjustment = 0;
		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
	}
	...
}

kvm_vcpu_ioctl_get_clock_guest() still sees ka->use_master_clock == true,
combines the post-resume l1_tsc_offset with the pre-suspend
ka->master_cycle_now, and succeeds instead of returning -ENODATA.  Because
ka->master_tsc_mul and ka->master_tsc_shift still match the vCPU's TSC
frequency, KVM_SET_CLOCK_GUEST does not reject the state with -ERANGE and
instead corrupts ka->kvmclock_offset.  Similarly, if KVM_SET_TSC_KHZ changes
one vCPU's frequency in a multi-vCPU VM, ka->all_vcpus_matched_freq becomes
false, but calling KVM_GET_CLOCK_GUEST on an unmodified vCPU sees stale
ka->use_master_clock == true with a mul/shift that still matches that vCPU.

Calling kvm_update_masterclock() when needed would refresh the host-side
master clock state in struct kvm_arch without calling
kvm_guest_time_update() or dirtying guest pvclock pages.

[ ... ]

> +	/*
> +	 * Nonzero, or pvclock_update_vm_gtod_copy() — which read it on
> +	 * this CPU, with interrupts still disabled — would have cleared
> +	 * use_master_clock above.
> +	 */
> +	curr_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
> +
> +	if (kvm_caps.has_tsc_control)
> +		curr_tsc_hz = kvm_scale_tsc(curr_tsc_hz,
> +					    v->arch.l1_tsc_scaling_ratio);
> +
> +	/*
> +	 * The mul/shift in the provided pvclock structure encode the guest
> +	 * TSC frequency at which it was generated. Sanity-check that it is
> +	 * consistent with this vCPU's effective TSC frequency, allowing a
> +	 * discrepancy of 1 kHz either way since independently calibrated
> +	 * hosts will not measure precisely the same value even for the
> +	 * same nominal frequency. Compare in kHz: the reconstruction of
> +	 * each side's frequency in Hz carries a few Hz of rounding, which
> +	 * must not push two hosts calibrated exactly 1 kHz apart over the
> +	 * line.
> +	 */
> +	user_tsc_khz = DIV_ROUND_CLOSEST_ULL(user_tsc_hz, HZ_PER_KHZ);
> +	curr_tsc_khz = DIV_ROUND_CLOSEST_ULL(curr_tsc_hz, HZ_PER_KHZ);
> +	if (user_tsc_khz + 1 < curr_tsc_khz ||
> +	    user_tsc_khz > curr_tsc_khz + 1) {
> +		rc = -ERANGE;
> +		goto out;
> +	}

[Severity: Medium]
Does KVM_GET_CLOCK_GUEST return an unscaled mul/shift that
KVM_SET_CLOCK_GUEST rejects with -ERANGE when a protected-TSC guest uses a
non-default TSC frequency?

For guests with vcpu->arch.guest_tsc_protected set (such as TDX or SEV-SNP
with Secure TSC), userspace configures the guest TSC frequency via the VM
KVM_SET_TSC_KHZ ioctl before creating vCPUs, which sets
kvm->arch.default_tsc_khz without updating kvm->arch.cur_tsc_scaling_ratio.
During KVM_CREATE_VCPU, kvm_arch_vcpu_create() sets
vcpu->arch.l1_tsc_scaling_ratio from default_tsc_khz, but
kvm_arch_vcpu_postcreate() -> kvm_synchronize_tsc() ->
__kvm_synchronize_tsc() returns early before updating
kvm->arch.cur_tsc_scaling_ratio:

arch/x86/kvm/x86.c:__kvm_synchronize_tsc() {
	...
	if (vcpu->arch.guest_tsc_protected)
		return;
	...
	kvm->arch.cur_tsc_scaling_ratio = vcpu->arch.l1_tsc_scaling_ratio;
	...
}

pvclock_update_vm_gtod_copy() then computes ka->master_tsc_shift and
ka->master_tsc_mul from ka->cur_tsc_scaling_ratio, which is still
kvm_caps.default_tsc_scaling_ratio.  As a result,
kvm_vcpu_ioctl_get_clock_guest() scales hv_clock.tsc_timestamp with
v->arch.l1_tsc_scaling_ratio via kvm_read_l1_tsc(), but copies the unscaled
ka->master_tsc_shift and ka->master_tsc_mul, and
kvm_vcpu_ioctl_set_clock_guest() rejects that structure with -ERANGE when
comparing against curr_tsc_hz scaled by v->arch.l1_tsc_scaling_ratio.

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