[PATCH] x86: kvmclock: zero initialize pvclock shared memory area

[PATCH] x86: kvmclock: zero initialize pvclock shared memory area

[Igor Mammedov]

===
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 <imammedo@redhat.com>
---
 arch/x86/kernel/kvmclock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d2c3812..3dd37eb 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -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;
-- 
1.7.1

Re: [PATCH] x86: kvmclock: zero initialize pvclock shared memory area

[Marcelo Tosatti]

On Mon, Jun 10, 2013 at 06:31:11PM +0200, Igor Mammedov wrote:
> ===
> Could be the following an acceptable fix?
> ===

Read of kvmclock should return proper value from hypervisor: system
timestamp + tsc delta.

Should find the offender site and have it register MSR_KVM_SYSTEM_TIME 
before reading the area.

> 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 <imammedo@redhat.com>
> ---
>  arch/x86/kernel/kvmclock.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index d2c3812..3dd37eb 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -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;
> -- 
> 1.7.1

[PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Eugene Batalov]

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 <ebatalov@parallels.com>
---
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(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 5bedbdd..a6e0af4 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -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);
 
+	if (!ret)
+		is_pv_clock_ready[cpu] = 1;
+
 	return ret;
 }
 
-- 
1.7.9.5

Re: [PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Marcelo Tosatti]

On Sat, Jun 15, 2013 at 10:01:45PM +0400, Eugene Batalov wrote:
> 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 <ebatalov@parallels.com>
> ---
> 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(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 5bedbdd..a6e0af4 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -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);
>  
> +	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.

Re: [PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Paolo Bonzini]

Il 19/06/2013 00:21, Marcelo Tosatti ha scritto:
> On Sat, Jun 15, 2013 at 10:01:45PM +0400, Eugene Batalov wrote:
>> 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 <ebatalov@parallels.com>
>> ---
>> 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(-)
>>
>> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
>> index 5bedbdd..a6e0af4 100644
>> --- a/arch/x86/kernel/kvmclock.c
>> +++ b/arch/x86/kernel/kvmclock.c
>> @@ -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);
>>  
>> +	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.

Yeah, definitely safer for master and stable.  I'm applying that to master.

But if we can invert cpu_init with early_clock_percpu_init, it would be
nicer to do that for 3.11.

Paolo

Re: [PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Paolo Bonzini]

Il 19/06/2013 15:20, Batalov Eugene ha scritto:
> 
> 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.

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

Re: [PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Igor Mammedov]

On Wed, 19 Jun 2013 15:29:31 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 19/06/2013 15:20, Batalov Eugene ha scritto:
> > 
> > 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.
> 
> 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.

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

Re: [PATCHv1] kvm guest: fix uninitialized kvmclock read by KVM guest

[Paolo Bonzini]

Il 20/06/2013 10:30, Igor Mammedov ha scritto:
> On Wed, 19 Jun 2013 15:29:31 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 19/06/2013 15:20, Batalov Eugene ha scritto:
>>>
>>> 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.
>>
>> 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.

Fine!  Please mention it (with --verbose flag) in the commit message.

Paolo

[PATCH 0/2 v2] x86: kvmclock: Prevent uninitialized per-cpu kvmclock usage

[Igor Mammedov]

Fixes regression introduced by
    7069ed6 x86: kvmclock: allocate pvclock shared memory area
and reported by Eugene:
    https://bugzilla.kernel.org/show_bug.cgi?id=59521

v2:
 - "[1/2] x86: kvmclock: zero initialize pvclock shared memory area"
      added to commit messaged another reason why it's clearing
      is necessary.
 - added "[2/2] x86: kvmclock: register per-cpu kvmclock at earliest possible time"
      to register clock earlier.

Igor Mammedov (2):
  x86: kvmclock: zero initialize pvclock shared memory area
  x86: kvmclock: register per-cpu kvmclock at earliest possible time

 arch/x86/kernel/kvmclock.c |   12 ++++++++++--
 arch/x86/kernel/smpboot.c  |    2 +-
 arch/x86/mm/pageattr.c     |    4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)

