[PATCH] show_task() and thread_saved_pc() fix for x86

[PATCH] show_task() and thread_saved_pc() fix for x86

[Alexander Viro]

	* thread_saved_pc() on x86 returns (thread->esp)[3]. Bogus, since the
third word from the stack top has absolutely nothing to return address of
any kind. Correct value: (thread->esp)[0][1] - ebp is on top of the stack
and the rest is obvious. Current code gives completely bogus addresses -
try to say Alt-SysRq-T and watch the show.

	* kernel/sched::show_state() used the wrong check for process being
on some CPU - p==current is nice for UP, but on SMP it's obviously wrong.
Moroever, nothing prevents schedule() another CPU while we are going through
the process state. Fix: show_state() grabs runqueue_lock, show_task() checks
p->has_cpu instead of p == current.

	Please, apply.
							Cheers,
								Al

diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
--- rc11-2/include/asm-i386/processor.h	Fri Nov 10 09:14:04 2000
+++ rc11-2-show_task/include/asm-i386/processor.h	Fri Nov 10 16:08:15 2000
@@ -412,7 +412,7 @@
  */
 extern inline unsigned long thread_saved_pc(struct thread_struct *t)
 {
-	return ((unsigned long *)t->esp)[3];
+	return ((unsigned long **)t->esp)[0][1];
 }
 
 unsigned long get_wchan(struct task_struct *p);
diff -urN rc11-2/kernel/sched.c rc11-2-show_task/kernel/sched.c
--- rc11-2/kernel/sched.c	Fri Nov 10 09:18:43 2000
+++ rc11-2-show_task/kernel/sched.c	Fri Nov 10 16:11:39 2000
@@ -1121,12 +1121,12 @@
 	else
 		printk(" ");
 #if (BITS_PER_LONG == 32)
-	if (p == current)
+	if (p->has_cpu)
 		printk(" current  ");
 	else
 		printk(" %08lX ", thread_saved_pc(&p->thread));
 #else
-	if (p == current)
+	if (p->has_cpu)
 		printk("   current task   ");
 	else
 		printk(" %016lx ", thread_saved_pc(&p->thread));
@@ -1186,6 +1186,7 @@
 void show_state(void)
 {
 	struct task_struct *p;
+	unsigned long flags;
 
 #if (BITS_PER_LONG == 32)
 	printk("\n"
@@ -1196,10 +1197,12 @@
 	       "                                 free                        sibling\n");
 	printk("  task                 PC        stack   pid father child younger older\n");
 #endif
+	spin_lock_irqsave(&runqueue_lock, flags);
 	read_lock(&tasklist_lock);
 	for_each_task(p)
 		show_task(p);
 	read_unlock(&tasklist_lock);
+	spin_unlock_irqrestore(&runqueue_lock, flags);
 }
 
 /*

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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Ralf Baechle]

On Fri, Nov 10, 2000 at 04:26:32PM -0500, Alexander Viro wrote:

> 	* thread_saved_pc() on x86 returns (thread->esp)[3]. Bogus, since the
> third word from the stack top has absolutely nothing to return address of
> any kind. Correct value: (thread->esp)[0][1] - ebp is on top of the stack
> and the rest is obvious. Current code gives completely bogus addresses -
> try to say Alt-SysRq-T and watch the show.

Reminds me that the Alpha implementation of get_wchan() looks to me like
it doesn't handle all cases of schedule() being called from another
scheduler function correctly.  Some Alpha guru may want to take a look at
it.

I recently had to fix the mips / mips64 versions of get_wchan() - for the
dozenth time.  I'd really like to see a wchan field in task_struct to avoid
get_wchan breaking every once in a while.  Current implementation more than
qualifies as a crazy hack ...

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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Linus Torvalds]

On Fri, 10 Nov 2000, Alexander Viro wrote:
> diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
> --- rc11-2/include/asm-i386/processor.h	Fri Nov 10 09:14:04 2000
> +++ rc11-2-show_task/include/asm-i386/processor.h	Fri Nov 10 16:08:15 2000
> @@ -412,7 +412,7 @@
>   */
>  extern inline unsigned long thread_saved_pc(struct thread_struct *t)
>  {
> -	return ((unsigned long *)t->esp)[3];
> +	return ((unsigned long **)t->esp)[0][1];
>  }

The above needs to get verified: it should be something like

	unsigned long *ebp = *((unsigned long **)t->esp);

	if ((void *) ebp < (void *) t)
		return 0;
	if ((void *) ebp >= (void *) t + 2*PAGE_SIZE)
		return 0;
	if (3 & (unsigned long)ebp)
		return 0;
	return *ebp;

because otherwise I guarantee that we'll eventually have a bug with a
invalid pointer reference in the debugging code and that would be bad.

		Linus

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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Alexander Viro]

On Sat, 11 Nov 2000, Linus Torvalds wrote:

