linux-kernel - [PATCH] Reduce __print_symbol/sprint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1190384893.30016.42.camel@gilboa-home-dev.localdomain>
Date:	Fri, 21 Sep 2007 16:28:13 +0200
From:	Gilboa Davara <gilboad@...il.com>
To:	linux-kernel@...r.kernel.org,
	Satyam Sharma <satyam.sharma@...il.com>,
	Paulo Marques <pmarques@...popie.com>
Subject: [PATCH] Reduce __print_symbol/sprint_symbol stack usage. (v3)

Hello all,

(1) Problem:
I. When CONFIG_4KSTACKS and CONFIG_DEBUG_STACKOVERFLOW are enabled on
i386 kernels, do_IRQ calls dump_stack which, down the path, uses
print_symbol (display) and  sprint_symbol (format) to format and display
the function name/address/module.
Both function use stack based char array (~350 bytes) that, given the
initial state (<512 bytes of stack space) may overrun the stack.
II. (Comments - previous patches) Using spinlock protected static
storage within these functions might block or even deadlock dump_stack
(E.g. Crash within dump_stack itself)

(2) Solution:
I. Break sprint_symbol into sprint_symbol (API functions; keeps the
current interface) and sprint_symbol_helper (helper function with
minimal local storage). 
II. Replace the char array in __print_symbol with two spinlock protected
static char arrays; call the __sprint_symbol helper function instead of
sprint_symbol.
III. Ignore the spinlock if oops_in_progress is set.

(3) Comments:
I. Currently, if oops_in_progress is set, multiple callers can trash
each other.
II. In order to solve it, print_symbol (and/or dump_stack itself) must
be aware of it's own context. (Read: if it's being called by oops, or by
a "debug/warning" code).
III. Possible solutions:
A. Add oops_in_progress_cpuid to bust_spinlocks (Hack, but generates far
less noise).
B. Add priority tag to all the functions that handle the calls stuck.
(Nicer, requires a massive tree-changing patch.)

Signed-off-by: Gilboa Davara <gilboad@...il.org>

Satyam, Paulo,
     A. I hope this patch better complies with your comments.

- Gilboa
--- linux-2.6/kernel/kallsyms.orig	2007-09-20 19:17:49.000000000 +0200
+++ linux-2.6/kernel/kallsyms.c	2007-09-21 16:24:03.000000000 +0200
@@ -306,13 +306,17 @@ int lookup_symbol_attrs(unsigned long ad
 	return lookup_module_symbol_attrs(addr, size, offset, modname, name);
 }

-/* Look up a kernel symbol and return it in a text buffer. */
-int sprint_symbol(char *buffer, unsigned long address)
+/*
+ * Helper function:
+ *
+ * Look up a kernel symbol and module name and return them to the
+ *   caller's buffer/namebuf buffers.
+*/
+int sprint_symbol_helper(char *buffer, char *namebuf, unsigned long
address)
 {
-	char *modname;
-	const char *name;
 	unsigned long offset, size;
-	char namebuf[KSYM_NAME_LEN];
+	const char *name;
+	char *modname;

 	name = kallsyms_lookup(address, &size, &offset, &modname, namebuf);
 	if (!name)
@@ -325,14 +329,52 @@ int sprint_symbol(char *buffer, unsigned
 		return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
 }

+/*
+ * API function:
+ *
+ * Look up a kernel symbol and return it in a text buffer.
+ */
+int sprint_symbol(char *buffer, unsigned long address)
+{
+	char namebuf[KSYM_NAME_LEN];
+
+	return sprint_symbol_helper(buffer, namebuf, address);
+}
+
+
 /* 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];
-
-	sprint_symbol(buffer, address);
+	/*
+	 * Use static buffers instead of char array to reduce the
+	 *   stack footprint in i386/4KSTACKS.
+	 * Buffers must be protected against re-entry.
+	 * ( As long as Oops is not in progress. )
+	 */
+	static DEFINE_SPINLOCK(symbol_lock);
+	static char namebuf[KSYM_NAME_LEN];
+	static char buffer[KSYM_SYMBOL_LEN];
+	unsigned long flags;
+	int locked = 0;
+
+
+	/*
+	 * Bust all spinlocks if oops is in progress.
+	 * FIXME: Might generate broken output if BUG()/oops
+	 *   generates a callstack while another one uses
+	 *   dump_stack for debug purposes.
+	 */
+	if (!oops_in_progress) {
+		/* Normal lock mode. */
+		spin_lock_irqsave(&symbol_lock, flags);
+		locked = 1;
+	}

+	sprint_symbol_helper(buffer, namebuf, address);
 	printk(fmt, buffer);
+
+	if (locked)
+		spin_unlock_irqrestore(&symbol_lock, flags);
 }

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