[Bug 59521] New: KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[Bug 59521] New: KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[bugzilla-daemon]

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

           Summary: KVM linux guest reads uninitialized pvclock values
                    before executing rdmsr MSR_KVM_WALL_CLOCK
           Product: Virtualization
           Version: unspecified
    Kernel Version: 3.8 and higher
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: high
          Priority: P1
         Component: kvm
        AssignedTo: virtualization_kvm@kernel-bugs.osdl.org
        ReportedBy: eabatalov89@gmail.com
        Regression: Yes


It turns out that since linux-image-3.8.0-1-generic_3.8.0-1.5 built from
sources Ubuntu kernel reads uninitialized KVM paravirtualized clocksource data
while working under hypervisor which supports KVM_FEATURE_CLOCKSOURCE.

I've trapped all the guest r/w memory accesses to paravirtualized clock
structures to proof it.

The event sequence is the following:
1. Kernel bootstrap cpu executes  rdmsr MSR_KVM_WALL_CLOCK and reads proper
initialized values of kvm clocksource
2. Memory for percpu paravirtualized KVM clocksources is allocated during
initialization of KVM subsystem.
arch/x86/kernel/kvmclock.c:
221 void __init kvmclock_init(void)
...
240         mem = memblock_alloc(size, PAGE_SIZE);
3. SMP boot is performed. One by one cpus 0, 1, 2, .. n are initialized.
4. Each cpu reads its uninitialized preallocated paravirtualized wallclock
structures before executing rdmsr MSR_KVM_WALL_CLOCK which should initialize
them. So the contents of structs are more or less random.

Here is the struct which is read:
struct pvclock_vcpu_time_info {
 62         u32   version;
 63         u32   pad0;
 64         u64   tsc_timestamp;
 65         u64   system_time;
 66         u32   tsc_to_system_mul;
 67         s8    tsc_shift;
 68         u8    flags;
 69         u8    pad[2];
 70 } __attribute__((__packed__)); /* 32 bytes */
Reading uninitialized contents creates 2 problems for us:
1. If version field is odd then cpu spins forever (see
./arch/x86/kernel/pvclock.c:pvclock_clocksource_read)
2. Depending on values of other struct fields and on the features of
virtualized hardware kernel scheduler can behave in different ways. I've
observed kernel boot hang after start of SMP initialization.

SMP cpu#0 doesn't read unintialized data because it reads bootstrap cpu struct
which was properly initialized using preliminary rdmsr.
At the same time SMP cpu#0 performs the same thing as other SMP cpus - executes
rdmsr MSR_KVM_WALL_CLOCK after reading its paravirtualized clock struct. So no
reproduction on UMP machine.

