Re: [PATCH v5 22/34] KVM: selftests: Add master clock offset test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The test uses an undefined, hallucinated ioctl macro `KVM_GET_CLOCK_GUEST` and fails to check the return value of `__vcpu_ioctl`, leading to compilation errors and potential use of uninitialized stack memory.
- [Medium] The test incorrectly assumes KVM master clock mode remains active when vCPUs have different TSC offsets.
- [Medium] The test code's assertion for `KVM_CLOCK_TSC_STABLE` directly contradicts the commit message.
- [Medium] The test incorrectly expects `PVCLOCK_TSC_STABLE_BIT` to be cleared when the master clock is active.
--

commit 810ed346c3086cf1091b3ee7294755bdf43c9973
Author: David Woodhouse <dwmw@amazon.co.uk>

Verify that KVM master clock mode remains active when vCPUs have different TSC offsets

This commit adds a test to verify that KVM master clock mode remains active
when vCPUs have different TSC offsets but the same frequency. It creates
three vCPUs, offsets one of them, and confirms the KVM_CLOCK_HOST_TSC
behavior.

>  - KVM_CLOCK_TSC_STABLE is NOT set (offsets differ)

[Severity: Medium]
Does this commit message contradict the implementation? The message states
that KVM_CLOCK_TSC_STABLE should NOT be set, but the test code explicitly
asserts that it is 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 000000000000..88e2bd2edab5
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/masterclock_offset_test.c
[ ... ]
> @@ -0,0 +1,180 @@
[ ... ]
> +	/* Check master clock is active */
> +	memset(&clock, 0, sizeof(clock));
> +	vm_ioctl(vm, KVM_GET_CLOCK, &clock);
> +	pr_info("KVM_GET_CLOCK flags: 0x%x\n", clock.flags);
> +	TEST_ASSERT(clock.flags & KVM_CLOCK_HOST_TSC,
> +		    "Master clock should be active, flags=0x%x", clock.flags);

[Severity: Medium]
Will this assertion fail? KVM disables the master clock (use_master_clock)
if all online vCPUs do not have matching TSC offsets and generations. Since
vcpus[2] is explicitly set to a different offset earlier in the test,
KVM_CLOCK_HOST_TSC shouldn't be set here.

> +	TEST_ASSERT(clock.flags & KVM_CLOCK_TSC_STABLE,
> +		    "KVM_CLOCK_TSC_STABLE should be set, flags=0x%x", clock.flags);

[Severity: Medium]
Is this assertion correct? The commit message claims that
KVM_CLOCK_TSC_STABLE is NOT set, but this code asserts that it is present
in clock.flags.

> +
> +	/* 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: High]
Does the KVM_GET_CLOCK_GUEST macro exist? It doesn't appear to be defined
in the KVM UAPI or selftests headers, which would cause a compilation error.

Additionally, __vcpu_ioctl() returns an error code instead of asserting on
failure. Since the return value isn't checked, any failure leaves the pvti
array uninitialized. Could we use vcpu_ioctl() or check the return value
to ensure we don't operate on uninitialized memory during pvclock_calc()?

[ ... ]
> +	/* PVCLOCK_TSC_STABLE_BIT should NOT be set (offsets differ) */
> +	TEST_ASSERT(!(pvti[2].flags & PVCLOCK_TSC_STABLE_BIT),
> +		    "PVCLOCK_TSC_STABLE_BIT should NOT be set, flags=0x%x",
> +		    pvti[2].flags);

[Severity: Medium]
Is this expectation correct when the master clock is active? In KVM, when
use_master_clock is true (which is required for KVM_CLOCK_HOST_TSC above),
kvm_guest_time_update() unconditionally sets PVCLOCK_TSC_STABLE_BIT in the
pvclock structure. If the master clock were actually active, this assertion
would always fail.

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

[Severity: High]
As mentioned above, KVM_GET_CLOCK_GUEST appears to be undefined, and the
unchecked __vcpu_ioctl() return value risks leaving pvti uninitialized
before it is read again.

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