[PATCH] KVM guest: prevent tracing recursion with kvmclock

[PATCH] KVM guest: prevent tracing recursion with kvmclock

[Avi Kivity]

Stop tracing when we read the clock, since tracing will also
want to read the clock, and recurse indefinitely.

Based on a similar patch for Xen from Jeremy Fitzhardinge.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kernel/kvmclock.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index c1a0188..44842d7 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -74,9 +74,10 @@ static cycle_t kvm_clock_read(void)
 	struct pvclock_vcpu_time_info *src;
 	cycle_t ret;
 
-	src = &get_cpu_var(hv_clock);
+	preempt_disable_notrace();
+	src = &__get_cpu_var(hv_clock);
 	ret = pvclock_clocksource_read(src);
-	put_cpu_var(hv_clock);
+	preempt_enable_notrace();
 	return ret;
 }
 
-- 
1.7.7.1

Re: [PATCH] KVM guest: prevent tracing recursion with kvmclock

[Gleb Natapov]

On Tue, Nov 15, 2011 at 03:01:15PM +0200, Avi Kivity wrote:
> Stop tracing when we read the clock, since tracing will also
> want to read the clock, and recurse indefinitely.
> 
> Based on a similar patch for Xen from Jeremy Fitzhardinge.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
Tested-by: Gleb Natapov <gleb@redhat.com>

> ---
>  arch/x86/kernel/kvmclock.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index c1a0188..44842d7 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -74,9 +74,10 @@ static cycle_t kvm_clock_read(void)
>  	struct pvclock_vcpu_time_info *src;
>  	cycle_t ret;
>  
> -	src = &get_cpu_var(hv_clock);
> +	preempt_disable_notrace();
> +	src = &__get_cpu_var(hv_clock);
>  	ret = pvclock_clocksource_read(src);
> -	put_cpu_var(hv_clock);
> +	preempt_enable_notrace();
>  	return ret;
>  }
>  
> -- 
> 1.7.7.1

--
			Gleb.

Re: [PATCH] KVM guest: prevent tracing recursion with kvmclock

[Steven Rostedt]

On Tue, 2011-11-15 at 15:01 +0200, Avi Kivity wrote:
> Stop tracing when we read the clock, since tracing will also
> want to read the clock, and recurse indefinitely.

I would rephrase the above. You don't actually stop tracing, you just
don't trace the preempt disable. I would reword that to something like:

Prevent tracing of preempt_disable() in get_cpu_var() in
kvm_clock_read(). When CONFIG_DEBUG_PREEMPT is enabled,
preempt_disable/enable() are traced and this causes the function_graph
tracer to go into an infinite recursion. By open coding the
preempt_disable() around the get_cpu_var(), we can use the notrace
version which prevents preempt_disable/enable() from being traced and
prevents the recursion.

Something like the above.

> 
> Based on a similar patch for Xen from Jeremy Fitzhardinge.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

This was exactly my first thought, but I was thinking it may be better
to have a get_cpu_var_notrace() than to have to open code this stuff.
Maybe there's not that many users that open code is not an issue. I'll
still want to add that recursion protection with the warn on though.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> ---
>  arch/x86/kernel/kvmclock.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index c1a0188..44842d7 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -74,9 +74,10 @@ static cycle_t kvm_clock_read(void)
>  	struct pvclock_vcpu_time_info *src;
>  	cycle_t ret;
>  
> -	src = &get_cpu_var(hv_clock);
> +	preempt_disable_notrace();
> +	src = &__get_cpu_var(hv_clock);
>  	ret = pvclock_clocksource_read(src);
> -	put_cpu_var(hv_clock);
> +	preempt_enable_notrace();
>  	return ret;
>  }
>  

Re: [PATCH] KVM guest: prevent tracing recursion with kvmclock

[Avi Kivity]

On 11/15/2011 03:38 PM, Steven Rostedt wrote:
> On Tue, 2011-11-15 at 15:01 +0200, Avi Kivity wrote:
> > Stop tracing when we read the clock, since tracing will also
> > want to read the clock, and recurse indefinitely.
>
> I would rephrase the above. You don't actually stop tracing, you just
> don't trace the preempt disable. I would reword that to something like:
>
> Prevent tracing of preempt_disable() in get_cpu_var() in
> kvm_clock_read(). When CONFIG_DEBUG_PREEMPT is enabled,
> preempt_disable/enable() are traced and this causes the function_graph
> tracer to go into an infinite recursion. By open coding the
> preempt_disable() around the get_cpu_var(), we can use the notrace
> version which prevents preempt_disable/enable() from being traced and
> prevents the recursion.
>
> Something like the above.

Thanks, I adopted your wording.

> > 
> > Based on a similar patch for Xen from Jeremy Fitzhardinge.
> > 
> > Signed-off-by: Avi Kivity <avi@redhat.com>
>
> This was exactly my first thought, but I was thinking it may be better
> to have a get_cpu_var_notrace() than to have to open code this stuff.
> Maybe there's not that many users that open code is not an issue. I'll
> still want to add that recursion protection with the warn on though.
>

What about function traces?  Will any noninlined calls cause the same
problem?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM guest: prevent tracing recursion with kvmclock

[Steven Rostedt]

On Tue, 2011-11-15 at 15:45 +0200, Avi Kivity wrote:

> What about function traces?  Will any noninlined calls cause the same
> problem?
> 

Both function_graph and function tracing have recursion protection
around the users of the call. function tracing is much lighter weight
than function_graph tracing. The problem here was that function_graph
tracing does some accounting, and calls the clock handler outside the
recursion protection. I need to add recursion protection around the
accounting too. But I still want to warn when it happens as recursion
protection protects from crashing, but recursion is still bad because it
still goes through a bit of arch specific setup before the recursion is
detected, slowing function_graph tracing down much more than function
tracing when it recurses.

-- Steve