Re: [PATCH v7 34/36] KVM: selftests: Add master clock offset test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test uses `__vcpu_ioctl` to query `KVM_GET_CLOCK_GUEST` but ignores its return value, potentially leading to the use of uninitialized stack memory.
- [Medium] The test's verification that the clock advanced by 1 hour is ineffective on machines with an uptime greater than 1 hour.
- [Medium] All vCPUs register the exact same guest physical address for their pvclock_vcpu_time_info structures.
--

--- Patch [34]: [PATCH v7 34/36] KVM: selftests: Add master clock offset test ---
commit 52f3f310d696d962b9db181f0b7e2661011f0cb2
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: selftests: Add master clock offset test

Verify that KVM master clock mode remains active when vCPUs have
different TSC offsets but the same frequency. Creates three vCPUs,
sets one to a different TSC value, and confirms KVM_CLOCK_HOST_TSC
and KVM_CLOCK_TSC_STABLE are set.

> diff --git a/tools/testing/selftests/kvm/x86/masterclock_offset_test.c b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c
> new file mode 100644
> index 0000000000000..88e2bd2edab51
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c

[ ... ]

> +static void guest_code(void)
> +{
> +	wrmsr(MSR_KVM_SYSTEM_TIME_NEW, KVMCLOCK_GPA | KVM_MSR_ENABLED);

[Severity: Medium]
Does this code instruct KVM to write to the identical GPA for all vCPUs?
Since guest_code() unconditionally hardcodes KVMCLOCK_GPA, KVM will
concurrently overwrite the same memory area with different vCPUs' time
information. Is this API contract violation intended?

> +	for (;;)
> +		GUEST_SYNC(0);
> +}

[ ... ]

> +	/* Get per-vCPU pvclock in order 0, 2, 1 */
> +	int order[] = {0, 2, 1};
> +	for (i = 0; i < 3; i++) {
> +		int idx = order[i];
> +		__vcpu_ioctl(vcpus[idx], KVM_GET_CLOCK_GUEST, &pvti[idx]);

[Severity: Medium]
Could this lead to the use of uninitialized stack memory? The return value
of __vcpu_ioctl() is ignored, so if KVM_GET_CLOCK_GUEST fails or is
unsupported on the host kernel, pvti[idx] will remain uninitialized.

> +		pr_info("vCPU %d: tsc_timestamp=%lu system_time=%lu "

[ ... ]

> +	/* Re-read pvclocks */
> +	for (i = 0; i < 3; i++)
> +		__vcpu_ioctl(vcpus[order[i]], KVM_GET_CLOCK_GUEST, &pvti[order[i]]);

[Severity: Medium]
Does this also ignore the return value of __vcpu_ioctl() when re-reading
pvclocks?

> +
> +	pr_info("After +1h: vCPU 0 system_time=%lu, vCPU 2 system_time=%lu\n",

[ ... ]

> +	/* Verify the clock actually moved by ~1 hour */
> +	TEST_ASSERT(clk0 > ONE_HOUR_NS,
> +		    "Clock should be > 1 hour after set, got %lu ns",
> +		    (unsigned long)clk0);

[Severity: Medium]
Will this assertion trivially pass on any machine with an uptime greater
than 1 hour? The test validates the absolute clock value rather than the
delta (for example, clk0 >= clk0_before + ONE_HOUR_NS). If KVM_SET_CLOCK
completely fails to advance the clock, wouldn't the assertion still pass
on hosts with sufficient uptime?

> +
> +	pr_info("PASSED: pvclock still consistent after KVM_SET_CLOCK +1h\n");
> +	kvm_vm_free(vm);
> +	return 0;
> +}

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