[PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call

[PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call

[Vitaly Kuznetsov]

I discovered that after KVM_SET_CLOCK(0) TSC page value in the guest can
go through the roof and apparently we have a signedness issue when the
update is performed. Fix the issue and add a selftest.

Vitaly Kuznetsov (2):
  KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go
    negative after KVM_REQ_CLOCK_UPDATE
  selftests: kvm: Check that TSC page value is small after
    KVM_SET_CLOCK(0)

 arch/x86/kvm/x86.c                                | 10 +++++++++-
 tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 13 +++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE

[Vitaly Kuznetsov]

When guest time is reset with KVM_SET_CLOCK(0), it is possible for
hv_clock->system_time to become a small negative number. This happens
because in KVM_SET_CLOCK handling we set kvm->arch.kvmclock_offset based
on get_kvmclock_ns(kvm) but when KVM_REQ_CLOCK_UPDATE is handled,
kvm_guest_time_update() does

hv_clock.system_time = ka->master_kernel_ns + v->kvm->arch.kvmclock_offset;

And 'master_kernel_ns' represents the last time when masterclock
got updated, it can precede KVM_SET_CLOCK() call. Normally, this is not a
problem, the difference is very small, e.g. I'm observing
hv_clock.system_time = -70 ns. The issue comes from the fact that
'hv_clock.system_time' is stored as unsigned and 'system_time / 100' in
compute_tsc_page_parameters() becomes a very big number.

Forbid 'hv_clock.system_time' to go negative in kvm_guest_time_update().
A similar computation in get_kvmclock_ns() seems fine and doesn't require
the quirk.

Alternatively, we could've used 'master_kernel_ns' when computing
'arch.kvmclock_offset' but that would reduce the precision for normal
cases a bit. Another solution is to cast 'hv_clock.system_time' to
's64' in compute_tsc_page_parameters() but it seems we also use
'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/x86.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fe806e894212..320da7912375 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2742,7 +2742,15 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	}
 
 	vcpu->hv_clock.tsc_timestamp = tsc_timestamp;
-	vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
+
+	/*
+	 * 'kvmclock_offset' can be negative and its absolute value can be
+	 * slightly greater than 'kernel_ns' because when KVM_SET_CLOCK is
+	 * handled, we use more precise get_kvmclock_ns() there. Make sure
+	 * unsigned 'system_time' doesn't go negative.
+	 */
+	vcpu->hv_clock.system_time = max(kernel_ns + v->kvm->arch.kvmclock_offset,
+					 (s64)0);
 	vcpu->last_guest_tsc = tsc_timestamp;
 
 	/* If the host uses TSC clocksource, then it is stable */
-- 
2.30.2

Re: [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE

[Paolo Bonzini]

On 26/03/21 16:55, Vitaly Kuznetsov wrote:
> Another solution is to cast 'hv_clock.system_time' to
> 's64' in compute_tsc_page_parameters() but it seems we also use
> 'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.

I think that is better.  There is no reason really to clamp the value to
to 0, while we know already that tsc_ref->tsc_offset can be either
positive or negative.  So treating hv_clock->system_time as signed
before the division would make sense.

It should be just

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 58fa8c029867..e573e987f41b 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1070,9 +1070,7 @@ static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info *hv_clock,
  				hv_clock->tsc_to_system_mul,
  				100);
  
-	tsc_ref->tsc_offset = hv_clock->system_time;
-	do_div(tsc_ref->tsc_offset, 100);
-	tsc_ref->tsc_offset -=
+	tsc_ref->tsc_offset = div_s64(hv_clock->system_time, 100) -
  		mul_u64_u64_shr(hv_clock->tsc_timestamp, tsc_ref->tsc_scale, 64);
  	return true;
  }

right?  The test passes for me with this change.

Paolo

Re: [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 26/03/21 16:55, Vitaly Kuznetsov wrote:
>> Another solution is to cast 'hv_clock.system_time' to
>> 's64' in compute_tsc_page_parameters() but it seems we also use
>> 'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.
>
> I think that is better.  There is no reason really to clamp the value to
> to 0, while we know already that tsc_ref->tsc_offset can be either
> positive or negative.  So treating hv_clock->system_time as signed
> before the division would make sense.
>
> It should be just
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 58fa8c029867..e573e987f41b 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1070,9 +1070,7 @@ static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info *hv_clock,
>   				hv_clock->tsc_to_system_mul,
>   				100);
>   
> -	tsc_ref->tsc_offset = hv_clock->system_time;
> -	do_div(tsc_ref->tsc_offset, 100);
> -	tsc_ref->tsc_offset -=
> +	tsc_ref->tsc_offset = div_s64(hv_clock->system_time, 100) -
>   		mul_u64_u64_shr(hv_clock->tsc_timestamp, tsc_ref->tsc_scale, 64);
>   	return true;
>   }
>
> right?  The test passes for me with this change.

Right,

in fact that's how v0 (which I've never sent out) of the patch looked
like but then I relalized that the fact that unsigned
'hv_clock->system_time' can sometimes keep a negative value is a
'gotcha' which may cause issues in the future.

I'll re-test and send v2, thanks!

-- 
Vitaly

[PATCH 2/2] selftests: kvm: Check that TSC page value is small after KVM_SET_CLOCK(0)

[Vitaly Kuznetsov]

Add a test for the issue when KVM_SET_CLOCK(0) call could cause
TSC page value to go very big because of a signedness issue around
hv_clock->system_time.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
index ffbc4555c6e2..7f1d2765572c 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
@@ -80,19 +80,24 @@ static inline void check_tsc_msr_rdtsc(void)
 	GUEST_ASSERT(delta_ns * 100 < (t2 - t1) * 100);
 }
 
+static inline u64 get_tscpage_ts(struct ms_hyperv_tsc_page *tsc_page)
+{
+	return mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+}
+
 static inline void check_tsc_msr_tsc_page(struct ms_hyperv_tsc_page *tsc_page)
 {
 	u64 r1, r2, t1, t2;
 
 	/* Compare TSC page clocksource with HV_X64_MSR_TIME_REF_COUNT */
-	t1 = mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+	t1 = get_tscpage_ts(tsc_page);
 	r1 = rdmsr(HV_X64_MSR_TIME_REF_COUNT);
 
 	/* 10 ms tolerance */
 	GUEST_ASSERT(r1 >= t1 && r1 - t1 < 100000);
 	nop_loop();
 
-	t2 = mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+	t2 = get_tscpage_ts(tsc_page);
 	r2 = rdmsr(HV_X64_MSR_TIME_REF_COUNT);
 	GUEST_ASSERT(r2 >= t1 && r2 - t2 < 100000);
 }
@@ -130,7 +135,11 @@ static void guest_main(struct ms_hyperv_tsc_page *tsc_page, vm_paddr_t tsc_page_
 
 	tsc_offset = tsc_page->tsc_offset;
 	/* Call KVM_SET_CLOCK from userspace, check that TSC page was updated */
+
 	GUEST_SYNC(7);
+	/* Sanity check TSC page timestamp, it should be close to 0 */
+	GUEST_ASSERT(get_tscpage_ts(tsc_page) < 100000);
+
 	GUEST_ASSERT(tsc_page->tsc_offset != tsc_offset);
 
 	nop_loop();
-- 
2.30.2