[PATCH 1/2] x86: kvmclock: zero initialize pvclock shared memory area

[Igor Mammedov]

Since commit:
  7069ed6 x86: kvmclock: allocate pvclock shared memory area

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.

Additionally before regression was introduced users of pre-cpu
hv_clock were relying on variable being zero initialized, and
7069ed6 breaks this assumption for usage of:
  hv_clock.migrate_count
which could be populated with random garbage now.

Clearing garbage in pvclock shared memory area right after it's
allocated helps to avoid these issues.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/kernel/kvmclock.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d2c3812..3dd37eb 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -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;
-- 
1.7.1

[PATCH 2/2] x86: kvmclock: register per-cpu kvmclock at earliest possible time

[Igor Mammedov]

printk's in cpu_init() might access per-cpu pv_clock before it's
registered. Which might lead to an incorrect last_value value or
big jumps in it depending on current contents of kvm's hv_clock.

Also ftrace by default uses local clock for time-stamping its
records, which might cause access to not yet registered per-cpu
kvmclock during cpu_init() execution:
   function_trace_call -> trace_function ->
       trace_buffer_lock_reserve -> ring_buffer_lock_reserve ->
           rb_reserve_next_event -> rb_time_stamp ->
               trace_clock_local -> sched_clock ->
                   paravirt_sched_clock ->
                       kvm_clock_read

Fix provides a simplified version of kvm_setup_secondary_clock(),
that could be safely used before cpu_init() and turns off tracing
for its underlying calls to prevent premature access to kvmclock
during its registration. So that any following usage of
sched_clock() would yield correct value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/kernel/kvmclock.c |   11 +++++++++--
 arch/x86/kernel/smpboot.c  |    2 +-
 arch/x86/mm/pageattr.c     |    4 ++--
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 3dd37eb..4660b59 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -185,13 +185,20 @@ static void kvm_restore_sched_clock_state(void)
 }
 
 #ifdef CONFIG_X86_LOCAL_APIC
-static void __cpuinit kvm_setup_secondary_clock(void)
+static void __cpuinit notrace kvm_setup_secondary_clock(void)
 {
 	/*
 	 * Now that the first cpu already had this clocksource initialized,
 	 * we shouldn't fail.
 	 */
-	WARN_ON(kvm_register_clock("secondary cpu clock"));
+	int cpu = stack_smp_processor_id();
+	int low, high;
+	struct pvclock_vcpu_time_info *src;
+
+	src = &hv_clock[cpu].pvti;
+	low = (int)slow_virt_to_phys(src) | 1;
+	high = ((u64)slow_virt_to_phys(src) >> 32);
+	native_write_msr_safe(msr_kvm_system_time, low, high);
 }
 #endif
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 9c73b51..5e2b7cb 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -244,8 +244,8 @@ notrace static void __cpuinit start_secondary(void *unused)
 	 * fragile that we want to limit the things done here to the
 	 * most necessary things.
 	 */
-	cpu_init();
 	x86_cpuinit.early_percpu_clock_init();
+	cpu_init();
 	preempt_disable();
 	smp_callin();
 
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index bb32480..06cabff 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -330,7 +330,7 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
  * or when the present bit is not set. Otherwise we would return a
  * pointer to a nonexisting mapping.
  */
-pte_t *lookup_address(unsigned long address, unsigned int *level)
+pte_t notrace *lookup_address(unsigned long address, unsigned int *level)
 {
 	pgd_t *pgd = pgd_offset_k(address);
 	pud_t *pud;
@@ -374,7 +374,7 @@ EXPORT_SYMBOL_GPL(lookup_address);
  * unoptimized should increase the testing coverage for
  * the more obscure platforms.
  */
-phys_addr_t slow_virt_to_phys(void *__virt_addr)
+phys_addr_t notrace slow_virt_to_phys(void *__virt_addr)
 {
 	unsigned long virt_addr = (unsigned long)__virt_addr;
 	phys_addr_t phys_addr;
-- 
1.7.1