[PATCH] Ftrace: irqsoff tracer may cause stack overflow

[Li Yi]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Mike Frysinger]

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

[Frederic Weisbecker]

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

[Li Yi]

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

[Mike Frysinger]

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

[Li Yi]

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

[Steven Rostedt]

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...@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.
*/

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

[Frederic Weisbecker]

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!