Date:	Sat, 15 Sep 2007 14:35:29 +0300
From:	Gilboa Davara <gilboad@...il.com>
To:	linux-kernel@...r.kernel.org
Subject: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.

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)

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.

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)

-- 
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.
B. per_cpu allocation will also require local_irq_disable/enable as both
functions are being called from multiple contexts. Too much hassle.


--- 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