Message-ID: <a781481a0709150602v6be754caw4fbe0f76c8db5beb@mail.gmail.com>
Date:	Sat, 15 Sep 2007 18:32:46 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	gilboad@...il.com
Cc:	linux-kernel@...r.kernel.org, "Eric Sandeen" <sandeen@...deen.net>
Subject: Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.

Hi,


On 9/15/07, Gilboa Davara <gilboad@...il.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@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists