New: KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

===
Could be the following an acceptable fix?
===

kernel might hung in pvclock_clocksource_read() due to uninitialized memory might contain odd version value in following cycle:

        do {
                version = __pvclock_read_cycles(src, &ret, &flags);
        } while ((src->version & 1) || version != src->version);

if secondary kvmclock is accessed before it's registered with kvm.

Clear garbage in pvclock shared memory area right after it's allocated to avoid this issue.

Ref: https://bugzilla.kernel.org/show_bug.cgi?id=59521

Signed-off-by Igor Mammedov <[email protected]>

---
 arch/x86/kernel/kvmclock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

arch/x86/kernel/kvmclock.c +1 -0

index d2c3812..3dd37eb 100644
@@ -242,6 +242,7 @@ void __init kvmclock_init(void)
 	if (!mem)
 		return;
 	hv_clock = __va(mem);
+	memset(hv_clock, 0, size);
 
 	if (kvm_register_clock("boot clock")) {
 		hv_clock = NULL;

Signature · 1 line

1.7.1

The problem is in cpu_init() which is called earlier. cpu_init() calls printk and possibly other stuff which can use timestamps. printk calls local_clock() to obtain a timestamp of a log message. On KVM guests call sequence usually ends up in kvm_clock_read but needed rdmsr is executed only in x86_cpuinit.early_percpu_clock_init().

I consider two approaches to fix the problem:
1. Swap cpu_init(); and x86_cpuinit.early_percpu_clock_init();
+ Simple
- We will get excessive restrictions on operations which allowed to be performed in early_percpu_clock_init() because percpu specific data is initialized only in cpu_init().

2. Return 0ULL from kvm_clock_read untill it is initialized.
+ Simple too
- Additional if statement inside kvm_clock_read (not serious even for performance paranoiacs)
- Returning 0ULL looks ok because it is the same thing which kernel bootstrap CPU does on early boot stages. But I am not quite sure. Better to ask guys who maintain the needed relevant subsystem.

I prefer the second way. It doesn't add complex restrictions to CPU bootup code. I'll send a patch soon which fixes the problem in the second way.

I don't propagate such a logic to levels higher then KVM clocksource (pv_time_ops level for example) because of the following code:
void __init kvmclock_init(void)
...

263         pv_time_ops.sched_clock = kvm_clock_read;
264         x86_platform.calibrate_tsc = kvm_get_tsc_khz;
265         x86_platform.get_wallclock = kvm_get_wallclock;
266         x86_platform.set_wallclock = kvm_set_wallclock;

267 #ifdef CONFIG_X86_LOCAL_APIC
268 x86_cpuinit.early_percpu_clock_init =
269 kvm_setup_secondary_clock;
270 #endif
271 x86_platform.save_sched_clock_state = kvm_save_sched_clock_state;
272 x86_platform.restore_sched_clock_state =
kvm_restore_sched_clock_state;

To propagate the logic I need to make changes both in x86_platform and pv_time_ops also I should make a similar fix for ia64 arch. It needs some subsystems refactoring to make the changes clean. Dont' think that its worth to fix the bug. Better to make a simple fix.

The problem is in cpu_init() which is called earlier. cpu_init() calls printk and possibly other stuff which can use timestamps. printk calls local_clock() to obtain a timestamp of a log message. On KVM guests call sequence usually ends up in kvm_clock_read but needed rdmsr is executed only in x86_cpuinit.early_percpu_clock_init().

I consider two approaches to fix the problem:
1. Swap cpu_init(); and x86_cpuinit.early_percpu_clock_init();
+ Simple
- We will get excessive restrictions on operations which allowed to be performed in early_percpu_clock_init() because percpu specific data is initialized only in cpu_init().

2. Return 0ULL from kvm_clock_read untill it is initialized.
+ Simple too
- Additional if statement inside kvm_clock_read (not serious even for performance paranoiacs)
- Returning 0ULL looks ok because it is the same thing which kernel bootstrap CPU does on early boot stages. But I am not quite sure. Better to ask guys who maintain the needed relevant subsystem.

I prefer the second way. It doesn't add complex restrictions to CPU bootup code. I'll send a patch soon which fixes the problem in the second way.

I don't propagate such a logic to levels higher then KVM clocksource (pv_time_ops level for example) because of the following code:
void __init kvmclock_init(void)
...

263         pv_time_ops.sched_clock = kvm_clock_read;
264         x86_platform.calibrate_tsc = kvm_get_tsc_khz;
265         x86_platform.get_wallclock = kvm_get_wallclock;
266         x86_platform.set_wallclock = kvm_set_wallclock;

267 #ifdef CONFIG_X86_LOCAL_APIC
268 x86_cpuinit.early_percpu_clock_init =
269 kvm_setup_secondary_clock;
270 #endif
271 x86_platform.save_sched_clock_state = kvm_save_sched_clock_state;
272 x86_platform.restore_sched_clock_state =
kvm_restore_sched_clock_state;

To propagate the logic I need to make changes both in x86_platform and pv_time_ops also I should make a similar fix for ia64 arch. It needs some subsystems refactoring to make the changes clean. Dont' think that its worth to fix the bug. Better to make a simple fix.

Reply-To: [email protected]

Il 15/06/2013 19:17, [email protected] ha scritto:

Quoted 6 lines

The problem is in cpu_init() which is called earlier. cpu_init() calls printk and possibly other stuff which can use timestamps. printk calls local_clock() to obtain a timestamp of a log message. On KVM guests call sequence usually ends up in kvm_clock_read but needed rdmsr is executed only in x86_cpuinit.early_percpu_clock_init().

Quoted 6 lines

I consider two approaches to fix the problem:
1. Swap cpu_init(); and x86_cpuinit.early_percpu_clock_init();
+ Simple
- We will get excessive restrictions on operations which allowed to be performed in early_percpu_clock_init() because percpu specific data is initialized only in cpu_init().

Considering how simple kvm_register_clock is, I think this is preferrable if it works. Ironically, commit 7069ed6 (x86: kvmclock:
allocate pvclock shared memory area, 2012-11-27), which introduced the regression, is what should make this simpler fix possible.

Paolo

Due to unintialized kvmclock read KVM guest is hanging on SMP boot stage. If unintialized memory contains fatal garbage then hang reproduction is 100%. Unintialized memory is allocated by memblock_alloc. So the garbage values depend on many many things.

See the detailed description of the bug and possible ways to fix it in the kernel bug tracker.
"Bug 59521 - KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK"

I decided to fix it simply returning 0ULL from kvm_clock_read when kvm clocksource is not initialized yet.
The same as kernel bootstrap CPU doesn on boot stage when kernel clocksources are not initialized yet.

Signed-off-by Eugene Batalov <[email protected]>

---
I dont' use kernel percpu variables because for each SMP CPU their contents are copied from the bootstrap CPU. And I don't think that fixing the value for each SMP CPU is a good style. If you know a better approach to store the is_pv_clock_ready flags I am ready to use it.

The patch applies cleanly to
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

I've tested the changes with Ubuntu 13.04 "raring" userspace and Ubuntu-3.8.0.19-30 kernel tag.

 arch/x86/kernel/kvmclock.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

arch/x86/kernel/kvmclock.c +8 -2

index 5bedbdd..a6e0af4 100644
@@ -43,6 +43,9 @@ early_param("no-kvmclock", parse_no_kvmclock);
 static struct pvclock_vsyscall_time_info *hv_clock;
 static struct pvclock_wall_clock wall_clock;
 
+/* For each cpu store here a flag which tells whether pvclock is initialized */
+static int __cacheline_aligned_in_smp is_pv_clock_ready[NR_CPUS] = {};
+
 /*
  * The wallclock is the time of day when we booted. Since then, some time may
  * have elapsed since the hypervisor wrote the data. So we try to account for

@@ -84,8 +87,11 @@ static cycle_t kvm_clock_read(void)
 
 	preempt_disable_notrace();
 	cpu = smp_processor_id();
-	src = &hv_clock[cpu].pvti;
-	ret = pvclock_clocksource_read(src);
+	if (is_pv_clock_ready[cpu]) {
+		src = &hv_clock[cpu].pvti;
+		ret = pvclock_clocksource_read(src);
+	} else
+		ret = 0ULL;
 	preempt_enable_notrace();
 	return ret;
 }

Quoted 64 lines

Due to unintialized kvmclock read KVM guest is hanging on SMP boot stage. If unintialized memory contains fatal garbage then hang reproduction is 100%. Unintialized memory is allocated by memblock_alloc. So the garbage values depend on many many things.

See the detailed description of the bug and possible ways to fix it in the kernel bug tracker.
"Bug 59521 - KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK"

I decided to fix it simply returning 0ULL from kvm_clock_read when kvm clocksource is not initialized yet.
The same as kernel bootstrap CPU doesn on boot stage when kernel clocksources are not initialized yet.

Signed-off-by Eugene Batalov <[email protected]>

---
I dont' use kernel percpu variables because for each SMP CPU their contents are copied from the bootstrap CPU. And I don't think that fixing the value for each SMP CPU is a good style. If you know a better approach to store the is_pv_clock_ready flags I am ready to use it.

The patch applies cleanly to
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

I've tested the changes with Ubuntu 13.04 "raring" userspace and Ubuntu-3.8.0.19-30 kernel tag.

 arch/x86/kernel/kvmclock.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

arch/x86/kernel/kvmclock.c +8 -2

index 5bedbdd..a6e0af4 100644
@@ -43,6 +43,9 @@ early_param("no-kvmclock", parse_no_kvmclock);
 static struct pvclock_vsyscall_time_info *hv_clock;
 static struct pvclock_wall_clock wall_clock;
 
+/* For each cpu store here a flag which tells whether pvclock is initialized */
+static int __cacheline_aligned_in_smp is_pv_clock_ready[NR_CPUS] = {};
+
 /*
  * The wallclock is the time of day when we booted. Since then, some time may
  * have elapsed since the hypervisor wrote the data. So we try to account for

@@ -84,8 +87,11 @@ static cycle_t kvm_clock_read(void)
 
 	preempt_disable_notrace();
 	cpu = smp_processor_id();
-	src = &hv_clock[cpu].pvti;
-	ret = pvclock_clocksource_read(src);
+	if (is_pv_clock_ready[cpu]) {
+		src = &hv_clock[cpu].pvti;
+		ret = pvclock_clocksource_read(src);
+	} else
+		ret = 0ULL;
 	preempt_enable_notrace();
 	return ret;
 }

@@ -168,6 +174,9 @@ int kvm_register_clock(char *txt)
 	printk(KERN_INFO "kvm-clock: cpu %d, msr %x:%x, %s\n",
 	       cpu, high, low, txt);
 

arch/x86/kernel/kvmclock.c int kvm_register_clock(char *txt) · line 181

Quoted 5 lines

arch/x86/kernel/kvmclock.c +3 -0

+	if (!ret)
+		is_pv_clock_ready[cpu] = 1;
+
 	return ret;
 }

The path can be really hot, so better avoid it if possible. The patch to zero the memblock_alloc'd area from Igor brings the behaviour back to before regression: return 0 until kvmclock is initialized. On top of your analysis in the BZ, it now seems the right (and safer) thing to do.

I've missed this detail. It looks like Igor's patch doesn't bring secondary cpus kvm_clocksource behavior back to one before the regression, Before the regression per_cpu variables are used to allocate kvm_pv_clock areas.
To to usage of percpu variables bootstrap cpu kvm_clock area contents were copied to smp secondary cpus kvm_clock areas when they were started. Bootstrap cpu kvm_clock area was not zeroed at this time.
So kvm_pv_clock for secondary cpus never returned "zero" clock before the regression.

During the analysis of the bug I introduced idea to return zero before kvm clocksource is initialized for secondary cpus
just like bootstrap cpu does on kernel boot. You can read that in BZ.

Quoted 16 lines · On Wed, 19 Jun 2013 15:29:31 +0200 Paolo Bonzini <[email protected]> wrote:

Il 19/06/2013 15:20, Batalov Eugene ha scritto:

Quoted 14 lines

I've missed this detail. It looks like Igor's patch doesn't bring secondary cpus kvm_clocksource behavior back to one before the regression, Before the regression per_cpu variables are used to allocate kvm_pv_clock areas.
To to usage of percpu variables bootstrap cpu kvm_clock area contents were copied to smp secondary cpus kvm_clock areas when they were started. Bootstrap cpu kvm_clock area was not zeroed at this time.
So kvm_pv_clock for secondary cpus never returned "zero" clock before the regression.

During the analysis of the bug I introduced idea to return zero before kvm clocksource is initialized for secondary cpus
just like bootstrap cpu does on kernel boot. You can read that in BZ.

Quoted 3 lines

Yes, this is why I prefer to invert the two function calls. But Igor's patch fixes the hang (trivially because version is even) and is more appropriate for -rc6.

I'll post this swap shortly, but zeroing out hv_clock at init time, would be still needed to provide sane values there if ftrace enabled at that time.

Il 19/06/2013 15:20, Batalov Eugene ha scritto:

Quoted 11 lines

I've missed this detail. It looks like Igor's patch doesn't bring secondary cpus kvm_clocksource behavior back to one before the regression, Before the regression per_cpu variables are used to allocate kvm_pv_clock areas.
To to usage of percpu variables bootstrap cpu kvm_clock area contents were copied to smp secondary cpus kvm_clock areas when they were started. Bootstrap cpu kvm_clock area was not zeroed at this time.
So kvm_pv_clock for secondary cpus never returned "zero" clock before the regression.

Quoted 3 lines

During the analysis of the bug I introduced idea to return zero before kvm clocksource is initialized for secondary cpus
just like bootstrap cpu does on kernel boot. You can read that in BZ.

Yes, this is why I prefer to invert the two function calls. But Igor's patch fixes the hang (trivially because version is even) and is more appropriate for -rc6.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/