Re: [PATCH v10 14/21] KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing `local_irq_save()` with `preempt_disable()` allows interrupts to occur between reading `host_tsc` and `kernel_ns`, causing the guest clock to abruptly jump forward.
--

commit 905bf0112d20298ccaf2fcc14bf1871e4d77ff84
Author: Sean Christopherson <seanjc@google.com>
Subject: KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

This commit replaces local_irq_save() with preempt_disable() when reading
the TSC and frequency pair to update guest time. The commit message notes
that disabling IRQs is overkill for task migration protection.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1824,11 +1823,14 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
>  		}
>  	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
>  
> -	/* Keep irq disabled to prevent changes to the clock */
> -	local_irq_save(flags);
> +	/*
> +	 * Ensure reading the TSC+frequency pair is done on the same CPU.  When
> +	 * NOT using the master clock, the TSC frequency may vary between CPUs.
> +	 */
> +	preempt_disable();
>  	tgt_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
>  	if (unlikely(tgt_tsc_hz == 0)) {
> -		local_irq_restore(flags);
> +		preempt_enable();
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
>  		return 1;
>  	}

[Severity: High]
Does this replacement of local_irq_save() with preempt_disable() expose the
fallback clock path to an interrupt latency race?

Because preempt_disable() does not disable hardware IRQs, it appears an 
interrupt can now fire between reading host_tsc and kernel_ns:

kvm_guest_time_update() {
    ...
    if (!use_master_clock) {
        host_tsc = rdtsc();
        <-- hardware interrupt fires here
        kernel_ns = get_kvmclock_base_ns();
    }
    ...
}

If an interrupt occurs between these two reads, kernel_ns will be captured 
after the interrupt completes. Would this artificially inflate the kernel_ns 
value relative to the exact moment host_tsc was captured? 

It looks like this could cause the elapsed interrupt time to be double-counted 
(once in kernel_ns and again when the guest calculates elapsed time since 
host_tsc), causing the guest clock to abruptly jump forward by milliseconds.

While later patches in this series update the use_master_clock path to use
the atomic ktime_get_snapshot_id(), it seems this !use_master_clock fallback 
path remains exposed to this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826213303.914988-1-seanjc@google.com?part=14