On Sat, 2007-09-15 at 18:32 +0530, Satyam Sharma wrote:
> Hi,
>
>
> On 9/15/07, Gilboa Davara <[email protected]> wrote:
> > Hello all,
> >
> > In a small exchange in fedora-kernel-list [1] Eric Sandeen has pointed
> > out a possible stack overflow... when CONFIG_DEBUG_STACKOVERFLOW is
> > enabled. (Though not limited to it)
>
> Yeah, I have experienced this phenomenon/problem myself.
>
>
> > Code path is simple: do_IRQ detects a a near stack overflow condition
> > and calls show_trace_log_lvl which, down the line uses __print_symbol
> > and sprint_symbol to print the call stack.
> > However, both __print_symbol + sprint_symbol are eating no-less then
> > 128+223 bytes on static char arrays, which, given the fact that this
> > code path is actually generated by low stack warning (< 512 bytes),
> > might turn a minor (?) problem (low stack) into a full blown crash.
>
> __print_symbol() and sprint_symbol() are called multiple times during
> oopsen / panics. I think those buffers were static char arrays for a good
> reason ...
OK. Point taken.
I pull this patch pending some additional thinking.
> > The patch itself is fairly simple and non-intrusive. [2]
> > Both functions allocate memory for their buffers - falling back to
> > minimal address display if memory allocation fails.
> >
> > P.S. Can anyone please point me to the maintainer of kernel/syms? (I
> > rather not spam world + dog for such a minor patch)
>
> Anything that touches the panic codepath is important, not minor at all.
Bad wording on my part.
By minor I meant, changes a single source file, doesn't change
interfaces, doesn't change code behavior beyond it's local scope.
> > [2]. In theory, there's a second option: pre-allocating memory on a
> > per_cpu basis, however:
> > A. dump_trace/stack are usually called when something bad has happened -
> > reducing the need for performance optimizations.
>
> That's not a performance optimization -- avoiding repeated kmalloc()'s in the
> panic codepath sounds like a *requirement* to me.
ACK.
Though in my defense, solution [2] requires a massive surgery that would
have made this patch far more intrusive.
>
>
> > B. per_cpu allocation will also require local_irq_disable/enable as both
> > functions are being called from multiple contexts. Too much hassle.
>
> I think not bothering about any locking in these codepaths may not be an
> entirely unreasonable thing to do (sorry about the triple negation in the
> sentence). What I mean is that there are places in these codepaths where
> we already don't bother with locking ...
>
> Overall I don't much like introducing kmalloc(GFP_ATOMIC) in these codepaths
> and would ask you guys to consider some other pre-allocation (i.e. static
> allocation not on stack but in .data) alternative instead ...
>
> Satyam
No locking what-so-ever is a bad idea. dump_stack/trace are being called
by non-fatal sources (sleep while atomic; stack-check; debugging) that
may produce problematic results if a static/shared buffer is being used
with no locks.
We can agree that using in-stack char buffer is very problematic -
especially given the fact that 4K is becoming the default build option.
I'll try and create an option 2 (static allocation, minimal locking)
patch and post ASAP.
Hopefully it'll fare better. (While keeping the current interface intact
and reducing the damage/noise)
- Gilboa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
