Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1488614 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Jisheng Zhang <jszhang@marvell.com> - 2016-09-22 10:10 +0200
    Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Thomas Gleixner <tglx@linutronix.de> - 2016-09-22 16:10 +0200
      Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Steven Rostedt <rostedt@goodmis.org> - 2016-09-22 16:30 +0200
      Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Jisheng Zhang <jszhang@marvell.com> - 2016-09-23 04:10 +0200
        Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Steven Rostedt <rostedt@goodmis.org> - 2016-09-23 04:50 +0200
          Re: [PATCH] clocksource/drivers/ti-32k: Prevent ftrace recursion Jisheng Zhang <jszhang@marvell.com> - 2016-09-23 05:00 +0200
    [tip:timers/core] clocksource/drivers/ti-32k: Prevent ftrace  recursion tip-bot for Jisheng Zhang <tipbot@zytor.com> - 2016-09-22 17:00 +0200

#1488614 — [PATCH] 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>
---
 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

[toc] | [next] | [standalone]

#1488913

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

[toc] | [prev] | [next] | [standalone]

#1488932

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

[toc] | [prev] | [next] | [standalone]

#1489702

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

[toc] | [prev] | [next] | [standalone]

#1489709

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

[toc] | [prev] | [next] | [standalone]

#1489714

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

[toc] | [prev] | [next] | [standalone]

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

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);
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web