Messages in this thread | | | Date | Sat, 15 Sep 2007 18:32:46 +0530 | From | "Satyam Sharma" <> | Subject | Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage. |
| |
Hi,
On 9/15/07, Gilboa Davara <gilboad@gmail.com> 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 <gilboad@gmail.com> > > [1] > http://www.mail-archive.com/fedora-kernel-list@redhat.com/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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |