[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

Currently ti-32k can be used as a scheduler clock. We properly marked
omap_32k_read_sched_clock() as notrace but we then call another
function ti_32k_read_cycles() that _wasn't_ notrace.

Having a traceable function in the sched_clock() path leads to a
recursion within ftrace and a kernel crash.

Fix this by adding notrace attribute to the ti_32k_read_cycles()
function.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clocksource/timer-ti-32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c
index 92b7e39..cf5b14e 100644
--- a/drivers/clocksource/timer-ti-32k.c
+++ b/drivers/clocksource/timer-ti-32k.c
@@ -65,7 +65,7 @@ static inline struct ti_32k *to_ti_32k(struct clocksource *cs)
 	return container_of(cs, struct ti_32k, cs);
 }
 
-static cycle_t ti_32k_read_cycles(struct clocksource *cs)
+static cycle_t notrace ti_32k_read_cycles(struct clocksource *cs)
 {
 	struct ti_32k *ti = to_ti_32k(cs);
 
-- 
2.9.3

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

Currently ti-32k can be used as a scheduler clock. We properly marked
omap_32k_read_sched_clock() as notrace but we then call another
function ti_32k_read_cycles() that _wasn't_ notrace.

Having a traceable function in the sched_clock() path leads to a
recursion within ftrace and a kernel crash.

Fix this by adding notrace attribute to the ti_32k_read_cycles()
function.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/clocksource/timer-ti-32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c
index 92b7e39..cf5b14e 100644
--- a/drivers/clocksource/timer-ti-32k.c
+++ b/drivers/clocksource/timer-ti-32k.c
@@ -65,7 +65,7 @@ static inline struct ti_32k *to_ti_32k(struct clocksource *cs)
 	return container_of(cs, struct ti_32k, cs);
 }
 
-static cycle_t ti_32k_read_cycles(struct clocksource *cs)
+static cycle_t notrace ti_32k_read_cycles(struct clocksource *cs)
 {
 	struct ti_32k *ti = to_ti_32k(cs);
 
-- 
2.9.3

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Thomas Gleixner]

On Thu, 22 Sep 2016, Jisheng Zhang wrote:

> Currently ti-32k can be used as a scheduler clock. We properly marked
> omap_32k_read_sched_clock() as notrace but we then call another
> function ti_32k_read_cycles() that _wasn't_ notrace.
> 
> Having a traceable function in the sched_clock() path leads to a
> recursion within ftrace and a kernel crash.

Kernel crash? Doesn't ftrace core prevent recursion?
 
Thanks,

	tglx

Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Thomas Gleixner]

On Thu, 22 Sep 2016, Jisheng Zhang wrote:

> Currently ti-32k can be used as a scheduler clock. We properly marked
> omap_32k_read_sched_clock() as notrace but we then call another
> function ti_32k_read_cycles() that _wasn't_ notrace.
> 
> Having a traceable function in the sched_clock() path leads to a
> recursion within ftrace and a kernel crash.

Kernel crash? Doesn't ftrace core prevent recursion?
 
Thanks,

	tglx

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Steven Rostedt]

On Thu, 22 Sep 2016 15:58:03 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> 
> > Currently ti-32k can be used as a scheduler clock. We properly marked
> > omap_32k_read_sched_clock() as notrace but we then call another
> > function ti_32k_read_cycles() that _wasn't_ notrace.
> > 
> > Having a traceable function in the sched_clock() path leads to a
> > recursion within ftrace and a kernel crash.  
> 
> Kernel crash? Doesn't ftrace core prevent recursion?
>

There is recursion protection, but there are some holes, as well as
calls where ftrace can't protect itself.

What triggered the bug? Just simple enabling of function tracing? And
what arch? I would like to close these holes. Although, I should add
some kind of flag to notify the user (or at least for me) that recursion
is happening, because that can really be a performance hit on tracing.

-- Steve

Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Steven Rostedt]

On Thu, 22 Sep 2016 15:58:03 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> 
> > Currently ti-32k can be used as a scheduler clock. We properly marked
> > omap_32k_read_sched_clock() as notrace but we then call another
> > function ti_32k_read_cycles() that _wasn't_ notrace.
> > 
> > Having a traceable function in the sched_clock() path leads to a
> > recursion within ftrace and a kernel crash.  
> 
> Kernel crash? Doesn't ftrace core prevent recursion?
>

There is recursion protection, but there are some holes, as well as
calls where ftrace can't protect itself.

What triggered the bug? Just simple enabling of function tracing? And
what arch? I would like to close these holes. Although, I should add
some kind of flag to notify the user (or at least for me) that recursion
is happening, because that can really be a performance hit on tracing.

-- Steve

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

Hi Thomas,

On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:

> On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> 
> > Currently ti-32k can be used as a scheduler clock. We properly marked
> > omap_32k_read_sched_clock() as notrace but we then call another
> > function ti_32k_read_cycles() that _wasn't_ notrace.
> > 
> > Having a traceable function in the sched_clock() path leads to a
> > recursion within ftrace and a kernel crash.  
> 
> Kernel crash? Doesn't ftrace core prevent recursion?

a recent similar issue:

http://www.spinics.net/lists/arm-kernel/msg533480.html

Thanks,
Jisheng

Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

Hi Thomas,

On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:

> On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> 
> > Currently ti-32k can be used as a scheduler clock. We properly marked
> > omap_32k_read_sched_clock() as notrace but we then call another
> > function ti_32k_read_cycles() that _wasn't_ notrace.
> > 
> > Having a traceable function in the sched_clock() path leads to a
> > recursion within ftrace and a kernel crash.  
> 
> Kernel crash? Doesn't ftrace core prevent recursion?

a recent similar issue:

http://www.spinics.net/lists/arm-kernel/msg533480.html

Thanks,
Jisheng

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Steven Rostedt]

On Fri, 23 Sep 2016 10:04:31 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Hi Thomas,
> 
> On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:
> 
> > On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> >   
> > > Currently ti-32k can be used as a scheduler clock. We properly marked
> > > omap_32k_read_sched_clock() as notrace but we then call another
> > > function ti_32k_read_cycles() that _wasn't_ notrace.
> > > 
> > > Having a traceable function in the sched_clock() path leads to a
> > > recursion within ftrace and a kernel crash.    
> > 
> > Kernel crash? Doesn't ftrace core prevent recursion?  
> 
> a recent similar issue:
> 
> http://www.spinics.net/lists/arm-kernel/msg533480.html

Right. But Thomas brought up recursion detection. And I said that would
be the fix, but now thinking about it, I've updated the recursion
protection so that timer issues should not cause a crash.

I'd like to know more, as this appears to be mostly arm related.

-- Steve

Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Steven Rostedt]

On Fri, 23 Sep 2016 10:04:31 +0800
Jisheng Zhang <jszhang@marvell.com> wrote:

> Hi Thomas,
> 
> On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:
> 
> > On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> >   
> > > Currently ti-32k can be used as a scheduler clock. We properly marked
> > > omap_32k_read_sched_clock() as notrace but we then call another
> > > function ti_32k_read_cycles() that _wasn't_ notrace.
> > > 
> > > Having a traceable function in the sched_clock() path leads to a
> > > recursion within ftrace and a kernel crash.    
> > 
> > Kernel crash? Doesn't ftrace core prevent recursion?  
> 
> a recent similar issue:
> 
> http://www.spinics.net/lists/arm-kernel/msg533480.html

Right. But Thomas brought up recursion detection. And I said that would
be the fix, but now thinking about it, I've updated the recursion
protection so that timer issues should not cause a crash.

I'd like to know more, as this appears to be mostly arm related.

-- Steve

[PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

On Thu, 22 Sep 2016 22:45:14 -0400 Steven Rostedt wrote:

> On Fri, 23 Sep 2016 10:04:31 +0800
> Jisheng Zhang <jszhang@marvell.com> wrote:
> 
> > Hi Thomas,
> > 
> > On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:
> >   
> > > On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> > >     
> > > > Currently ti-32k can be used as a scheduler clock. We properly marked
> > > > omap_32k_read_sched_clock() as notrace but we then call another
> > > > function ti_32k_read_cycles() that _wasn't_ notrace.
> > > > 
> > > > Having a traceable function in the sched_clock() path leads to a
> > > > recursion within ftrace and a kernel crash.      
> > > 
> > > Kernel crash? Doesn't ftrace core prevent recursion?    
> > 
> > a recent similar issue:
> > 
> > http://www.spinics.net/lists/arm-kernel/msg533480.html  
> 
> Right. But Thomas brought up recursion detection. And I said that would
> be the fix, but now thinking about it, I've updated the recursion
> protection so that timer issues should not cause a crash.
> 

Got it. Thanks for the clarification

Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion

[Jisheng Zhang]

On Thu, 22 Sep 2016 22:45:14 -0400 Steven Rostedt wrote:

> On Fri, 23 Sep 2016 10:04:31 +0800
> Jisheng Zhang <jszhang@marvell.com> wrote:
> 
> > Hi Thomas,
> > 
> > On Thu, 22 Sep 2016 15:58:03 +0200 Thomas Gleixner wrote:
> >   
> > > On Thu, 22 Sep 2016, Jisheng Zhang wrote:
> > >     
> > > > Currently ti-32k can be used as a scheduler clock. We properly marked
> > > > omap_32k_read_sched_clock() as notrace but we then call another
> > > > function ti_32k_read_cycles() that _wasn't_ notrace.
> > > > 
> > > > Having a traceable function in the sched_clock() path leads to a
> > > > recursion within ftrace and a kernel crash.      
> > > 
> > > Kernel crash? Doesn't ftrace core prevent recursion?    
> > 
> > a recent similar issue:
> > 
> > http://www.spinics.net/lists/arm-kernel/msg533480.html  
> 
> Right. But Thomas brought up recursion detection. And I said that would
> be the fix, but now thinking about it, I've updated the recursion
> protection so that timer issues should not cause a crash.
> 

Got it. Thanks for the clarification

[tip:timers/core] clocksource/drivers/ti-32k: Prevent ftrace recursion

[tip-bot for Jisheng Zhang]

Commit-ID:  3aa601492babdf3acdec89e5aa9c44e1a357a4d8
Gitweb:     http://git.kernel.org/tip/3aa601492babdf3acdec89e5aa9c44e1a357a4d8
Author:     Jisheng Zhang <jszhang@marvell.com>
AuthorDate: Thu, 22 Sep 2016 15:56:21 +0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 22 Sep 2016 16:49:19 +0200

clocksource/drivers/ti-32k: Prevent ftrace recursion

Currently ti-32k can be used as a scheduler clock. We properly marked
omap_32k_read_sched_clock() as notrace but we then call another
function ti_32k_read_cycles() that _wasn't_ notrace.

Having a traceable function in the sched_clock() path leads to a
recursion within ftrace and a kernel crash.

Fix this by adding notrace attribute to the ti_32k_read_cycles()
function.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
Cc: daniel.lezcano@linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/20160922075621.3725-1-jszhang@marvell.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/clocksource/timer-ti-32k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c
index 92b7e39..cf5b14e 100644
--- a/drivers/clocksource/timer-ti-32k.c
+++ b/drivers/clocksource/timer-ti-32k.c
@@ -65,7 +65,7 @@ static inline struct ti_32k *to_ti_32k(struct clocksource *cs)
 	return container_of(cs, struct ti_32k, cs);
 }
 
-static cycle_t ti_32k_read_cycles(struct clocksource *cs)
+static cycle_t notrace ti_32k_read_cycles(struct clocksource *cs)
 {
 	struct ti_32k *ti = to_ti_32k(cs);