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 ...
> 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.
> Gilboa Davara <[email protected]>
>
> [1]
> http://www.mail-archive.com/[email protected]/msg00640.html
>
> [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.
> 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
> --- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.000000000 +0300
> +++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.000000000 +0300
> @@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
> /* Look up a kernel symbol and return it in a text buffer. */
> int sprint_symbol(char *buffer, unsigned long address)
> {
> - char *modname;
> - const char *name;
> unsigned long offset, size;
> - char namebuf[KSYM_NAME_LEN];
> + const char *name = NULL;
> + char *namebuf = NULL;
> + char *modname;
> + int ret;
> +
> +
> + /* Static buffer allocation.
> + Required in-order to reduce stack footprint on
> + do_IRQ/4KSTACK/i386 */
> + namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
> + if (namebuf)
> + name = kallsyms_lookup(address, &size, &offset,
> + &modname, namebuf);
>
> - name = kallsyms_lookup(address, &size, &offset, &modname, namebuf);
> if (!name)
> - return sprintf(buffer, "0x%lx", address);
> + ret = sprintf(buffer, "0x%lx", address);
> + else {
> + if (modname)
> + ret = sprintf(buffer, "%s+%#lx/%#lx [%s]",
> + name, offset, size, modname);
> + else
> + ret = sprintf(buffer, "%s+%#lx/%#lx",
> + name, offset, size);
> + }
>
> - if (modname)
> - return sprintf(buffer, "%s+%#lx/%#lx [%s]", name, offset,
> - size, modname);
> - else
> - return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
> + if (namebuf)
> + kfree(namebuf);
> +
> + return ret;
> }
>
> /* Look up a kernel symbol and print it to the kernel messages. */
> void __print_symbol(const char *fmt, unsigned long address)
> {
> - char buffer[KSYM_SYMBOL_LEN];
> + char *buffer = NULL;
>
> - sprint_symbol(buffer, address);
>
> - printk(fmt, buffer);
> + /* Static buffer allocation.
> + Required in-order to reduce stack footprint on
> + do_IRQ/4KSTACK/i386 */
> + buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
> + if (buffer) {
> + sprint_symbol(buffer, address);
> + printk(fmt, buffer);
> + kfree(buffer);
> + } else {
> + /* Address + '0x' + NULL. */
> + char sbuffer[(BITS_PER_LONG / 4) + 3];
> +
> + /* Fall-back mode.
> + Memory allocation failed.
> + Convert the address to string and display it. */
> + sprintf(sbuffer, "0x%lx", address);
> + printk(fmt, sbuffer);
> + }
> }
>
> /* To avoid using get_symbol_offset for every symbol, we carry prefix
> along. */
-
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]
