Re: [PATCH v7 24/36] KVM: x86: Use ktime_get_snapshot_id() for master clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Mathematically incorrect usage of `ktime_mono_to_any()` with a RAW timestamp.
- [High] Cannot retrieve CLOCK_MONOTONIC using only the standard system_time_snapshot.
- [High] The patch silently breaks KVM masterclock support on Hyper-V hosts.
- [High] Missing validation for zero cycles in kvm_snapshot_has_tsc leads to a massive guest time jump and potential DoS.
- [Low] The patch introduces a build failure by calling a non-existent kernel API and accessing non-existent struct fields.
- [Low] Static helper functions were abandoned but not removed, breaking the build.
--

--- Patch [24]: [PATCH v7 24/36] KVM: x86: Use ktime_get_snapshot_id() for master clock ---
commit 08cee26ad4312fb3cda5c262811876db0f33c933
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Use ktime_get_snapshot_id() for master clock

This commit replaces the KVM-private timekeeping reimplementation with calls 
to a generic ktime_get_snapshot_id interface. It is intended as a step towards 
eliminating the pvclock_gtod_data private copy of timekeeping state.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 57494812cc839..45a768fd7cb35 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1580,14 +1581,39 @@ static int do_realtime(struct timespec64 *ts, u64 *tsc_timestamp)

[Severity: Low]
The commit message mentions replacing do_kvmclock_base, do_monotonic, 
do_realtime, and vgettsc. Were these functions meant to be removed in this 
patch? Leaving them defined as static helper functions without any callers 
will break builds compiled with -Werror=unused-function.

>   * reports the TSC value from which it do so. Returns true if host is
>   * using TSC based clocksource.
>   */
> +static bool kvm_snapshot_has_tsc(struct system_time_snapshot *snap,
> +				u64 *tsc_timestamp)
> +{
> +	/*
> +	 * ktime_get_snapshot_id() cannot fail for standard clock IDs
> +	 * (only for invalid/aux clocks or during suspend, with a WARN).
> +	 */
> +	if (!snap->valid)

[Severity: Low]
Does the system_time_snapshot structure have a valid field? It appears this 
field, along with hw_csid and hw_cycles used below, might not exist in the 
mainline struct system_time_snapshot, which would cause a build failure.

> +		return false;
> +
> +	if (snap->cs_id == CSID_X86_TSC) {

[Severity: High]
Does this explicit check for CSID_X86_TSC exclude the previously supported 
Hyper-V clocksource? Since VDSO_CLOCKMODE_HVCLOCK defaults to CSID_GENERIC, 
it seems this might silently break KVM masterclock support for nested VMs 
running on Hyper-V hosts.

[Severity: High]
If the hardware read transiently fails (such as during host migration) and 
snap->cycles is 0, this block unconditionally returns true and passes 0 as the 
host TSC reference. Could this result in a massive leap in guest time and 
cause a timer storm that soft-locks or crashes the guest VM? The commit 
message mentions raw_cycles being zero means the clocksource could not provide 
a raw counter value, but it does not appear to be handled here.

> +		*tsc_timestamp = snap->cycles;
> +		return true;
> +	}
> +
> +	if (snap->hw_csid == CSID_X86_TSC && snap->hw_cycles) {
> +		*tsc_timestamp = snap->hw_cycles;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *tsc_timestamp)
>  {
> -	/* checked again under seqlock below */
> -	if (!gtod_is_based_on_tsc(pvclock_gtod_data.clock.vclock_mode))
> +	struct system_time_snapshot snap = {};
> +
> +	ktime_get_snapshot_id(CLOCK_MONOTONIC_RAW, &snap);

[Severity: Low]
Is ktime_get_snapshot_id available in the generic timekeeping code? Calling a 
non-existent API will result in a build failure.

> +	if (!kvm_snapshot_has_tsc(&snap, tsc_timestamp))
>  		return false;
>  
> -	return gtod_is_based_on_tsc(do_kvmclock_base(kernel_ns,
> -						     tsc_timestamp));
> +	*kernel_ns = ktime_to_ns(ktime_mono_to_any(snap.systime, TK_OFFS_BOOT));

[Severity: High]
Does passing a RAW timestamp (from CLOCK_MONOTONIC_RAW) to ktime_mono_to_any 
mathematically mix two different time domains? It looks like this adds the 
monotonic-to-boot offset to a raw monotonic timestamp, which could corrupt 
the guest masterclock base time.

Additionally, does snap.systime exist in struct system_time_snapshot, or is 
this another missing field that will break the build?

> +	return true;
>  }
>  
>  /*
> @@ -1596,12 +1622,14 @@ static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *tsc_timestamp)
>   */
>  bool kvm_get_monotonic_and_clockread(s64 *kernel_ns, u64 *tsc_timestamp)
>  {
> -	/* checked again under seqlock below */
> -	if (!gtod_is_based_on_tsc(pvclock_gtod_data.clock.vclock_mode))
> +	struct system_time_snapshot snap = {};
> +
> +	ktime_get_snapshot_id(CLOCK_MONOTONIC, &snap);
> +	if (!kvm_snapshot_has_tsc(&snap, tsc_timestamp))
>  		return false;
>  
> -	return gtod_is_based_on_tsc(do_monotonic(kernel_ns,
> -						 tsc_timestamp));
> +	*kernel_ns = ktime_to_ns(snap.systime);

[Severity: High]
Can CLOCK_MONOTONIC time actually be retrieved using only the standard 
system_time_snapshot? The standard struct captures real, boot, and raw times, 
but lacks the timekeeper offsets needed to properly compute monotonic time. 
KVM guest timekeeping requests might return corrupt data if relying solely on 
this structure without proper offsets.

> +	return true;
>  }

[ ... ]

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