Re: [PATCH v8 13/17] KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing `local_irq_save()` with `preempt_disable()` allows hardware interrupts to break the temporal atomicity of clock reads, leading to guest time corruption.
--

--- Patch [13]: [PATCH v8 13/17] KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair ---
commit a5cd5356feb83302b6464039e18ebbb2f4d759f9
Author: Sean Christopherson <[email protected]>

KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

This commit replaces local_irq_save() with preempt_disable() in
kvm_guest_time_update() when reading the TSC and frequency pair. The
intention is to avoid overkill IRQ disabling, as preemption disabling is
sufficient to prevent task migration across CPUs during get_cpu_tsc_khz().

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fa00799ba13da..e0e8c6883b831 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1805,11 +1804,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]
Could replacing local_irq_save() with preempt_disable() allow hardware
interrupts to break the temporal atomicity of the fallback clock reads?

If KVM falls back to the non-master clock path just below this code,
host_tsc and kernel_ns are read sequentially:

        if (!use_master_clock) {
                host_tsc = rdtsc();
                kernel_ns = get_kvmclock_base_ns();
        }

If an interrupt fires between these two reads, it would cause kernel_ns
to be sampled later than host_tsc.

Could this inflated kernel_ns then falsely trigger a catch-up adjustment
later in kvm_guest_time_update():

        if (vcpu->tsc_catchup) {
                u64 tsc = compute_guest_tsc(v, kernel_ns);
                if (tsc > tsc_timestamp) {
                        adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
                        tsc_timestamp = tsc;
                }
        }

and permanently advance the guest's TSC offset or cause temporary time freezes?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13