Skip to contentSkip to conversation

detect & print stack overruns at oops time

RFC PATCH In review Eric Sandeen 3 participants 7 messages Aug 31, 2007 – Sep 5, 2007

Subsystems (3)
MM core Scheduler exec and binfmt
lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

RFC PATCH 1/1 detect & print stack overruns at oops time

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 <[email protected]>
arch/i386/mm/fault.c +23 -0
Index: 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);
kernel/fork.c +3 -0
Index: 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();
#1 lore raw
Quoted 5 lines · On Fri, 31 Aug 2007 00:28:58 -0500 Eric Sandeen wrote:

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c no_context: · line 553

Quoted 9 lines
arch/i386/mm/fault.c +6 -0
@@ -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?

Quoted 8 lines
arch/i386/mm/fault.c +8 -0
+#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"

arch/i386/mm/fault.c no_context: · line 571

Quoted 10 lines
arch/i386/mm/fault.c +7 -0
+					" 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 ***

author
#2 lore raw
Quoted 19 lines · Randy Dunlap
Quoted 5 lines · On Fri, 31 Aug 2007 00:28:58 -0500 Eric Sandeen wrote:

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c no_context: · line 553

Quoted 9 lines
arch/i386/mm/fault.c +6 -0
@@ -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,

Quoted 2 lines

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

Sure, the rest of the oops report will.

-Eric

#3 lore raw
In reply to Eric Sandeen
Quoted 1 line · Eric Sandeen

In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought

...

Quoted 5 lines

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.

Quoted 22 lines
Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c +5 -0
@@ -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) {

arch/i386/mm/fault.c no_context: · line 560

Quoted 10 lines
arch/i386/mm/fault.c +10 -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?

arch/i386/mm/fault.c no_context: · line 565

Quoted 5 lines
arch/i386/mm/fault.c +5 -0
+				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.

author
#4 lore raw
Quoted 9 lines · Jarek Poplawski
Quoted 1 line · Eric Sandeen

In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought

...

Quoted 5 lines

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. :)

Quoted 3 lines

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.

Quoted 28 lines
Quoted 28 lines
Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c +11 -0
@@ -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;
Quoted 8 lines

arch/i386/mm/fault.c no_context: · line 560

Quoted 4 lines
arch/i386/mm/fault.c +4 -0
+			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");
Quoted 8 lines

arch/i386/mm/fault.c no_context: · line 565

Quoted 5 lines
arch/i386/mm/fault.c +5 -0
+				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

Quoted 3 lines

Regards,
Jarek P.

#5 lore raw
Quoted 14 lines · Eric Sandeen
Quoted 9 lines · Jarek Poplawski
Quoted 1 line · Eric Sandeen

In thinking about the 4KSTACKS + STACKOVERFLOW problems, I thought

...

Quoted 5 lines

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. :)

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 45 lines
Quoted 30 lines
Quoted 30 lines
Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6.22-rc4/arch/i386/mm/fault.c
===================================================================

arch/i386/mm/fault.c +11 -0
@@ -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;
Quoted 8 lines

arch/i386/mm/fault.c no_context: · line 560

Quoted 4 lines
arch/i386/mm/fault.c +4 -0
+			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:

Quoted 5 lines
	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.

Quoted 10 lines
Quoted 9 lines

arch/i386/mm/fault.c no_context: · line 565

Quoted 6 lines
arch/i386/mm/fault.c +5 -0
+				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.)

Quoted 8 lines

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.

#6 lore raw

On Wed, Sep 05, 2007 at 04:19:23PM +0200, Jarek Poplawski wrote:
...

Quoted 1 line

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.