* [PATCH] alternate stack dump fix.
@ 2004-10-03 16:00 Oleg Nesterov
2004-10-03 17:06 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2004-10-03 16:00 UTC (permalink / raw)
To: linux-kernel; +Cc: Kirill Korotaev, Andrew Morton, Linus Torvalds
Hello.
Kirill Korotaev wrote:
>
> This patch fixes incorrect check for stack ptr in
> show_trace()->valid_stack_ptr(). When called from hardirq/softirq
> show_trace() prints "Stack pointer is garbage, not printing trace"
> message instead of call traces.
There is another problem in show_trace(). With CONFIG_FRAME_POINTER
every call to print_context_stack() will now print entire call chain,
switching the stacks transparently, beacause valid_stack_ptr()
now accepts ebp in irq stack.
Then show trace switch the stack, and calls print_context_stack()
again with the same value in ebp, and we have the same dump
after printk(" =======================\n").
What do you think about the following patch?
Against 2.6.9-rc3.
Oleg.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- 2.6.9-rc3/arch/i386/kernel/traps.c~ Sun Oct 3 18:45:52 2004
+++ 2.6.9-rc3/arch/i386/kernel/traps.c Sun Oct 3 19:54:07 2004
@@ -107,36 +107,27 @@ int register_die_notifier(struct notifie
return err;
}
-static int valid_stack_ptr(struct task_struct *task, void *p)
+static int valid_stack_ptr(struct thread_info *tinfo, void *p)
{
- if (p <= (void *)task->thread_info)
- return 0;
- if (kstack_end(p))
- return 0;
- return 1;
+ return p > (void *)tinfo &&
+ p < (void *)tinfo + THREAD_SIZE - 3;
}
-#ifdef CONFIG_FRAME_POINTER
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
+static unsigned long print_context_stack(struct thread_info *tinfo, unsigned long *stack,
unsigned long ebp)
{
unsigned long addr;
- while (valid_stack_ptr(task, (void *)ebp)) {
+#ifdef CONFIG_FRAME_POINTER
+ while (valid_stack_ptr(tinfo, (void *)ebp)) {
addr = *(unsigned long *)(ebp + 4);
printk(" [<%08lx>] ", addr);
print_symbol("%s", addr);
printk("\n");
ebp = *(unsigned long *)ebp;
}
-}
#else
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
- unsigned long ebp)
-{
- unsigned long addr;
-
- while (!kstack_end(stack)) {
+ while (valid_stack_ptr(tinfo, stack)) {
addr = *stack++;
if (__kernel_text_address(addr)) {
printk(" [<%08lx>]", addr);
@@ -144,9 +135,11 @@ static void print_context_stack(struct t
printk("\n");
}
}
-}
#endif
+ return ebp;
+}
+
void show_trace(struct task_struct *task, unsigned long * stack)
{
unsigned long ebp;
@@ -154,11 +147,6 @@ void show_trace(struct task_struct *task
if (!task)
task = current;
- if (!valid_stack_ptr(task, stack)) {
- printk("Stack pointer is garbage, not printing trace\n");
- return;
- }
-
if (task == current) {
/* Grab ebp right from our regs */
asm ("movl %%ebp, %0" : "=r" (ebp) : );
@@ -171,7 +159,7 @@ void show_trace(struct task_struct *task
struct thread_info *context;
context = (struct thread_info *)
((unsigned long)stack & (~(THREAD_SIZE - 1)));
- print_context_stack(task, stack, ebp);
+ ebp = print_context_stack(context, stack, ebp);
stack = (unsigned long*)context->previous_esp;
if (!stack)
break;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] alternate stack dump fix.
2004-10-03 16:00 [PATCH] alternate stack dump fix Oleg Nesterov
@ 2004-10-03 17:06 ` Andrew Morton
2004-10-04 8:22 ` Oleg Nesterov
2004-10-04 8:22 ` Oleg Nesterov
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2004-10-03 17:06 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, dev, torvalds
Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> There is another problem in show_trace(). With CONFIG_FRAME_POINTER
> every call to print_context_stack() will now print entire call chain,
> switching the stacks transparently, beacause valid_stack_ptr()
> now accepts ebp in irq stack.
>
> Then show trace switch the stack, and calls print_context_stack()
> again with the same value in ebp, and we have the same dump
> after printk(" =======================\n").
>
> What do you think about the following patch?
>
> Against 2.6.9-rc3.
But it conflicts in a big way with Kirill's patch. Could you redo it
against 2.6.9-rc3-mm1, or against just
fix-of-stack-dump-in-soft-hardirqs.patch
fix-of-stack-dump-in-soft-hardirqs-cleanup.patch
fix-of-stack-dump-in-soft-hardirqs-build-fix.patch
from ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc3/2.6.9-rc3-mm1/broken-out?
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] alternate stack dump fix.
2004-10-03 17:06 ` Andrew Morton
@ 2004-10-04 8:22 ` Oleg Nesterov
2004-10-04 8:27 ` Andrew Morton
2004-10-04 8:22 ` Oleg Nesterov
1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2004-10-04 8:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, dev, torvalds
Andrew Morton wrote:
>
> But it conflicts in a big way with Kirill's patch. Could you redo it
> against 2.6.9-rc3-mm1, or against just
>
> fix-of-stack-dump-in-soft-hardirqs.patch
> fix-of-stack-dump-in-soft-hardirqs-cleanup.patch
> fix-of-stack-dump-in-soft-hardirqs-build-fix.patch
>
Rediffed against rc3-mm1.
For your convenience, i will post the same patch against mm tree with those
3 patches reverted in a separate message.
Oleg.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
diff -rup rc3-mm1-clean/arch/i386/kernel/irq.c rc3-mm1-trace/arch/i386/kernel/irq.c
--- rc3-mm1-clean/arch/i386/kernel/irq.c 2004-10-04 10:26:26.000000000 +0400
+++ rc3-mm1-trace/arch/i386/kernel/irq.c 2004-10-04 10:38:06.000000000 +0400
@@ -10,14 +10,12 @@
* io_apic.c.)
*/
+#include <asm/uaccess.h>
#include <linux/module.h>
#include <linux/seq_file.h>
#include <linux/interrupt.h>
#include <linux/kernel_stat.h>
-#include <asm/uaccess.h>
-#include <asm/hardirq.h>
-
#ifdef CONFIG_4KSTACKS
/*
* per-CPU IRQ handling contexts (thread information and stack)
@@ -178,23 +176,8 @@ asmlinkage void do_softirq(void)
local_irq_restore(flags);
}
-EXPORT_SYMBOL(do_softirq);
-
-int is_irq_stack_ptr(struct task_struct *task, void *p)
-{
- unsigned long off;
-
- off = task->thread_info->cpu * THREAD_SIZE;
- if (p >= (void *)hardirq_stack + off &&
- p < (void *)hardirq_stack + off + THREAD_SIZE)
- return 1;
- if (p >= (void *)softirq_stack + off &&
- p < (void *)softirq_stack + off + THREAD_SIZE)
- return 1;
-
- return 0;
-}
+EXPORT_SYMBOL(do_softirq);
#endif
/*
diff -rup rc3-mm1-clean/arch/i386/kernel/traps.c rc3-mm1-trace/arch/i386/kernel/traps.c
--- rc3-mm1-clean/arch/i386/kernel/traps.c 2004-10-04 10:26:26.000000000 +0400
+++ rc3-mm1-trace/arch/i386/kernel/traps.c 2004-10-04 10:55:03.000000000 +0400
@@ -50,7 +50,6 @@
#include <asm/smp.h>
#include <asm/arch_hooks.h>
#include <asm/kdebug.h>
-#include <asm/hardirq.h>
#include <linux/irq.h>
#include <linux/module.h>
@@ -106,18 +105,6 @@ int register_die_notifier(struct notifie
return err;
}
-static int valid_stack_ptr(struct task_struct *task, void *p)
-{
- if (is_irq_stack_ptr(task, p))
- return 1;
- if (p >= (void *)task->thread_info &&
- p < (void *)task->thread_info + THREAD_SIZE &&
- !kstack_end(p))
- return 1;
-
- return 0;
-}
-
#ifdef CONFIG_KGDB
extern void sysenter_past_esp(void);
#include <asm/kgdb.h>
@@ -151,28 +138,27 @@ void breakpoint(void)
#define CHK_REMOTE_DEBUG(trapnr,signr,error_code,regs,after)
#endif
+static int valid_stack_ptr(struct thread_info *tinfo, void *p)
+{
+ return p > (void *)tinfo &&
+ p < (void *)tinfo + THREAD_SIZE - 3;
+}
-#ifdef CONFIG_FRAME_POINTER
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
+static unsigned long print_context_stack(struct thread_info *tinfo, unsigned long *stack,
unsigned long ebp)
{
unsigned long addr;
- while (valid_stack_ptr(task, (void *)ebp)) {
+#ifdef CONFIG_FRAME_POINTER
+ while (valid_stack_ptr(tinfo, (void *)ebp)) {
addr = *(unsigned long *)(ebp + 4);
printk(" [<%08lx>] ", addr);
print_symbol("%s", addr);
printk("\n");
ebp = *(unsigned long *)ebp;
}
-}
#else
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
- unsigned long ebp)
-{
- unsigned long addr;
-
- while (!kstack_end(stack)) {
+ while (valid_stack_ptr(tinfo, stack)) {
addr = *stack++;
if (__kernel_text_address(addr)) {
printk(" [<%08lx>]", addr);
@@ -180,8 +166,9 @@ static void print_context_stack(struct t
printk("\n");
}
}
-}
#endif
+ return ebp;
+}
void show_trace(struct task_struct *task, unsigned long * stack)
{
@@ -190,11 +177,6 @@ void show_trace(struct task_struct *task
if (!task)
task = current;
- if (!valid_stack_ptr(task, stack)) {
- printk("Stack pointer is garbage, not printing trace\n");
- return;
- }
-
if (task == current) {
/* Grab ebp right from our regs */
asm ("movl %%ebp, %0" : "=r" (ebp) : );
@@ -207,7 +189,7 @@ void show_trace(struct task_struct *task
struct thread_info *context;
context = (struct thread_info *)
((unsigned long)stack & (~(THREAD_SIZE - 1)));
- print_context_stack(task, stack, ebp);
+ ebp = print_context_stack(context, stack, ebp);
stack = (unsigned long*)context->previous_esp;
if (!stack)
break;
diff -rup rc3-mm1-clean/include/asm-i386/hardirq.h rc3-mm1-trace/include/asm-i386/hardirq.h
--- rc3-mm1-clean/include/asm-i386/hardirq.h 2004-10-04 10:26:41.000000000 +0400
+++ rc3-mm1-trace/include/asm-i386/hardirq.h 2004-10-04 10:38:04.000000000 +0400
@@ -36,15 +36,4 @@ static inline void ack_bad_irq(unsigned
#endif
}
-struct task_struct;
-
-#ifdef CONFIG_4KSTACKS
-int is_irq_stack_ptr(struct task_struct *task, void *p);
-#else
-static inline int is_irq_stack_ptr(struct task_struct *task, void *p)
-{
- return 0;
-}
-#endif
-
#endif /* __ASM_HARDIRQ_H */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] alternate stack dump fix.
2004-10-03 17:06 ` Andrew Morton
2004-10-04 8:22 ` Oleg Nesterov
@ 2004-10-04 8:22 ` Oleg Nesterov
1 sibling, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2004-10-04 8:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, dev, torvalds
Oleg Nesterov wrote:
>
> > Andrew Morton wrote:
> >
> > But it conflicts in a big way with Kirill's patch. Could you redo it
> > against 2.6.9-rc3-mm1, or against just
>
> For your convenience, i will post the same patch against mm tree with those
> 3 patches reverted in a separate message.
Against 2.6.9-rc3-mm1 +
-R fix-of-stack-dump-in-soft-hardirqs-build-fix.patch +
-R fix-of-stack-dump-in-soft-hardirqs-cleanup.patch +
-R fix-of-stack-dump-in-soft-hardirqs.patch
Oleg.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- rc3-mm1/arch/i386/kernel/traps.c~ Mon Oct 4 10:46:10 2004
+++ rc3-mm1/arch/i386/kernel/traps.c Mon Oct 4 10:56:22 2004
@@ -105,15 +105,6 @@ int register_die_notifier(struct notifie
return err;
}
-static int valid_stack_ptr(struct task_struct *task, void *p)
-{
- if (p <= (void *)task->thread_info)
- return 0;
- if (kstack_end(p))
- return 0;
- return 1;
-}
-
#ifdef CONFIG_KGDB
extern void sysenter_past_esp(void);
#include <asm/kgdb.h>
@@ -147,28 +138,27 @@ void breakpoint(void)
#define CHK_REMOTE_DEBUG(trapnr,signr,error_code,regs,after)
#endif
+static int valid_stack_ptr(struct thread_info *tinfo, void *p)
+{
+ return p > (void *)tinfo &&
+ p < (void *)tinfo + THREAD_SIZE - 3;
+}
-#ifdef CONFIG_FRAME_POINTER
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
+static unsigned long print_context_stack(struct thread_info *tinfo, unsigned long *stack,
unsigned long ebp)
{
unsigned long addr;
- while (valid_stack_ptr(task, (void *)ebp)) {
+#ifdef CONFIG_FRAME_POINTER
+ while (valid_stack_ptr(tinfo, (void *)ebp)) {
addr = *(unsigned long *)(ebp + 4);
printk(" [<%08lx>] ", addr);
print_symbol("%s", addr);
printk("\n");
ebp = *(unsigned long *)ebp;
}
-}
#else
-static void print_context_stack(struct task_struct *task, unsigned long *stack,
- unsigned long ebp)
-{
- unsigned long addr;
-
- while (!kstack_end(stack)) {
+ while (valid_stack_ptr(tinfo, stack)) {
addr = *stack++;
if (__kernel_text_address(addr)) {
printk(" [<%08lx>]", addr);
@@ -176,8 +166,9 @@ static void print_context_stack(struct t
printk("\n");
}
}
-}
#endif
+ return ebp;
+}
void show_trace(struct task_struct *task, unsigned long * stack)
{
@@ -186,11 +177,6 @@ void show_trace(struct task_struct *task
if (!task)
task = current;
- if (!valid_stack_ptr(task, stack)) {
- printk("Stack pointer is garbage, not printing trace\n");
- return;
- }
-
if (task == current) {
/* Grab ebp right from our regs */
asm ("movl %%ebp, %0" : "=r" (ebp) : );
@@ -203,7 +189,7 @@ void show_trace(struct task_struct *task
struct thread_info *context;
context = (struct thread_info *)
((unsigned long)stack & (~(THREAD_SIZE - 1)));
- print_context_stack(task, stack, ebp);
+ ebp = print_context_stack(context, stack, ebp);
stack = (unsigned long*)context->previous_esp;
if (!stack)
break;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] alternate stack dump fix.
2004-10-04 8:22 ` Oleg Nesterov
@ 2004-10-04 8:27 ` Andrew Morton
2004-10-04 9:28 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2004-10-04 8:27 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-kernel, dev, torvalds
Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> For your convenience, i will post the same patch against mm tree with those
> 3 patches reverted in a separate message.
In that case I'm not understanding you. Are you saying that your patch
fixes the same problem as that which Kirill's patch fixed? That wasn't at
all clear from your earlier comments.
If so, please describe precisely what problems your patch fixes, and how.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] alternate stack dump fix.
2004-10-04 8:27 ` Andrew Morton
@ 2004-10-04 9:28 ` Oleg Nesterov
0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2004-10-04 9:28 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, dev, torvalds
Andrew Morton wrote:
>
> In that case I'm not understanding you. Are you saying that your patch
> fixes the same problem as that which Kirill's patch fixed?
It seems to me, yes.
Here the pseudo code for CONFIG_FRAME_POINTER:
int valid_stack_ptr(struct thread_info *tinfo, void *p)
{
return p > (void *)tinfo &&
p < (void *)tinfo + THREAD_SIZE - 3;
}
long print_context_stack(thread_info *tinfo, long *stack, long ebp)
{
while (valid_stack_ptr(tinfo, ebp))
{
print_symbol("%s", *(ebp+4));
ebp = *(unsigned long *)ebp;
}
return ebp;
}
void show_trace(struct task_struct *task, unsigned long * stack)
{
while (1) {
struct thread_info *context = (struct thread_info *)
((unsigned long)stack & (~(THREAD_SIZE - 1)));
ebp = print_context_stack(context, stack, ebp);
stack = (unsigned long*)context->previous_esp;
if (!stack)
break;
printk(" =======================\n");
}
}
show_trace() now does not use task argument in the main
loop. Instead, it converts stack to thread_info* context,
and passes it to print_context_stack() and (implicitly)
to valid_stack_ptr().
valid_stack_ptr() does not care now whether or not it is
irq stack, it just does bounds checking.
Please note, i simply deleted this printk("Stack pointer is garbage"),
it can be restored, if neccessary.
Did i miss something?
> That wasn't at all clear from your earlier comments.
Yes, sorry.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-10-04 9:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-10-03 16:00 [PATCH] alternate stack dump fix Oleg Nesterov
2004-10-03 17:06 ` Andrew Morton
2004-10-04 8:22 ` Oleg Nesterov
2004-10-04 8:27 ` Andrew Morton
2004-10-04 9:28 ` Oleg Nesterov
2004-10-04 8:22 ` Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®