[RFC][PATCH] detect & print stack overruns at oops time

[RFC][PATCH] detect & print stack overruns at oops time

[Eric Sandeen]

In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought
about this - if an oops occurs, explicitly print whether the current
esp is now overrunning the stack, whether the thread has ever overrun
the stack, else print the max stack excursion for the oopsing thread,
if DEBUG_STACK_USAGE is enabled.

1) maybe printing the info for a non-blown stack is not useful..
2) current esp past end of stack can already be deduced, but this
   makes it more plain...
3) if the initialization of end_of_stack is problematic for
   any reason, it could be removed and the "did we ever overrun"
   test could be moved under the DEBUG_STACK_USAGE ifdef

Thoughts?  This is a separate problem from the piggy dump_stack()
path, but it seems to me it might be useful in looking at stack-related
oopses when they do occur.  With this change, it seems feasible
to turn off DEBUG_STACKOVERFLOW, turn on DEBUG_STACK_USAGE, and just
get the bad news when it's actually happened.  :) 

Thanks,
-Eric

Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================
--- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
+++ linux-2.6.22-rc4/arch/i386/mm/fault.c
@@ -525,6 +525,8 @@ no_context:
 
 	if (oops_may_print()) {
 		__typeof__(pte_val(__pte(0))) page;
+		unsigned long *stackend = end_of_stack(tsk);
+		int overrun;
 
 #ifdef CONFIG_X86_PAE
 		if (error_code & 16) {
@@ -543,6 +545,27 @@ no_context:
 			printk(KERN_ALERT "BUG: unable to handle kernel paging"
 					" request");
 		printk(" at virtual address %08lx\n",address);
+
+		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
+		if (overrun > 0) {
+			printk(KERN_ALERT "Thread overrunning stack by %d "
+							"bytes\n", overrun);
+		} else {
+#ifdef CONFIG_DEBUG_STACK_USAGE
+			int free;
+			unsigned long *n = stackend;
+			while (!*n)
+				n++;
+			free = (unsigned long)n - (unsigned long)stackend;
+			if (free)
+				printk(KERN_ALERT "Thread used within %d bytes"
+					" of stack end\n", free);
+#endif
+			/* won't catch 100% - stack may have 0s here by chance */
+			if (*stackend)  /* was init'd to 0 */
+				printk(KERN_ALERT "Thread overran the stack?\n");
+		}
+
 		printk(KERN_ALERT " printing eip:\n");
 		printk("%08lx\n", regs->eip);
 
Index: linux-2.6.22-rc4/kernel/fork.c
===================================================================
--- linux-2.6.22-rc4.orig/kernel/fork.c
+++ linux-2.6.22-rc4/kernel/fork.c
@@ -163,6 +163,7 @@ static struct task_struct *dup_task_stru
 {
 	struct task_struct *tsk;
 	struct thread_info *ti;
+	unsigned long *stackend;
 
 	prepare_to_copy(orig);
 
@@ -179,6 +180,8 @@ static struct task_struct *dup_task_stru
 	*tsk = *orig;
 	tsk->stack = ti;
 	setup_thread_stack(tsk, orig);
+	stackend = end_of_stack(tsk);
+	*stackend = 0;	/* for overflow detection */
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 	tsk->stack_canary = get_random_int();

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Randy Dunlap]

On Fri, 31 Aug 2007 00:28:58 -0500 Eric Sandeen wrote:

> Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
> +++ linux-2.6.22-rc4/arch/i386/mm/fault.c
> @@ -543,6 +545,27 @@ no_context:
>  			printk(KERN_ALERT "BUG: unable to handle kernel paging"
>  					" request");
>  		printk(" at virtual address %08lx\n",address);
> +
> +		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
> +		if (overrun > 0) {
> +			printk(KERN_ALERT "Thread overrunning stack by %d "
> +							"bytes\n", overrun);
> +		} else {

Hi,

Is there something that tells us what <Thread> is doing all of this
bad juju?

> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +			int free;
> +			unsigned long *n = stackend;
> +			while (!*n)
> +				n++;
> +			free = (unsigned long)n - (unsigned long)stackend;
> +			if (free)
> +				printk(KERN_ALERT "Thread used within %d bytes"
> +					" of stack end\n", free);
> +#endif
> +			/* won't catch 100% - stack may have 0s here by chance */
> +			if (*stackend)  /* was init'd to 0 */
> +				printk(KERN_ALERT "Thread overran the stack?\n");
> +		}
> +
>  		printk(KERN_ALERT " printing eip:\n");
>  		printk("%08lx\n", regs->eip);
>  

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Eric Sandeen]

Randy Dunlap wrote:
> On Fri, 31 Aug 2007 00:28:58 -0500 Eric Sandeen wrote:
> 
>> Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
>> ===================================================================
>> --- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
>> +++ linux-2.6.22-rc4/arch/i386/mm/fault.c
>> @@ -543,6 +545,27 @@ no_context:
>>  			printk(KERN_ALERT "BUG: unable to handle kernel paging"
>>  					" request");
>>  		printk(" at virtual address %08lx\n",address);
>> +
>> +		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
>> +		if (overrun > 0) {
>> +			printk(KERN_ALERT "Thread overrunning stack by %d "
>> +							"bytes\n", overrun);
>> +		} else {
> 
> Hi,
> 
> Is there something that tells us what <Thread> is doing all of this
> bad juju?

Sure, the rest of the oops report will.

-Eric

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Jarek Poplawski]

On 31-08-2007 07:28, Eric Sandeen wrote:
> In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought
...
> Thoughts?  This is a separate problem from the piggy dump_stack()
> path, but it seems to me it might be useful in looking at stack-related
> oopses when they do occur.  With this change, it seems feasible
> to turn off DEBUG_STACKOVERFLOW, turn on DEBUG_STACK_USAGE, and just
> get the bad news when it's actually happened.  :) 

Very good idea, but maybe, at least for some time, it should be with
ifdef CONFIG_4KSTACKS, to check if it's really needed if some other
similar checks also set.

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
> +++ linux-2.6.22-rc4/arch/i386/mm/fault.c
> @@ -525,6 +525,8 @@ no_context:
>  
>  	if (oops_may_print()) {
>  		__typeof__(pte_val(__pte(0))) page;
> +		unsigned long *stackend = end_of_stack(tsk);
> +		int overrun;
>  
>  #ifdef CONFIG_X86_PAE
>  		if (error_code & 16) {
> @@ -543,6 +545,27 @@ no_context:
>  			printk(KERN_ALERT "BUG: unable to handle kernel paging"
>  					" request");
>  		printk(" at virtual address %08lx\n",address);
> +
> +		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
> +		if (overrun > 0) {
> +			printk(KERN_ALERT "Thread overrunning stack by %d "
> +							"bytes\n", overrun);
> +		} else {
> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +			int free;
> +			unsigned long *n = stackend;
> +			while (!*n)
> +				n++;
> +			free = (unsigned long)n - (unsigned long)stackend;
> +			if (free)

Maybe there should be some min 'free' and max number of printks? There
could be also considered if, with some minimal values of 'free', prink
is the best thing we can do before stack overruning?

> +				printk(KERN_ALERT "Thread used within %d bytes"
> +					" of stack end\n", free);
> +#endif
> +			/* won't catch 100% - stack may have 0s here by chance */
> +			if (*stackend)  /* was init'd to 0 */

Isn't a MAGIC number better for this? (Then of course above n should
start a bit higher.)

Regards,
Jarek P.

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Eric Sandeen]

Jarek Poplawski wrote:
> On 31-08-2007 07:28, Eric Sandeen wrote:
>> In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought
> ...
>> Thoughts?  This is a separate problem from the piggy dump_stack()
>> path, but it seems to me it might be useful in looking at stack-related
>> oopses when they do occur.  With this change, it seems feasible
>> to turn off DEBUG_STACKOVERFLOW, turn on DEBUG_STACK_USAGE, and just
>> get the bad news when it's actually happened.  :) 
> 
> Very good idea, but maybe, at least for some time, it should be with
> ifdef CONFIG_4KSTACKS, to check if it's really needed if some other
> similar checks also set.

Hi Jarek - thanks for the comments.

I don't see much, if any, runtime impact to having it on all the time,
except maybe the end-of-stack zeroing, which would have at least some
impact.  Everything except that single " = 0;" only runs if you've
already oopsed...  I'd hate to clutter it with #ifdefs unless there's a
good reason.


>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>
>> Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
>> ===================================================================
>> --- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
>> +++ linux-2.6.22-rc4/arch/i386/mm/fault.c
>> @@ -525,6 +525,8 @@ no_context:
>>  
>>  	if (oops_may_print()) {
>>  		__typeof__(pte_val(__pte(0))) page;
>> +		unsigned long *stackend = end_of_stack(tsk);
>> +		int overrun;
>>  
>>  #ifdef CONFIG_X86_PAE
>>  		if (error_code & 16) {
>> @@ -543,6 +545,27 @@ no_context:
>>  			printk(KERN_ALERT "BUG: unable to handle kernel paging"
>>  					" request");
>>  		printk(" at virtual address %08lx\n",address);
>> +
>> +		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
>> +		if (overrun > 0) {
>> +			printk(KERN_ALERT "Thread overrunning stack by %d "
>> +							"bytes\n", overrun);
>> +		} else {
>> +#ifdef CONFIG_DEBUG_STACK_USAGE
>> +			int free;
>> +			unsigned long *n = stackend;
>> +			while (!*n)
>> +				n++;
>> +			free = (unsigned long)n - (unsigned long)stackend;
>> +			if (free)
> 
> Maybe there should be some min 'free' and max number of printks? There
> could be also considered if, with some minimal values of 'free', prink
> is the best thing we can do before stack overruning?

You mean, don't print unless the free value is in some range?  My
thought was, if you always print *something*, then you know for sure
you're looking at an oops from a kernel with this code in place.
Though, if "all's well" I suppose the bytes remaining isn't so
important; it would really be enough to just say:

	if (!(*stackend))
		printk("Thread stayed within stack space\n");
	else
		printk("Thread overran stack\n");


>> +				printk(KERN_ALERT "Thread used within %d bytes"
>> +					" of stack end\n", free);
>> +#endif
>> +			/* won't catch 100% - stack may have 0s here by chance */
>> +			if (*stackend)  /* was init'd to 0 */
> 
> Isn't a MAGIC number better for this? (Then of course above n should
> start a bit higher.)

I thought about that, though really *anything* could legitimately be on
the stack, so I guess it's a question of probabilities.  And, "0" is
compatible with CONFIG_DEBUG_STACK_USAGE, which sets the the whole stack
to 0 and uses that to detect max stack excursion...  I guess that option
could probably be tweaked to zero the whole stack, and then also set the
last position to something magic (0xDEAD57AC - deadstack?), and check
for that as well.  (I had thought about making helper functions for
DEBUG_STACK_OVERFLOW, right now the checks are open coded in a few places).

Thanks,

-Eric

> 
> Regards,
> Jarek P.

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Jarek Poplawski]

On Wed, Sep 05, 2007 at 07:51:16AM -0500, Eric Sandeen wrote:
> Jarek Poplawski wrote:
> > On 31-08-2007 07:28, Eric Sandeen wrote:
> >> In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought
> > ...
> >> Thoughts?  This is a separate problem from the piggy dump_stack()
> >> path, but it seems to me it might be useful in looking at stack-related
> >> oopses when they do occur.  With this change, it seems feasible
> >> to turn off DEBUG_STACKOVERFLOW, turn on DEBUG_STACK_USAGE, and just
> >> get the bad news when it's actually happened.  :) 
> > 
> > Very good idea, but maybe, at least for some time, it should be with
> > ifdef CONFIG_4KSTACKS, to check if it's really needed if some other
> > similar checks also set.
> 
> Hi Jarek - thanks for the comments.
> 
> I don't see much, if any, runtime impact to having it on all the time,
> except maybe the end-of-stack zeroing, which would have at least some
> impact.  Everything except that single " = 0;" only runs if you've
> already oopsed...  I'd hate to clutter it with #ifdefs unless there's a
> good reason.

I've meant: with 4KSTACKS there is probably no doubt it's needed, even
at the cost of some impact, which by the way could be more tested (and
there is a way to turn it off for, maybe, some conflicts). But, if you
checked it all enough and think such 'debugging' time is not necessary
then of course, without ifdefs it looks nicer.

> 
> 
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>
> >> Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
> >> ===================================================================
> >> --- linux-2.6.22-rc4.orig/arch/i386/mm/fault.c
> >> +++ linux-2.6.22-rc4/arch/i386/mm/fault.c
> >> @@ -525,6 +525,8 @@ no_context:
> >>  
> >>  	if (oops_may_print()) {
> >>  		__typeof__(pte_val(__pte(0))) page;
> >> +		unsigned long *stackend = end_of_stack(tsk);
> >> +		int overrun;
> >>  
> >>  #ifdef CONFIG_X86_PAE
> >>  		if (error_code & 16) {
> >> @@ -543,6 +545,27 @@ no_context:
> >>  			printk(KERN_ALERT "BUG: unable to handle kernel paging"
> >>  					" request");
> >>  		printk(" at virtual address %08lx\n",address);
> >> +
> >> +		overrun = (unsigned long)stackend - (unsigned long)(&regs->esp);
> >> +		if (overrun > 0) {
> >> +			printk(KERN_ALERT "Thread overrunning stack by %d "
> >> +							"bytes\n", overrun);
> >> +		} else {
> >> +#ifdef CONFIG_DEBUG_STACK_USAGE
> >> +			int free;
> >> +			unsigned long *n = stackend;
> >> +			while (!*n)
> >> +				n++;
> >> +			free = (unsigned long)n - (unsigned long)stackend;
> >> +			if (free)
> > 
> > Maybe there should be some min 'free' and max number of printks? There
> > could be also considered if, with some minimal values of 'free', prink
> > is the best thing we can do before stack overruning?
> 
> You mean, don't print unless the free value is in some range?  My
> thought was, if you always print *something*, then you know for sure
> you're looking at an oops from a kernel with this code in place.
> Though, if "all's well" I suppose the bytes remaining isn't so
> important; it would really be enough to just say:
> 
> 	if (!(*stackend))
> 		printk("Thread stayed within stack space\n");
> 	else
> 		printk("Thread overran stack\n");
> 

OOPS! I didn't read the code around enough. Since it's one time event
there should be no problem of too much repeating, sorry! But, there
is probably still a question: if such a printk can overrun the stack
and possibly stuck the machine I would prefer to have some log on the
disk or possibility of sysrq or ctrl-alt-del than to write by hand or
take a picture with this one printk more... Of course, if I missed it
again, and there is choice or risk, then - no question & all correct.

> 
> >> +				printk(KERN_ALERT "Thread used within %d bytes"
> >> +					" of stack end\n", free);
> >> +#endif
> >> +			/* won't catch 100% - stack may have 0s here by chance */
> >> +			if (*stackend)  /* was init'd to 0 */
> > 
> > Isn't a MAGIC number better for this? (Then of course above n should
> > start a bit higher.)
> 
> I thought about that, though really *anything* could legitimately be on
> the stack, so I guess it's a question of probabilities.  And, "0" is
> compatible with CONFIG_DEBUG_STACK_USAGE, which sets the the whole stack
> to 0 and uses that to detect max stack excursion...  I guess that option
> could probably be tweaked to zero the whole stack, and then also set the
> last position to something magic (0xDEAD57AC - deadstack?), and check
> for that as well.  (I had thought about making helper functions for
> DEBUG_STACK_OVERFLOW, right now the checks are open coded in a few places).

IMHO 0 is the worst possible choice for this - I think it's used far
more often than others, there is only a question to stay compatible
with other debug checkers (or ifdef from them...), but anything else
looks much more credible to me.

Thanks,
Jarek P.

Re: [RFC][PATCH] detect & print stack overruns at oops time

[Jarek Poplawski]

On Wed, Sep 05, 2007 at 04:19:23PM +0200, Jarek Poplawski wrote:
...
> again, and there is choice or risk, then - no question & all correct.

Should be:
again, and there is no choice or risk, then - no question & all correct.

Jarek P.