* [PATCH] Ftrace: irqsoff tracer may cause stack overflow @ 2010-01-08 4:45 Li Yi 2010-01-08 4:54 ` Mike Frysinger 2010-01-08 5:18 ` Frederic Weisbecker 0 siblings, 2 replies; 8+ messages in thread From: Li Yi @ 2010-01-08 4:45 UTC (permalink / raw) To: rostedt, linux-kernel; +Cc: uclinux-dist-devel "irqsoff" tracer may cause stack overflow on architectures using asm-generic/atomic.h, due to recursive invoking of, e.g. trace_hardirqs_off(). trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() Signed-off-by: Yi Li <yi.li@analog.com> --- include/asm-generic/atomic.h | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h index c99c64d..c3e5f4b 100644 --- a/include/asm-generic/atomic.h +++ b/include/asm-generic/atomic.h @@ -60,11 +60,11 @@ static inline int atomic_add_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); temp = v->counter; temp += i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -82,11 +82,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); temp = v->counter; temp -= i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -139,9 +139,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) unsigned long flags; mask = ~mask; - local_irq_save(flags); + raw_local_irq_save(flags); *addr &= mask; - local_irq_restore(flags); + raw_local_irq_restore(flags); } #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 4:45 [PATCH] Ftrace: irqsoff tracer may cause stack overflow Li Yi @ 2010-01-08 4:54 ` Mike Frysinger 2010-01-08 6:26 ` Li Yi 2010-01-08 5:18 ` Frederic Weisbecker 1 sibling, 1 reply; 8+ messages in thread From: Mike Frysinger @ 2010-01-08 4:54 UTC (permalink / raw) To: Li Yi, Arnd Bergmann; +Cc: rostedt, linux-kernel, uclinux-dist-devel On Thu, Jan 7, 2010 at 23:45, Li Yi wrote: > "irqsoff" tracer may cause stack overflow on architectures using > asm-generic/atomic.h, due to recursive invoking of, e.g. > trace_hardirqs_off(). > > trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> > atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() > > Signed-off-by: Yi Li <yi.li@analog.com> > --- > include/asm-generic/atomic.h | 12 ++++++------ Arnd is watching over asm-generic/ atm > @@ -60,11 +60,11 @@ static inline int atomic_add_return(int i, atomic_t > *v) > @@ -82,11 +82,11 @@ static inline int atomic_sub_return(int i, atomic_t > *v) > @@ -139,9 +139,9 @@ static inline void atomic_clear_mask(unsigned long > mask, unsigned long *addr) looks like your mailer has line wrapping problems ... might want to fix that. maybe just find a smtp server in ADI shanghai's office to send to and use sendemail.smtpersver in your ~/.gitconfig ? -mike ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 4:54 ` Mike Frysinger @ 2010-01-08 6:26 ` Li Yi 2010-01-08 6:26 ` Mike Frysinger 0 siblings, 1 reply; 8+ messages in thread From: Li Yi @ 2010-01-08 6:26 UTC (permalink / raw) To: Mike Frysinger; +Cc: Arnd Bergmann, rostedt, linux-kernel, uclinux-dist-devel On Thu, 2010-01-07 at 23:54 -0500, Mike Frysinger wrote: > > @@ -60,11 +60,11 @@ static inline int atomic_add_return(int i, atomic_t > > *v) > > @@ -82,11 +82,11 @@ static inline int atomic_sub_return(int i, atomic_t > > *v) > > @@ -139,9 +139,9 @@ static inline void atomic_clear_mask(unsigned long > > mask, unsigned long *addr) > > looks like your mailer has line wrapping problems ... might want to > fix that. maybe just find a smtp server in ADI shanghai's office to > send to and use sendemail.smtpersver in your ~/.gitconfig ? > -mike > Thanks. Disabled auto-wrap and resend: "irqsoff" tracer may cause stack overflow on architectures using asm-generic/atomic.h, due to recursive invoking of trace_hardirqs_off(). trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() Signed-off-by: Yi Li <yi.li@analog.com> --- include/asm-generic/atomic.h | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h index c99c64d..c3e5f4b 100644 --- a/include/asm-generic/atomic.h +++ b/include/asm-generic/atomic.h @@ -60,11 +60,11 @@ static inline int atomic_add_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); temp = v->counter; temp += i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -82,11 +82,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + raw_local_irq_save(flags); temp = v->counter; temp -= i; v->counter = temp; - local_irq_restore(flags); + raw_local_irq_restore(flags); return temp; } @@ -139,9 +139,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) unsigned long flags; mask = ~mask; - local_irq_save(flags); + raw_local_irq_save(flags); *addr &= mask; - local_irq_restore(flags); + raw_local_irq_restore(flags); } #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 6:26 ` Li Yi @ 2010-01-08 6:26 ` Mike Frysinger 0 siblings, 0 replies; 8+ messages in thread From: Mike Frysinger @ 2010-01-08 6:26 UTC (permalink / raw) To: Li Yi; +Cc: Arnd Bergmann, rostedt, linux-kernel, uclinux-dist-devel On Fri, Jan 8, 2010 at 01:26, Li Yi wrote: > On Thu, 2010-01-07 at 23:54 -0500, Mike Frysinger wrote: >> > @@ -60,11 +60,11 @@ static inline int atomic_add_return(int i, atomic_t >> > *v) >> > @@ -82,11 +82,11 @@ static inline int atomic_sub_return(int i, atomic_t >> > *v) >> > @@ -139,9 +139,9 @@ static inline void atomic_clear_mask(unsigned long >> > mask, unsigned long *addr) >> >> looks like your mailer has line wrapping problems ... might want to >> fix that. maybe just find a smtp server in ADI shanghai's office to >> send to and use sendemail.smtpersver in your ~/.gitconfig ? > > Thanks. Disabled auto-wrap and resend: > > "irqsoff" tracer may cause stack overflow on architectures using asm-generic/atomic.h, due to recursive invoking of trace_hardirqs_off(). well now you have the opposite problem ... your changelog needs to be line wrapped -mike ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 4:45 [PATCH] Ftrace: irqsoff tracer may cause stack overflow Li Yi 2010-01-08 4:54 ` Mike Frysinger @ 2010-01-08 5:18 ` Frederic Weisbecker 2010-01-08 9:13 ` Li Yi 2010-01-08 15:22 ` Steven Rostedt 1 sibling, 2 replies; 8+ messages in thread From: Frederic Weisbecker @ 2010-01-08 5:18 UTC (permalink / raw) To: Li Yi; +Cc: rostedt, linux-kernel, uclinux-dist-devel On Fri, Jan 08, 2010 at 12:45:25PM +0800, Li Yi wrote: > "irqsoff" tracer may cause stack overflow on architectures using > asm-generic/atomic.h, due to recursive invoking of, e.g. > trace_hardirqs_off(). > > trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> > atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() > > Signed-off-by: Yi Li <yi.li@analog.com> Good catch! However, may be we should keep the local_irq_save there and have __raw_atomic_* versions only for tracing. It's better to keep track of most irq disabled sites. Why not something like the following (untested): diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h index c99c64d..ffc6772 100644 --- a/include/asm-generic/atomic.h +++ b/include/asm-generic/atomic.h @@ -17,6 +17,14 @@ #error not SMP safe #endif +#ifdef __ATOMIC_NEED_RAW_IRQ_SAVE +#define __atomic_op_irq_save(f) raw_local_irq_save(f) +#define __atomic_op_irq_restore(f) raw_local_irq_restore(f) +#else +#define __atomic_op_irq_save(f) local_irq_save(f) +#define __atomic_op_irq_restore(f) local_irq_restore(f) +#endif + /* * Atomic operations that C can't guarantee us. Useful for * resource counting etc.. @@ -60,11 +68,11 @@ static inline int atomic_add_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + __atomic_op_irq_save(flags); temp = v->counter; temp += i; v->counter = temp; - local_irq_restore(flags); + __atomic_op_irq_restore(flags); return temp; } @@ -82,11 +90,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) unsigned long flags; int temp; - local_irq_save(flags); + __atomic_op_irq_save(flags); temp = v->counter; temp -= i; v->counter = temp; - local_irq_restore(flags); + __atomic_op_irq_restore(flags); return temp; } @@ -139,9 +147,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) unsigned long flags; mask = ~mask; - local_irq_save(flags); + __atomic_op_irq_save(flags); *addr &= mask; - local_irq_restore(flags); + __atomic_op_irq_restore(flags); } #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c index 2974bc7..6bcb1d1 100644 --- a/kernel/trace/trace_irqsoff.c +++ b/kernel/trace/trace_irqsoff.c @@ -9,6 +9,9 @@ * Copyright (C) 2004-2006 Ingo Molnar * Copyright (C) 2004 William Lee Irwin III */ + +#define __ATOMIC_NEED_RAW_IRQ_SAVE + #include <linux/kallsyms.h> #include <linux/debugfs.h> #include <linux/uaccess.h> ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 5:18 ` Frederic Weisbecker @ 2010-01-08 9:13 ` Li Yi 2010-01-08 15:22 ` Steven Rostedt 1 sibling, 0 replies; 8+ messages in thread From: Li Yi @ 2010-01-08 9:13 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: rostedt, linux-kernel, uclinux-dist-devel On Fri, 2010-01-08 at 06:18 +0100, Frederic Weisbecker wrote: > On Fri, Jan 08, 2010 at 12:45:25PM +0800, Li Yi wrote: > > "irqsoff" tracer may cause stack overflow on architectures using > > asm-generic/atomic.h, due to recursive invoking of, e.g. > > trace_hardirqs_off(). > > > > trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> > > atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() > > > > Signed-off-by: Yi Li <yi.li@analog.com> > > > > Good catch! > > However, may be we should keep the local_irq_save there > and have __raw_atomic_* versions only for tracing. > > It's better to keep track of most irq disabled sites. > > Why not something like the following (untested): Yes. I agree this is better solution. -Yi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 5:18 ` Frederic Weisbecker 2010-01-08 9:13 ` Li Yi @ 2010-01-08 15:22 ` Steven Rostedt 2010-01-08 18:27 ` Frederic Weisbecker 1 sibling, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2010-01-08 15:22 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Li Yi, linux-kernel, uclinux-dist-devel On Fri, 2010-01-08 at 06:18 +0100, Frederic Weisbecker wrote: > On Fri, Jan 08, 2010 at 12:45:25PM +0800, Li Yi wrote: > > "irqsoff" tracer may cause stack overflow on architectures using > > asm-generic/atomic.h, due to recursive invoking of, e.g. > > trace_hardirqs_off(). > > > > trace_hardirqs_off() -> start_critical_timing() -> atomic_inc() -> > > atomic_add_return() -> local_irq_save() -> trace_hardirqs_off() > > > > Signed-off-by: Yi Li <yi.li@analog.com> > > > > Good catch! Yes, nice catch indeed. Hmm, I wonder if lockdep has any issues here as well? /me looks, no it uses current->lockdep_recursion++, where trace hardirqsoff uses atomic_inc_return :-/ > > However, may be we should keep the local_irq_save there > and have __raw_atomic_* versions only for tracing. > > It's better to keep track of most irq disabled sites. > > Why not something like the following (untested): > > > diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h > index c99c64d..ffc6772 100644 > --- a/include/asm-generic/atomic.h > +++ b/include/asm-generic/atomic.h > @@ -17,6 +17,14 @@ > #error not SMP safe > #endif Comment needed here: /* * The irqsoff tracer uses atomic_inc_return to prevent recursion. * Unfortunately, in this file, atomic_inc_return disables interrupts * which causes the recursion the irqsoff trace was trying to prevent. * * The irqsoff tracer will define __ATOMIC_NEED_RAW_IRQ_SAVE before * including this file, which will make the atomic_inc_return use * the raw versions of interrupts disabling. This will allow other * users of the atomic_inc_return to still have the interrupt * disabling be traced, but will prevent the recursion by the * irqsoff tracer itself. */ > > +#ifdef __ATOMIC_NEED_RAW_IRQ_SAVE > +#define __atomic_op_irq_save(f) raw_local_irq_save(f) > +#define __atomic_op_irq_restore(f) raw_local_irq_restore(f) > +#else > +#define __atomic_op_irq_save(f) local_irq_save(f) > +#define __atomic_op_irq_restore(f) local_irq_restore(f) > +#endif > + > /* > * Atomic operations that C can't guarantee us. Useful for > * resource counting etc.. > @@ -60,11 +68,11 @@ static inline int atomic_add_return(int i, atomic_t *v) > unsigned long flags; > int temp; > > - local_irq_save(flags); > + __atomic_op_irq_save(flags); > temp = v->counter; > temp += i; > v->counter = temp; > - local_irq_restore(flags); > + __atomic_op_irq_restore(flags); > > return temp; > } > @@ -82,11 +90,11 @@ static inline int atomic_sub_return(int i, atomic_t *v) > unsigned long flags; > int temp; > > - local_irq_save(flags); > + __atomic_op_irq_save(flags); > temp = v->counter; > temp -= i; > v->counter = temp; > - local_irq_restore(flags); > + __atomic_op_irq_restore(flags); > > return temp; > } > @@ -139,9 +147,9 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr) > unsigned long flags; > > mask = ~mask; > - local_irq_save(flags); > + __atomic_op_irq_save(flags); > *addr &= mask; > - local_irq_restore(flags); > + __atomic_op_irq_restore(flags); > } > > #define atomic_xchg(ptr, v) (xchg(&(ptr)->counter, (v))) > diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c > index 2974bc7..6bcb1d1 100644 > --- a/kernel/trace/trace_irqsoff.c > +++ b/kernel/trace/trace_irqsoff.c > @@ -9,6 +9,9 @@ > * Copyright (C) 2004-2006 Ingo Molnar > * Copyright (C) 2004 William Lee Irwin III > */ > + > +#define __ATOMIC_NEED_RAW_IRQ_SAVE > + > #include <linux/kallsyms.h> > #include <linux/debugfs.h> > #include <linux/uaccess.h> > I wonder if we could just use a per_cpu variable and increment that instead. Since the irqsoff tracer only gets called with interrupts disabled (and the preemptoff with preemption disabled), a per_cpu variable should be protected well. -- Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Ftrace: irqsoff tracer may cause stack overflow 2010-01-08 15:22 ` Steven Rostedt @ 2010-01-08 18:27 ` Frederic Weisbecker 0 siblings, 0 replies; 8+ messages in thread From: Frederic Weisbecker @ 2010-01-08 18:27 UTC (permalink / raw) To: Steven Rostedt; +Cc: Li Yi, linux-kernel, uclinux-dist-devel On Fri, Jan 08, 2010 at 10:22:43AM -0500, Steven Rostedt wrote: > On Fri, 2010-01-08 at 06:18 +0100, Frederic Weisbecker wrote: > Comment needed here: > > /* > * The irqsoff tracer uses atomic_inc_return to prevent recursion. > * Unfortunately, in this file, atomic_inc_return disables interrupts > * which causes the recursion the irqsoff trace was trying to prevent. > * > * The irqsoff tracer will define __ATOMIC_NEED_RAW_IRQ_SAVE before > * including this file, which will make the atomic_inc_return use > * the raw versions of interrupts disabling. This will allow other > * users of the atomic_inc_return to still have the interrupt > * disabling be traced, but will prevent the recursion by the > * irqsoff tracer itself. > */ > Yep, that was a first catch, just to ping opinions, it was even not tested :) > I wonder if we could just use a per_cpu variable and increment that > instead. Since the irqsoff tracer only gets called with interrupts > disabled (and the preemptoff with preemption disabled), a per_cpu > variable should be protected well. Doh! I thought about that but feared about preempt_disable recursion. I didn't realize this code was under such context already. True, that's indeed a much better idea! ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-01-08 18:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-01-08 4:45 [PATCH] Ftrace: irqsoff tracer may cause stack overflow Li Yi 2010-01-08 4:54 ` Mike Frysinger 2010-01-08 6:26 ` Li Yi 2010-01-08 6:26 ` Mike Frysinger 2010-01-08 5:18 ` Frederic Weisbecker 2010-01-08 9:13 ` Li Yi 2010-01-08 15:22 ` Steven Rostedt 2010-01-08 18:27 ` Frederic Weisbecker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.