It may be hard to reproduce the bug because of its random nature.
To easily reproduce it I propose the following patch:
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 5bedbdd..44aaf25 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -219,6 +222,7 @@ void __init kvmclock_init(void)
 {
        unsigned long mem;
        int size;
+       int i;

        size = PAGE_ALIGN(sizeof(struct pvclock_vsyscall_time_info)*NR_CPUS);

@@ -238,6 +242,9 @@ void __init kvmclock_init(void)
        if (!mem)
                return;
        hv_clock = __va(mem);
+       for (i = 0; i < NR_CPUS; ++i) {
+               hv_clock[i].pvti.version = 0xDEADBEAF;
+       }

        if (kvm_register_clock("boot clock")) {
                hv_clock = NULL;

This patch set version field to an odd value so if a cpu reads it before rdmsr
it spins forever because hypervisor won't set it to even value.

The first patch which makes the bug reproducible is kernel mainline commit:
"x86: kvmclock: allocate pvclock shared memory area".

Before the patch memory for KVM clocksources was allocated in percpu variables.
Every SMP cpu which starts execution got copy of bootstrab cpu pvclock
variable.
That's why pvclock structs were "initialized" before the patch and there was no
bug with uninitialized memory read.
After the patch memory is allocated using memblock_alloc. Nothing is written to
the allocated memory before actual call of MSR_KVM_WALL_CLOCK.

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[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

[Bug 59521] KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[bugzilla-daemon]

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





--- Comment #1 from Eugene Batalov <eabatalov89@gmail.com>  2013-06-11 16:03:55 ---
I have reconstructed the uninitialized pvclock read backtrace.
References to file lines are for Ubuntu-raring kernel
git://kernel.ubuntu.com/ubuntu/ubuntu-raring.git
tag is Ubuntu-3.8.0-19.30

bp: 0xf3ccbe68 ip: 0xc103cfbd
arch/x86/include/asm/pvclock.h:78
arch/x86/kernel/pvclock.c:74
bp: 0xf3ccbe70 ip: 0xc103c057
arch/x86/kernel/kvmclock.c:91
bp: 0xf3ccbe78 ip: 0xc1017598
arch/x86/kernel/tsc.c:58
bp: 0xf3ccbea8 ip: 0xc107e98d
kernel/sched/clock.c:248
bp: 0xf3ccbeb8 ip: 0xc107ea35
kernel/sched/clock.c:342
bp: 0xf3ccbf08 ip: 0xc104ad85
kernel/printk.c:356
bp: 0xf3ccbf50 ip: 0xc104c4e1
kernel/printk.c:1607
bp: 0xf3ccbf70 ip: 0xc1609bb6
kernel/printk.c:1688
bp: 0xf3ccbf90 ip: 0xc1600a51
arch/x86/include/asm/bitops.h:321
arch/x86/kernel/cpu/common.c:1325
bp: 0xf3ccbfb4 ip: 0xc1604000
??
bp: 0x00000000


kernel/printk.c:356
calls local_clock()
calls sched_clock_cpu()
calls sched_clock()
calls paravirt_sched_clock()
calls indirectly kvm_clock_read()
unintialized pv_clock is read here

vcpu kvmclock initialization is performed in kvm_register_clock.
kvm_register_clock is called from
static void __init kvm_smp_prepare_boot_cpu(void)
called form
 ./init/main.c:524 as smp_prepare_boot_cpu

I'll think about proper fix soon. We probably should fix cpu initialization
stages order or disable usage of pvclock before it initialized.

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[Bug 59521] KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[bugzilla-daemon]

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





--- Comment #2 from Eugene Batalov <eabatalov89@gmail.com>  2013-06-15 17:17:18 ---
KVM pv_clock initialization for SMP CPU is called very early on SMP CPU boot
stage:
arch/x86/kernel/smpboot.c:
__cpuinit start_secondary(void *unused)
...
    cpu_init();
    x86_cpuinit.early_percpu_clock_init();

early_percpu_clock_init is set as virtual function in
./arch/x86/kernel/kvmclock.c:
#ifdef CONFIG_X86_LOCAL_APIC
        x86_cpuinit.early_percpu_clock_init =
                kvm_setup_secondary_clock;
#endif

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.

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

Re: [Bug 59521] KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[Paolo Bonzini]

Il 15/06/2013 19:17, bugzilla-daemon@bugzilla.kernel.org ha scritto:
> 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().

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

> 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.
> 

[Bug 59521] KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[bugzilla-daemon]

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





--- Comment #3 from Anonymous Emailer <anonymous@kernel-bugs.osdl.org>  2013-06-17 15:50:27 ---
Reply-To: pbonzini@redhat.com

Il 15/06/2013 19:17, bugzilla-daemon@bugzilla.kernel.org ha scritto:
> 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().

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

> 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.
>

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[Bug 59521] KVM linux guest reads uninitialized pvclock values before executing rdmsr MSR_KVM_WALL_CLOCK

[bugzilla-daemon]

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





--- Comment #4 from Eugene Batalov <eabatalov89@gmail.com>  2013-06-17 21:29:08 ---
(In reply to comment #3)
> Reply-To: pbonzini@redhat.com
> 
> Il 15/06/2013 19:17, bugzilla-daemon@bugzilla.kernel.org ha scritto:
> > 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().
> 
> 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

Understood your point. I'll test this fix and report the results.

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[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