> On Fri, 10 Nov 2000, Alexander Viro wrote:
> > diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
> > --- rc11-2/include/asm-i386/processor.h	Fri Nov 10 09:14:04 2000
> > +++ rc11-2-show_task/include/asm-i386/processor.h	Fri Nov 10 16:08:15 2000
> > @@ -412,7 +412,7 @@
> >   */
> >  extern inline unsigned long thread_saved_pc(struct thread_struct *t)
> >  {
> > -	return ((unsigned long *)t->esp)[3];
> > +	return ((unsigned long **)t->esp)[0][1];
> >  }
> 
> The above needs to get verified: it should be something like
> 
> 	unsigned long *ebp = *((unsigned long **)t->esp);
> 
> 	if ((void *) ebp < (void *) t)
> 		return 0;
> 	if ((void *) ebp >= (void *) t + 2*PAGE_SIZE)
> 		return 0;
> 	if (3 & (unsigned long)ebp)
> 		return 0;
> 	return *ebp;
> 
> because otherwise I guarantee that we'll eventually have a bug with a
> invalid pointer reference in the debugging code and that would be bad.

I would probably turn it into
	unsigned long *ebp = *((unsigned long **)t->esp);

	/* Bits 0,1 and 13..31 must be shared with the stack base */
	if (((unsigned long)ebp ^ (unsigned long)t) & ~(2*PAGE_SIZE-4))
		return 0;

	return *ebp;

Comments? Alternative variant: just let schedule() store its return address
in the task_struct. Yeah, it's couple of tacts per schedule(). And much saner
code, without second-guessing the compiler. OTOH, the value is used only
by Alt-SysRq-T, so... Hell knows.
								Cheers,
									Al

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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Alexander Viro]

On Sat, 11 Nov 2000, Alexander Viro wrote:

> I would probably turn it into
> 	unsigned long *ebp = *((unsigned long **)t->esp);

	ebp++;	/* We want return address, not the previous frame pointer */

> 	/* Bits 0,1 and 13..31 must be shared with the stack base */
> 	if (((unsigned long)ebp ^ (unsigned long)t) & ~(2*PAGE_SIZE-4))
> 		return 0;
> 
> 	return *ebp;

Sorry.
								Cheers,
									Al

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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Richard Henderson]

On Sat, Nov 11, 2000 at 02:06:34PM +0100, Ralf Baechle wrote:
> Reminds me that the Alpha implementation of get_wchan() looks to me like
> it doesn't handle all cases of schedule() being called from another
> scheduler function correctly.

Certainly not -- it's impossible.

> I'd really like to see a wchan field in task_struct to avoid
> get_wchan breaking every once in a while.

Indeed.


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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Richard Henderson]

On Sat, Nov 11, 2000 at 10:18:46PM -0500, Alexander Viro wrote:
> Alternative variant: just let schedule() store its return address
> in the task_struct.

Please please.  I can't reliably unwind the stack on Alpha.

> OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.

No, it's also used by 'ps -l'.  See wchan.


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

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Jean Wolter]

Richard Henderson <rth@twiddle.net> writes:

> > OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.
> 
> No, it's also used by 'ps -l'.  See wchan.

ps -l uses get_wchan() (an architecture specific function from
arch/*/kernel/process.c) to get the return address from
schedule(). And now thread_saved_pc() seems to do the same (at least
on x86). Is there any reason to have two architecture specific
functions doing the same or do I miss something?

Jean

PS: Architectures other then x86 use thread_saved_pc() to implement
get_wchan(). If the debug output of Alt-SysRq-T is supposed to show
the waiting channel we should use get_wchan() instead of thread_saved_pc().
-- 
I get up each morning, gather my wits.
Pick up the paper, read the obits.
if I'm not there I know I'm not dead.
So I eat a good breakfast and go back to bed. Peete Seeger
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] show_task() and thread_saved_pc() fix for x86

[Ralf Baechle]

On Tue, Nov 14, 2000 at 10:19:32AM +0100, Jean Wolter wrote:

> > > OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.
> > 
> > No, it's also used by 'ps -l'.  See wchan.
> 
> ps -l uses get_wchan() (an architecture specific function from
> arch/*/kernel/process.c) to get the return address from
> schedule(). And now thread_saved_pc() seems to do the same (at least
> on x86). Is there any reason to have two architecture specific
> functions doing the same or do I miss something?
> 
> Jean
> 
> PS: Architectures other then x86 use thread_saved_pc() to implement
> get_wchan(). If the debug output of Alt-SysRq-T is supposed to show
> the waiting channel we should use get_wchan() instead of thread_saved_pc().

Probably historic reasons, it's been that way as long as I can think back.
Yet the use of thread_saved_pc() in kernel/sched.c should imho be considered
a buglet and be replaced by get_wchan to get more meaningful debugging
information.

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