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

[Eric Sandeen <sandeen@redhat.com>]

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.