Lenny Szubowicz <lszubowi@xxxxxxxxxx> writes:
Turn off host updates to the registered kvmclock memory
locations when transitioning to a hibernated kernel in
resume_target_kernel().
This is accomplished for secondary vcpus by disabling host
clock updates for that vcpu when it is put offline. For the
primary vcpu, it's accomplished by using the existing call back
from save_processor_state() to kvm_save_sched_clock_state().
The registered kvmclock memory locations may differ between
the currently running kernel and the hibernated kernel, which
is being restored and resumed. Kernel memory corruption is thus
possible if the host clock updates are allowed to run while the
hibernated kernel is relocated to its original physical memory
locations.
This is similar to the problem solved for kexec by
commit 1e977aa12dd4 ("x86: KVM guest: disable clock before rebooting.")
Commit 95a3d4454bb1 ("x86/kvmclock: Switch kvmclock data to a
PER_CPU variable") innocently increased the exposure for this
problem by dynamically allocating the physical pages that are
used for host clock updates when the vcpu count exceeds 64.
This increases the likelihood that the registered kvmclock
locations will differ for vcpus above 64.
Reported-by: Xiaoyi Chen <cxiaoyi@xxxxxxxxxx>
Tested-by: Mohamed Aboubakr <mabouba@xxxxxxxxxx>
Signed-off-by: Lenny Szubowicz <lszubowi@xxxxxxxxxx>
---
arch/x86/kernel/kvmclock.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index aa593743acf6..291ffca41afb 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -187,8 +187,18 @@ static void kvm_register_clock(char *txt)
pr_info("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
}
+/*
+ * Turn off host clock updates to the registered memory location when the
+ * cpu clock context is saved via save_processor_state(). Enables correct
+ * handling of the primary cpu clock when transitioning to a hibernated
+ * kernel in resume_target_kernel(), where the old and new registered
+ * memory locations may differ.
+ */
static void kvm_save_sched_clock_state(void)
{
+ native_write_msr(msr_kvm_system_time, 0, 0);
+ kvm_disable_steal_time();
+ pr_info("kvm-clock: cpu %d, clock stopped", smp_processor_id());
}
Nitpick: should we rename kvm_save_sched_clock_state() to something more
generic, like kvm_disable_host_clock_updates() to indicate, that what it
does is not only sched clock related?
static void kvm_restore_sched_clock_state(void)
@@ -311,9 +321,23 @@ static int kvmclock_setup_percpu(unsigned int cpu)
return p ? 0 : -ENOMEM;
}
+/*
+ * Turn off host clock updates to the registered memory location when a
+ * cpu is placed offline. Enables correct handling of secondary cpu clocks
+ * when transitioning to a hibernated kernel in resume_target_kernel(),
+ * where the old and new registered memory locations may differ.
+ */
+static int kvmclock_cpu_offline(unsigned int cpu)
+{
+ native_write_msr(msr_kvm_system_time, 0, 0);
+ pr_info("kvm-clock: cpu %d, clock stopped", cpu);
I'd say this pr_info() is superfluous: on a system with hundereds of
vCPUs users will get flooded with 'clock stopped' messages which don't
actually mean much: in case native_write_msr() fails the error gets
reported in dmesg anyway. I'd suggest we drop this and pr_info() in
kvm_save_sched_clock_state().
+ return 0;
Why don't we disable steal time accounting here? MSR_KVM_STEAL_TIME is
also per-cpu. Can we merge kvm_save_sched_clock_state() with
kvmclock_cpu_offline() maybe?
+}
+
void __init kvmclock_init(void)
{
u8 flags;
+ int cpuhp_prepare;
if (!kvm_para_available() || !kvmclock)
return;
@@ -325,8 +349,14 @@ void __init kvmclock_init(void)
return;
}
- if (cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "kvmclock:setup_percpu",
- kvmclock_setup_percpu, NULL) < 0) {
+ cpuhp_prepare = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
+ "kvmclock:setup_percpu",
+ kvmclock_setup_percpu, NULL);
+ if (cpuhp_prepare < 0)
+ return;
+ if (cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "kvmclock:cpu_offline",
+ NULL, kvmclock_cpu_offline) < 0) {
+ cpuhp_remove_state(cpuhp_prepare);
In case we fail to set up kvmclock_cpu_offline() callback we get broken
hybernation but without kvmclock_setup_percpu() call we get a broken
guest (as kvmclock() may be the only reliable clocksource) so I'm not
exactly sure we're active in a best way with cpuhp_remove_state()
here. I don't feel strong though, I think it can stay but in that case
I'd add a pr_warn() at least.
return;
}