Re: [PATCH v11 14/23] 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 6 potential issue(s) to consider:
- [High] KVM: x86: stale masterclock state in kvm_vcpu_ioctl_get_clock_guest()
- [High] KVM: x86: u64 underflow and overflow in kvm_vcpu_ioctl_set_clock_guest()
- [High] KVM: x86: unsigned TSC wraparound check in kvm_vcpu_ioctl_set_clock_guest()
- [High] KVM: x86: premature all_vcpus_matched_freq in kvm_track_tsc_matching()
- [Medium] KVM: x86: unreachable -EBUSY path in kvm_vcpu_ioctl_set_clock_guest()
- [Medium] KVM: x86: early return inside pvclock_sc seqcount read section in kvm_vcpu_ioctl_get_clock_guest()
--

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

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

This commit adds the per-vCPU KVM_GET_CLOCK_GUEST and KVM_SET_CLOCK_GUEST
ioctls (advertised via KVM_CAP_CLOCK_GUEST) to save and restore the
pvclock_vcpu_time_info structure in terms of the guest vCPU TSC when the VM
is using the master clock.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6dbdb60962fe7..bdc838b12fdc0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -3541,6 +3544,168 @@ 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;
> +	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.
> +	 */
> +	do {
> +		seq = read_seqcount_begin(&ka->pvclock_sc);
> +
> +		if (!ka->use_master_clock)
> +			return -ENODATA;

[Severity: Medium]
Is it safe to return directly from inside the read_seqcount_begin() loop
when !ka->use_master_clock without calling read_seqcount_retry()?

Returning early here skips the pairing smp_rmb() and sequence validation on
ka->use_master_clock (which pvclock_update_vm_gtod_copy() updates inside
write_seqcount_begin/end and may transiently set to true before clearing it
if tsc_hz is 0), and leaves KCSAN's seqlock atomic region state active on
current because do___read_seqcount_retry() -> kcsan_atomic_next(0) is never
reached.

For comparison, kvm_guest_time_update() in arch/x86/kvm/x86.c completes the
seqcount loop before checking use_master_clock:

	do {
		seq = read_seqcount_begin(&ka->pvclock_sc);
		use_master_clock = ka->use_master_clock;
		if (!use_master_clock)
			continue;
		...
	} while (read_seqcount_retry(&ka->pvclock_sc, seq));

> +
> +		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 a corrupt pvclock_vcpu_time_info
structure when KVM_REQ_MASTERCLOCK_UPDATE is pending?

Operations that change vCPU TSC frequency, TSC offset, or masterclock
eligibility — such as KVM_SET_TSC_KHZ via kvm_set_tsc_khz() ->
kvm_synchronize_tsc() -> __kvm_synchronize_tsc() -> kvm_track_tsc_matching(),
host-initiated MSR_IA32_TSC writes, KVM_VCPU_TSC_OFFSET via
kvm_arch_tsc_set_attr(), host clocksource updates via
pvclock_gtod_update_fn(), or backwards TSC detection — immediately update
v->arch.l1_tsc_scaling_ratio, v->arch.l1_tsc_offset,
ka->cur_tsc_scaling_ratio, and ka->all_vcpus_matched_freq, but defer
refreshing ka->use_master_clock, ka->master_tsc_shift, ka->master_tsc_mul,
ka->master_cycle_now, and ka->master_kernel_ns by queuing
KVM_REQ_MASTERCLOCK_UPDATE:

arch/x86/kvm/x86.c:kvm_track_tsc_matching() {
	...
	if ((ka->use_master_clock && update_mclock) ||
	    (ka->use_master_clock != use_master_clock))
		kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
}

Because KVM_REQ_MASTERCLOCK_UPDATE is only serviced in vcpu_enter_guest()
during KVM_RUN (and kvm_track_tsc_matching() only queues it on the single
vCPU being updated rather than VM-wide), invoking KVM_GET_CLOCK_GUEST while
vCPUs are stopped in userspace reads stale ka->use_master_clock,
ka->master_tsc_shift, and ka->master_tsc_mul while combining them with the
updated v->arch.l1_tsc_scaling_ratio and v->arch.l1_tsc_offset in
kvm_read_l1_tsc(v, ka->master_cycle_now). This either mixes a newly scaled
tsc_timestamp with old master_tsc_mul/shift (causing KVM_SET_CLOCK_GUEST to
fail with -ERANGE) or returns 0 with stale clock data instead of -ENODATA
after masterclock eligibility was lost.

[Severity: High]
Also on this read path in kvm_vcpu_ioctl_get_clock_guest(), can
ka->use_master_clock remain true even when vCPUs have mismatched TSC
frequencies?

If userspace creates vCPU 0 (online_vcpus == 1) and sets its TSC frequency
via KVM_SET_TSC_KHZ to F1 != ka->default_tsc_khz, ka->cur_tsc_khz is F1 and
ka->all_vcpus_matched_freq is true.

When vCPU 1 is subsequently created in kvm_vm_ioctl_create_vcpu(),
kvm_arch_vcpu_create() runs before atomic_inc(&kvm->online_vcpus) (while
online_vcpus is still 1) and calls kvm_set_tsc_khz(vcpu1, default_tsc_khz) ->
kvm_synchronize_tsc() -> __kvm_synchronize_tsc():

	if (vcpu->arch.virtual_tsc_khz != kvm->arch.cur_tsc_khz) {
		kvm->arch.cur_tsc_freq_generation++;
		kvm->arch.all_vcpus_matched_freq = false;
		kvm->arch.nr_vcpus_matched_freq = 0;
	}

	/* Count each vCPU once per freq generation */
	if (vcpu->arch.this_tsc_freq_generation != kvm->arch.cur_tsc_freq_generation) {
		vcpu->arch.this_tsc_freq_generation = kvm->arch.cur_tsc_freq_generation;
		kvm->arch.nr_vcpus_matched_freq++;
	}

	kvm->arch.cur_tsc_khz = vcpu->arch.virtual_tsc_khz;

__kvm_synchronize_tsc() sees default_tsc_khz != F1, resets
nr_vcpus_matched_freq to 1, updates ka->cur_tsc_khz to default_tsc_khz, and
calls kvm_track_tsc_matching():

	if (ka->all_vcpus_matched_tsc ||
	    ka->nr_vcpus_matched_freq >= online)
		ka->all_vcpus_matched_freq = true;

Because online is still 1 during kvm_arch_vcpu_create(), 1 >= 1 is true and
ka->all_vcpus_matched_freq is latched back to true. When
kvm_arch_vcpu_postcreate() later runs after online_vcpus is incremented to 2,
vcpu1->arch.virtual_tsc_khz already matches ka->cur_tsc_khz
(default_tsc_khz), and kvm_track_tsc_matching() never clears
ka->all_vcpus_matched_freq when nr_vcpus_matched_freq < online.

As a result, ka->all_vcpus_matched_freq and ka->use_master_clock stay true
while vCPU 0 is at F1 and vCPU 1 is at default_tsc_khz, causing
kvm_vcpu_ioctl_get_clock_guest() on vCPU 0 to return a pvclock structure with
tsc_timestamp scaled for F1 and tsc_to_system_mul/tsc_shift computed for
default_tsc_khz instead of returning -ENODATA.

> +
> +	if (copy_to_user(argp, &hv_clock, sizeof(hv_clock)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +/*
> + * Reverse the calculation in the hv_clock definition.
> + *
> + * time_ns = ( (cycles << shift) * mul ) >> 32;
> + * (although shift can be negative, so that's bad C)
> + *
> + * So for a single second,
> + * NSEC_PER_SEC = ( ( FREQ_HZ << shift) * mul ) >> 32
> + * NSEC_PER_SEC << 32 = ( FREQ_HZ << shift ) * mul
> + * ( NSEC_PER_SEC << 32 ) / mul = FREQ_HZ << shift
> + * ( NSEC_PER_SEC << 32 ) / mul ) >> shift = FREQ_HZ
> + */
> +static u64 hvclock_to_hz(u32 mul, s8 shift)
> +{
> +	u64 tm = NSEC_PER_SEC << 32;
> +
> +	/* Maximise precision. Shift left until the top bit is set */
> +	tm <<= 2;
> +	shift += 2;
> +
> +	/* While 'mul' is even, increase the shift *after* the division */
> +	while (!(mul & 1)) {
> +		shift++;
> +		mul >>= 1;
> +	}
> +
> +	tm /= mul;
> +
> +	if (shift >= 64)
> +		return 0;
> +	if (shift > 0)
> +		return tm >> shift;
> +	if (shift <= -64)
> +		return 0;
> +	return tm << -shift;
> +}
> +
> +static int kvm_vcpu_ioctl_set_clock_guest(struct kvm_vcpu *v, void __user *argp)
> +{
> +	struct pvclock_vcpu_time_info user_hv_clock;
> +	struct kvm *kvm = v->kvm;
> +	struct kvm_arch *ka = &kvm->arch;
> +	u64 curr_tsc_hz, user_tsc_hz;
> +	u64 user_clk_ns;
> +	u64 guest_tsc;
> +	int rc = 0;
> +
> +	if (copy_from_user(&user_hv_clock, argp, sizeof(user_hv_clock)))
> +		return -EFAULT;
> +
> +	if (user_hv_clock.pad0 || user_hv_clock.pad[0] || user_hv_clock.pad[1])
> +		return -EINVAL;
> +
> +	if (!user_hv_clock.tsc_to_system_mul)
> +		return -EINVAL;
> +
> +	if (user_hv_clock.tsc_shift < -31 || user_hv_clock.tsc_shift > 31)
> +		return -EINVAL;
> +
> +	user_tsc_hz = hvclock_to_hz(user_hv_clock.tsc_to_system_mul,
> +				    user_hv_clock.tsc_shift);
> +
> +	kvm_hv_request_tsc_page_update(kvm);
> +
> +	/*
> +	 * kvm_start_pvclock_update() takes tsc_write_lock and opens
> +	 * the pvclock seqcount; kvm_end_pvclock_update() closes both.
> +	 * All clock state modifications between them are atomic with
> +	 * respect to readers in kvm_guest_time_update().
> +	 */
> +	kvm_start_pvclock_update(kvm);
> +	pvclock_update_vm_gtod_copy(kvm);
> +
> +	if (!ka->use_master_clock) {
> +		rc = -ENODATA;
> +		goto out;
> +	}
> +
> +	curr_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
> +	if (unlikely(curr_tsc_hz == 0)) {
> +		rc = -EBUSY;
> +		goto out;
> +	}

[Severity: Medium]
Can this curr_tsc_hz == 0 (-EBUSY) branch ever be reached?

kvm_start_pvclock_update() acquires raw_spin_lock_irq(&ka->tsc_write_lock),
disabling interrupts and preemption on the local CPU. Immediately before the
!ka->use_master_clock check, pvclock_update_vm_gtod_copy() reads
get_cpu_tsc_khz() on the same CPU and clears ka->use_master_clock if tsc_hz
is 0:

arch/x86/kvm/x86.c:pvclock_update_vm_gtod_copy() {
	...
		tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
		if (tsc_hz && kvm_caps.has_tsc_control)
			tsc_hz = kvm_scale_tsc(tsc_hz,
					       ka->master_tsc_scaling_ratio);
		if (tsc_hz)
			kvm_get_time_scale(NSEC_PER_SEC, tsc_hz,
					   &ka->master_tsc_shift,
					   &ka->master_tsc_mul);
		else
			ka->use_master_clock = false;
}

Because IRQs remain disabled across both calls, get_cpu_tsc_khz() == 0 will
always set ka->use_master_clock = false in pvclock_update_vm_gtod_copy(),
causing kvm_vcpu_ioctl_set_clock_guest() to take the !ka->use_master_clock
path and return -ENODATA rather than the documented -EBUSY.

> +
> +	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.
> +	 */
> +	if (user_tsc_hz < curr_tsc_hz - 1000 ||
> +	    user_tsc_hz > curr_tsc_hz + 1000) {
> +		rc = -ERANGE;
> +		goto out;
> +	}

[Severity: High]
Are there three boundary and overflow issues in the frequency calculation and
comparison between hvclock_to_hz() and kvm_vcpu_ioctl_set_clock_guest()?

First, if a vCPU's guest TSC frequency is set to 1 kHz via KVM_SET_TSC_KHZ,
set_tsc_khz() computes ratio = mul_u64_u32_div(1ULL << frac_bits, 1,
tsc_khz), which truncates down. Scaling the host TSC frequency in Hz via
kvm_scale_tsc() then yields curr_tsc_hz == 999 Hz (non-zero, so
ka->use_master_clock stays true). Because curr_tsc_hz is u64, evaluating
curr_tsc_hz - 1000 underflows to U64_MAX, making user_tsc_hz < curr_tsc_hz -
1000 unconditionally true and failing with -ERANGE.

Second, host tsc_khz is measured in whole kHz (1000 Hz steps), so a 1 kHz
calibration difference between two hosts is already 1000 Hz. Because
kvm_get_time_scale() floors tsc_to_system_mul via div_frac(), hvclock_to_hz()
rounds user_tsc_hz up by 1 to 3 Hz (for example, 2100001 kHz becomes
2100001001 Hz) while kvm_scale_tsc() can floor curr_tsc_hz by 1 Hz. Doesn't a
1 kHz host calibration difference therefore exceed +/- 1000 Hz by 1 to 3 Hz
and get rejected with -ERANGE?

Third, in hvclock_to_hz(), tm = (NSEC_PER_SEC << 34) / mul always has at
least bit 31 set (and bit 63 set when mul == 1). When shift < 0 (for example,
user_hv_clock.tsc_shift in [-31, -3]), won't tm << -shift overflow 64 bits
and wrap around modulo 2^64?

> +
> +	/*
> +	 * Calculate the guest TSC at the new reference point, and the
> +	 * corresponding KVM clock value according to user_hv_clock.
> +	 * Adjust kvmclock_offset so both definitions agree.
> +	 */
> +	guest_tsc = kvm_read_l1_tsc(v, ka->master_cycle_now);
> +	if (guest_tsc < user_hv_clock.tsc_timestamp) {
> +		rc = -EINVAL;
> +		goto out;
> +	}

[Severity: High]
Does the unsigned comparison guest_tsc < user_hv_clock.tsc_timestamp mishandle
64-bit modular TSC wraparound?

Guest TSC values from kvm_read_l1_tsc() and pvclock deltas in
__pvclock_read_cycles() (u64 delta = tsc - src->tsc_timestamp) use 64-bit
modular arithmetic. When a vCPU has a negative TSC offset, its guest TSC can
start near U64_MAX and wrap across 0 into small positive values as time
advances.

If user_hv_clock.tsc_timestamp was captured just before wrapping across 0
(near U64_MAX) and guest_tsc on the destination is just after 0, guest_tsc is
ahead of tsc_timestamp, but guest_tsc < user_hv_clock.tsc_timestamp evaluates
to true and rejects the call with -EINVAL. Conversely, if tsc_timestamp is a
small value just above 0 and guest_tsc is slightly behind it (wrapped below 0
to near U64_MAX), the unsigned check evaluates to false and passes a huge
delta (~U64_MAX) into __pvclock_read_cycles().

Should this check (s64)(guest_tsc - user_hv_clock.tsc_timestamp) < 0 instead?

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