[PATCH] show_trace() module_end = 0?

[PATCH] show_trace() module_end = 0?

[Hugh Dickins]

show_trace() contains an erroneous line, introduced in 2.4.6-pre4,
which disables trace on module text: appears to be from temporary
testing, since code and comments for tracing module text remain.

Hugh

--- linux-2.4.7-pre6/arch/i386/kernel/traps.c	Wed Jun 20 21:59:44 2001
+++ linux/arch/i386/kernel/traps.c	Thu Jul 12 16:25:42 2001
@@ -105,7 +105,6 @@
 	i = 1;
 	module_start = VMALLOC_START;
 	module_end = VMALLOC_END;
-	module_end = 0;
 	while (((long) stack & (THREAD_SIZE-1)) != 0) {
 		addr = *stack++;
 		/*

Re: [PATCH] show_trace() module_end = 0?

[Linus Torvalds]

On Thu, 12 Jul 2001, Hugh Dickins wrote:
>
> show_trace() contains an erroneous line, introduced in 2.4.6-pre4,
> which disables trace on module text: appears to be from temporary
> testing, since code and comments for tracing module text remain.

It as actually disabled on purpose.

It's there because without it the backtrace is sometimes so full of crud
that it is almost impossible to read.

I chose to disable the module back-trace, because what we should _really_
do is to walk the vmalloc space and verify whether it's a valid address or
not. But as I don't use modules myself, I didn't have much incentive to do
so, or to test that it worked.

The simple "disable module backtraces" approach at least makes the normal
backtraces possible to read sanely (well, you still have the issue that
gcc often ends up leaving tons of empty stackslots around and those can
contain stale information, but that can't be fixed as easily).

		Linus

Re: [PATCH] show_trace() module_end = 0?

[Hugh Dickins]

On Thu, 12 Jul 2001, Linus Torvalds wrote:
> On Thu, 12 Jul 2001, Hugh Dickins wrote:
> > show_trace() contains an erroneous line, introduced in 2.4.6-pre4,
> > which disables trace on module text: appears to be from temporary
> > testing, since code and comments for tracing module text remain.
> 
> It as actually disabled on purpose.
> 
> It's there because without it the backtrace is sometimes so full of crud
> that it is almost impossible to read.
> 
> I chose to disable the module back-trace, because what we should _really_
> do is to walk the vmalloc space and verify whether it's a valid address or
> not. But as I don't use modules myself, I didn't have much incentive to do
> so, or to test that it worked.

Thanks for owning up to that line!  I see your point.

I fear that most values found on the stack and within the vmalloc address
range would actually turn out to be valid addresses.  We probably should
not re-enable show_trace() on module text without something (in vm_struct?)
to distinguish module text from data, bss, and from other uses of vmalloc.

Hugh

Re: [PATCH] show_trace() module_end = 0?

[Kai Germaschewski]

On Thu, 12 Jul 2001, Linus Torvalds wrote:

> On Thu, 12 Jul 2001, Hugh Dickins wrote:
> >
> > show_trace() contains an erroneous line, introduced in 2.4.6-pre4,
> > which disables trace on module text: appears to be from temporary
> > testing, since code and comments for tracing module text remain.
> 
> It as actually disabled on purpose.
> 
> It's there because without it the backtrace is sometimes so full of crud
> that it is almost impossible to read.
> 
> I chose to disable the module back-trace, because what we should _really_
> do is to walk the vmalloc space and verify whether it's a valid address or
> not. But as I don't use modules myself, I didn't have much incentive to do
> so, or to test that it worked.
> 
> The simple "disable module backtraces" approach at least makes the normal
> backtraces possible to read sanely (well, you still have the issue that
> gcc often ends up leaving tons of empty stackslots around and those can
> contain stale information, but that can't be fixed as easily).

As I need module back traces once in a while, I was not really happy with 
the current approach. I coded a patch which will reenable printing a call 
trace including addresses in modules.

I'm not sure if this an approach you would accept. The code makes sure
that only addresses within a vmalloc'ed module area are printed, not
everything between VMALLOC_START and _END. However, we don't distinguish
between module .text as opposed to .data, .bss... Doing so seems a lot
harder to implement.

The other, minor problem is that we should walk the module_list under 
lock_kernel() only, but I wasn't brave enough to add this to the 
show_trace() code path.

Comments?

--Kai

diff -ur linux-2.4.7/arch/i386/kernel/traps.c linux-2.4.7.work/arch/i386/kernel/traps.c
--- linux-2.4.7/arch/i386/kernel/traps.c	Wed Jun 20 22:59:44 2001
+++ linux-2.4.7.work/arch/i386/kernel/traps.c	Thu Aug  2 14:44:44 2001
@@ -48,6 +48,7 @@
 #endif
 
 #include <linux/irq.h>
+#include <linux/module.h>
 
 asmlinkage int system_call(void);
 asmlinkage void lcall7(void);
@@ -89,36 +90,60 @@
 int kstack_depth_to_print = 24;
 
 /*
- * These constants are for searching for possible module text
- * segments.
+ * If the address is either in the .text section of the
+ * kernel, or in the vmalloc'ed module regions, it *may* 
+ * be the address of a calling routine
  */
+#ifdef CONFIG_MODULES
+
+extern struct module *module_list;
+extern struct module kernel_module;
+
+static inline int kernel_text_address(unsigned long addr)
+{
+	int retval = 0;
+	struct module *mod;
+
+	if (addr >= (unsigned long) &_stext &&
+	    addr <= (unsigned long) &_etext)
+		return 1;
+
+	for (mod = module_list; mod != &kernel_module; mod = mod->next) {
+		/* mod_bound tests for addr being inside the vmalloc'ed
+		 * module area. Of course it'd be better to test only
+		 * for the .text subset... */
+		if (mod_bound(addr, 0, mod)) {
+			retval = 1;
+			break;
+		}
+	}
+
+	return retval;
+}
+
+#else
+
+static inline int kernel_text_address(unsigned long addr)
+{
+	return (addr >= (unsigned long) &_stext &&
+		addr <= (unsigned long) &_etext);
+}
+
+#endif
 
 void show_trace(unsigned long * stack)
 {
 	int i;
-	unsigned long addr, module_start, module_end;
+	unsigned long addr;
 
 	if (!stack)
 		stack = (unsigned long*)&stack;
 
 	printk("Call Trace: ");
 	i = 1;
-	module_start = VMALLOC_START;
-	module_end = VMALLOC_END;
-	module_end = 0;
 	while (((long) stack & (THREAD_SIZE-1)) != 0) {
 		addr = *stack++;
-		/*
-		 * If the address is either in the text segment of the
-		 * kernel, or in the region which contains vmalloc'ed
-		 * memory, it *may* be the address of a calling
-		 * routine; if so, print it so that someone tracing
-		 * down the cause of the crash will be able to figure
-		 * out the call path that was taken.
-		 */
-		if (((addr >= (unsigned long) &_stext) &&
-		     (addr <= (unsigned long) &_etext)) ||
-		    ((addr >= module_start) && (addr <= module_end))) {
+		if (kernel_text_address(addr)) {
 			if (i && ((i % 8) == 0))
 				printk("\n       ");
 			printk("[<%08lx>] ", addr);
diff -ur linux-2.4.7/kernel/module.c linux-2.4.7.work/kernel/module.c
--- linux-2.4.7/kernel/module.c	Wed May  2 01:05:00 2001
+++ linux-2.4.7.work/kernel/module.c	Thu Aug  2 13:44:19 2001
@@ -39,7 +39,7 @@
 extern const char __start___kallsyms[] __attribute__ ((weak));
 extern const char __stop___kallsyms[] __attribute__ ((weak));
 
-static struct module kernel_module =
+struct module kernel_module =
 {
 	size_of_struct:		sizeof(struct module),
 	name: 			"",

Re: [PATCH] show_trace() module_end = 0?

[Hugh Dickins]

On Thu, 2 Aug 2001, Kai Germaschewski wrote:
> 
> As I need module back traces once in a while, I was not really happy with 
> the current approach. I coded a patch which will reenable printing a call 
> trace including addresses in modules.

Haven't tried it, but looks good to me.

> I'm not sure if this an approach you would accept. The code makes sure
> that only addresses within a vmalloc'ed module area are printed, not
> everything between VMALLOC_START and _END. However, we don't distinguish
> between module .text as opposed to .data, .bss... Doing so seems a lot
> harder to implement.

Much better than the original VMALLOC_START to VMALLOC_END, much better
than the current none-at-all, much better than going down the vmlist.

(The text could perhaps be identified at module load time, by looking
at the "__insmod_" symbols; but that would be a hack, you're right not
to try any such thing in this patch.)

> The other, minor problem is that we should walk the module_list under 
> lock_kernel() only, but I wasn't brave enough to add this to the 
> show_trace() code path.

I think it's just modlist_lock you'd need.  Not sure whether better
to try for it or ignore it.  CC'ed Andrew "spinlock-buster" Morton
for his opinion.

Hugh

Re: [PATCH] show_trace() module_end = 0?

[Andrew Morton]

Hugh Dickins wrote:
> 
> > The other, minor problem is that we should walk the module_list under
> > lock_kernel() only, but I wasn't brave enough to add this to the
> > show_trace() code path.
> 
> I think it's just modlist_lock you'd need.  Not sure whether better
> to try for it or ignore it.  CC'ed Andrew "spinlock-buster" Morton
> for his opinion.

Debugging things like show_trace() may be called from any context
at all, and hence really cannot take locks.  We just live with the
race possibilities.

One could possibly play games with spin_trylock, but I don't expect
it'd help much - if you failed to get the lock what can you do?

-