Print taint info in more places.

Print taint info in more places.

[Dave Jones]

We've found in the past that various bug reports have had minimal
information, just a few printk's rather than a complete oops,
and it's taken several round-trips with the bug reporter before
we've discovered they had some proprietary module loaded.

The patches below adds dumping of the tainted state to some
extra parts of the kernel that report 'bad things' happening.
These patches have been in the Fedora kernel for some time now,
and have proved useful often.

Signed-off-by: Dave Jones <davej@redhat.com>


--- linux-2.6/include/asm-generic/bug.h~	2007-02-12 16:18:21.000000000 -0500
+++ linux-2.6/include/asm-generic/bug.h	2007-02-12 16:19:57.000000000 -0500
@@ -3,6 +3,10 @@
 
 #include <linux/compiler.h>
 
+#ifndef __ASSEMBLY__
+extern const char *print_tainted(void);
+#endif
+
 #ifdef CONFIG_BUG
 
 #ifdef CONFIG_GENERIC_BUG
@@ -22,7 +26,8 @@ struct bug_entry {
 
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
-	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
+	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
+		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 	panic("BUG!"); \
 } while (0)
 #endif
@@ -39,8 +39,9 @@ struct bug_entry {
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on)) {					\
-		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
-			__LINE__, __FUNCTION__);			\
+		printk(KERN_ERR "WARNING: at %s:%d %s() (%s)\n",	\
+			 __FILE__, __LINE__, __FUNCTION__,		\
+			 print_tainted());				\
 		dump_stack();						\
 	}								\
 	unlikely(__ret_warn_on);					\
diff -urNp --exclude-from=/home/davej/.exclude linux-1740/kernel/panic.c linux-2000/kernel/panic.c
--- linux-1740/kernel/panic.c
+++ linux-2000/kernel/panic.c
@@ -151,6 +151,7 @@ const char *print_tainted(void)
 		snprintf(buf, sizeof(buf), "Not tainted");
 	return(buf);
 }
+EXPORT_SYMBOL(print_tainted);
 
 void add_taint(unsigned flag)
 {
--- linux-2.6/mm/page_alloc.c~	2006-01-07 20:48:33.000000000 -0500
+++ linux-2.6/mm/page_alloc.c	2006-01-07 20:49:24.000000000 -0500
@@ -137,12 +137,12 @@ static inline int bad_range(struct zone 
 static void bad_page(struct page *page)
 {
 	printk(KERN_EMERG "Bad page state in process '%s'\n"
-		KERN_EMERG "page:%p flags:0x%0*lx mapping:%p mapcount:%d count:%d\n"
+		KERN_EMERG "page:%p flags:0x%0*lx mapping:%p mapcount:%d count:%d (%s)\n"
 		KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
 		KERN_EMERG "Backtrace:\n",
 		current->comm, page, (int)(2*sizeof(unsigned long)),
 		(unsigned long)page->flags, page->mapping,
-		page_mapcount(page), page_count(page));
+		page_mapcount(page), page_count(page), print_tainted());
 	dump_stack();
 	page->flags &= ~(1 << PG_lru	|
 			1 << PG_private |
--- linux-2.6/mm/slab.c~	2007-05-27 22:57:44.000000000 -0400
+++ linux-2.6/mm/slab.c	2007-05-27 22:58:08.000000000 -0400
@@ -1816,8 +1816,8 @@ static void check_poison_obj(struct kmem
 			/* Print header */
 			if (lines == 0) {
 				printk(KERN_ERR
-					"Slab corruption: %s start=%p, len=%d\n",
-					cachep->name, realobj, size);
+					"Slab corruption (%s): %s start=%p, len=%d\n",
+					print_tainted(), cachep->name, realobj, size);
 				print_objinfo(cachep, objp, 0);
 			}
 			/* Hexdump the affected line */
@@ -2924,8 +2924,8 @@ static void check_slabp(struct kmem_cach
 	if (entries != cachep->num - slabp->inuse) {
 bad:
 		printk(KERN_ERR "slab: Internal list corruption detected in "
-				"cache '%s'(%d), slabp %p(%d). Hexdump:\n",
-			cachep->name, cachep->num, slabp, slabp->inuse);
+				"cache '%s'(%d), slabp %p(%d). Tainted(%s). Hexdump:\n",
+			cachep->name, cachep->num, slabp, slabp->inuse, print_tainted());
 		for (i = 0;
 		     i < sizeof(*slabp) + cachep->num * sizeof(kmem_bufctl_t);
 		     i++) {
--- linux-2.6/mm/slub.c~	2007-07-20 14:15:05.000000000 -0400
+++ linux-2.6/mm/slub.c	2007-07-20 14:17:37.000000000 -0400
@@ -450,7 +450,7 @@ static void slab_bug(struct kmem_cache *
 	va_end(args);
 	printk(KERN_ERR "========================================"
 			"=====================================\n");
-	printk(KERN_ERR "BUG %s: %s\n", s->name, buf);
+	printk(KERN_ERR "BUG %s (%s): %s\n", s->name, print_tainted(), buf);
 	printk(KERN_ERR "----------------------------------------"
 			"-------------------------------------\n\n");
 }
--- linux-2.6/lib/spinlock_debug.c~	2007-10-22 01:15:04.000000000 -0400
+++ linux-2.6/lib/spinlock_debug.c	2007-10-22 01:17:03.000000000 -0400
@@ -58,9 +58,9 @@ static void spin_bug(spinlock_t *lock, c
 
 	if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT)
 		owner = lock->owner;
-	printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d\n",
+	printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d (%s)\n",
 		msg, raw_smp_processor_id(),
-		current->comm, task_pid_nr(current));
+		current->comm, task_pid_nr(current), print_tainted());
 	printk(KERN_EMERG " lock: %p, .magic: %08x, .owner: %s/%d, "
 			".owner_cpu: %d\n",
 		lock, lock->magic,
@@ -114,9 +114,9 @@ static void __spin_lock_debug(spinlock_t
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: spinlock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				task_pid_nr(current), lock);
+				task_pid_nr(current), lock, print_tainted());
 			dump_stack();
 #ifdef CONFIG_SMP
 			trigger_all_cpu_backtrace();
@@ -159,9 +159,9 @@ static void rwlock_bug(rwlock_t *lock, c
 	if (!debug_locks_off())
 		return;
 
-	printk(KERN_EMERG "BUG: rwlock %s on CPU#%d, %s/%d, %p\n",
+	printk(KERN_EMERG "BUG: rwlock %s on CPU#%d, %s/%d, %p (%s)\n",
 		msg, raw_smp_processor_id(), current->comm,
-		task_pid_nr(current), lock);
+		task_pid_nr(current), lock, print_tainted());
 	dump_stack();
 }
 
@@ -177,9 +177,9 @@ static void __read_lock_debug(rwlock_t *
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: read-lock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				current->pid, lock);
+				current->pid, lock, print_tainted());
 			dump_stack();
 		}
 	}
@@ -250,9 +250,9 @@ static void __write_lock_debug(rwlock_t 
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: write-lock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				current->pid, lock);
+				current->pid, lock, print_tainted());
 			dump_stack();
 		}
 	}
-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Andi Kleen]

Dave Jones <davej@redhat.com> writes:

>  #define WARN_ON(condition) ({						\
>  	int __ret_warn_on = !!(condition);				\
>  	if (unlikely(__ret_warn_on)) {					\
> -		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
> -			__LINE__, __FUNCTION__);			\
> +		printk(KERN_ERR "WARNING: at %s:%d %s() (%s)\n",	\
> +			 __FILE__, __LINE__, __FUNCTION__,		\
> +			 print_tainted());				\
>  		dump_stack();						\

Have you checked how this affects code size? It might be worth it 
now to do a out of line helper.

-Andi

Re: Print taint info in more places.

[Mauricio Mauad Menegaz Filho]

2007/12/13, Andi Kleen <andi@firstfloor.org>:
> Dave Jones <davej@redhat.com> writes:
>
> >  #define WARN_ON(condition) ({                                                \
> >       int __ret_warn_on = !!(condition);                              \
> >       if (unlikely(__ret_warn_on)) {                                  \
> > -             printk("WARNING: at %s:%d %s()\n", __FILE__,            \
> > -                     __LINE__, __FUNCTION__);                        \
> > +             printk(KERN_ERR "WARNING: at %s:%d %s() (%s)\n",        \
> > +                      __FILE__, __LINE__, __FUNCTION__,              \
> > +                      print_tainted());                              \
> >               dump_stack();                                           \
>
> Have you checked how this affects code size? It might be worth it
> now to do a out of line helper.
>

Have you checked how does spotting a bug is worth the extra size
sometimes (almost all the time)?

Re: Print taint info in more places.

[Dave Jones]

On Fri, Dec 14, 2007 at 12:08:46AM +0100, Andi Kleen wrote:
 > Dave Jones <davej@redhat.com> writes:
 > 
 > >  #define WARN_ON(condition) ({						\
 > >  	int __ret_warn_on = !!(condition);				\
 > >  	if (unlikely(__ret_warn_on)) {					\
 > > -		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
 > > -			__LINE__, __FUNCTION__);			\
 > > +		printk(KERN_ERR "WARNING: at %s:%d %s() (%s)\n",	\
 > > +			 __FILE__, __LINE__, __FUNCTION__,		\
 > > +			 print_tainted());				\
 > >  		dump_stack();						\
 > 
 > Have you checked how this affects code size? It might be worth it 
 > now to do a out of line helper.

64 bit debug build of the fedora kernel (which should be worse 
 case for WARN_ONs etc).

         text     data     bss     dec     hex filename
before   3833875  834267  631136 5299278  50dc4e vmlinux
after    3837054  834267  631136 5302457  50e8b9 vmlinux

Some of that growth is the addition of the missing KERN_ERR,
but yeah, it grows a little bit.

	Dave

-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Dave Jones]

On Thu, Dec 13, 2007 at 06:52:42PM -0500, Dave Jones wrote:

 >  > Have you checked how this affects code size? It might be worth it 
 >  > now to do a out of line helper.
 > 
 > 64 bit debug build of the fedora kernel (which should be worse 
 >  case for WARN_ONs etc).
 > 
 >          text     data     bss     dec     hex filename
 > before   3833875  834267  631136 5299278  50dc4e vmlinux
 > after    3837054  834267  631136 5302457  50e8b9 vmlinux
 > 
 > Some of that growth is the addition of the missing KERN_ERR,
 > but yeah, it grows a little bit.

And the out-of-line variant of WARN_ON ends up at ..

   text    data     bss     dec     hex filename
3828632  834267  631136 5294035  50c7d3 vmlinux

	Dave

-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Adrian Bunk]

On Thu, Dec 13, 2007 at 05:49:27PM -0500, Dave Jones wrote:
> We've found in the past that various bug reports have had minimal
> information, just a few printk's rather than a complete oops,
> and it's taken several round-trips with the bug reporter before
> we've discovered they had some proprietary module loaded.
> 
> The patches below adds dumping of the tainted state to some
> extra parts of the kernel that report 'bad things' happening.
> These patches have been in the Fedora kernel for some time now,
> and have proved useful often.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> 
> --- linux-2.6/include/asm-generic/bug.h~	2007-02-12 16:18:21.000000000 -0500
> +++ linux-2.6/include/asm-generic/bug.h	2007-02-12 16:19:57.000000000 -0500
> @@ -3,6 +3,10 @@
>  
>  #include <linux/compiler.h>
>  
> +#ifndef __ASSEMBLY__
> +extern const char *print_tainted(void);
> +#endif
> +
>  #ifdef CONFIG_BUG
>  
>  #ifdef CONFIG_GENERIC_BUG
> @@ -22,7 +26,8 @@ struct bug_entry {
>  
>  #ifndef HAVE_ARCH_BUG
>  #define BUG() do { \
> -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
> +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
> +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
>  	panic("BUG!"); \
>  } while (0)
>  #endif
>...

Note that this only changes a handful of architectures and most likely 
not the ones you are interested in.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Print taint info in more places.

[Dave Jones]

On Fri, Dec 14, 2007 at 01:03:50AM +0100, Adrian Bunk wrote:

 > >  #ifndef HAVE_ARCH_BUG
 > >  #define BUG() do { \
 > > -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 > > +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
 > > +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 > >  	panic("BUG!"); \
 > >  } while (0)
 > >  #endif
 > >...
 > 
 > Note that this only changes a handful of architectures and most likely 
 > not the ones you are interested in.

Good catch.  I'm sure we had coverage on at least x86 before, so
it seems over the many rediffs this patch has had, some chunks got lost.
I'll resurrect that.

thanks,

	Dave

-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Dave Jones]

On Fri, Dec 14, 2007 at 01:03:50AM +0100, Adrian Bunk wrote:

 > >  #ifndef HAVE_ARCH_BUG
 > >  #define BUG() do { \
 > > -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 > > +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
 > > +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 > >  	panic("BUG!"); \
 > >  } while (0)
 > >  #endif
 > >...
 > 
 > Note that this only changes a handful of architectures and most likely 
 > not the ones you are interested in.

Hmm, it appears that I was mistaken, and we never did patch x86.
Which leaves me wondering if its worth it or not to  patch BUG()
Anyways, here's the latest rev with the out-of-line changes as
suggested by Andi.

init/main.c may not be the best place for the ool variant. suggestions?

	Dave


diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d56fedb..e35833a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -3,6 +3,10 @@
 
 #include <linux/compiler.h>
 
+#ifndef __ASSEMBLY__
+extern const char *print_tainted(void);
+#endif
+
 #ifdef CONFIG_BUG
 
 #ifdef CONFIG_GENERIC_BUG
@@ -22,7 +26,8 @@ struct bug_entry {
 
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
-	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
+	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
+		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 	panic("BUG!"); \
 } while (0)
 #endif
@@ -32,13 +37,11 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
+void out_of_line_warnon(char *file, unsigned int line, const char *func);
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
-	if (unlikely(__ret_warn_on)) {					\
-		printk("WARNING: at %s:%d %s()\n", __FILE__,		\
-			__LINE__, __FUNCTION__);			\
-		dump_stack();						\
-	}								\
+	if (unlikely(__ret_warn_on))					\
+		out_of_line_warnon(__FILE__, __LINE__, __FUNCTION__);	\
 	unlikely(__ret_warn_on);					\
 })
 #endif
diff --git a/init/main.c b/init/main.c
index 80b04b6..b1fad76 100644
--- a/init/main.c
+++ b/init/main.c
@@ -855,3 +855,11 @@ static int __init kernel_init(void * unused)
 	init_post();
 	return 0;
 }
+
+void out_of_line_warnon(char *file, unsigned int line, const char *func)
+{
+	printk(KERN_ERR "WARNING: at %s:%d %s() (%s)\n",
+		file, line, func, print_tainted());
+	dump_stack();
+}
+EXPORT_SYMBOL(out_of_line_warnon);
diff --git a/kernel/panic.c b/kernel/panic.c
index 6f6e03e..198fc58 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -173,6 +173,7 @@ const char *print_tainted(void)
 		snprintf(buf, sizeof(buf), "Not tainted");
 	return(buf);
 }
+EXPORT_SYMBOL(print_tainted);
 
 void add_taint(unsigned flag)
 {
diff --git a/lib/spinlock_debug.c b/lib/spinlock_debug.c
index 9c4b025..b7a010a 100644
--- a/lib/spinlock_debug.c
+++ b/lib/spinlock_debug.c
@@ -58,9 +58,9 @@ static void spin_bug(spinlock_t *lock, const char *msg)
 
 	if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT)
 		owner = lock->owner;
-	printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d\n",
+	printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d (%s)\n",
 		msg, raw_smp_processor_id(),
-		current->comm, task_pid_nr(current));
+		current->comm, task_pid_nr(current), print_tainted());
 	printk(KERN_EMERG " lock: %p, .magic: %08x, .owner: %s/%d, "
 			".owner_cpu: %d\n",
 		lock, lock->magic,
@@ -114,9 +114,9 @@ static void __spin_lock_debug(spinlock_t *lock)
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: spinlock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				task_pid_nr(current), lock);
+				task_pid_nr(current), lock, print_tainted());
 			dump_stack();
 #ifdef CONFIG_SMP
 			trigger_all_cpu_backtrace();
@@ -159,9 +159,9 @@ static void rwlock_bug(rwlock_t *lock, const char *msg)
 	if (!debug_locks_off())
 		return;
 
-	printk(KERN_EMERG "BUG: rwlock %s on CPU#%d, %s/%d, %p\n",
+	printk(KERN_EMERG "BUG: rwlock %s on CPU#%d, %s/%d, %p (%s)\n",
 		msg, raw_smp_processor_id(), current->comm,
-		task_pid_nr(current), lock);
+		task_pid_nr(current), lock, print_tainted());
 	dump_stack();
 }
 
@@ -184,9 +184,9 @@ static void __read_lock_debug(rwlock_t *lock)
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: read-lock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				current->pid, lock);
+				current->pid, lock, print_tainted());
 			dump_stack();
 		}
 	}
@@ -259,9 +259,9 @@ static void __write_lock_debug(rwlock_t *lock)
 		if (print_once) {
 			print_once = 0;
 			printk(KERN_EMERG "BUG: write-lock lockup on CPU#%d, "
-					"%s/%d, %p\n",
+					"%s/%d, %p (%s)\n",
 				raw_smp_processor_id(), current->comm,
-				current->pid, lock);
+				current->pid, lock, print_tainted());
 			dump_stack();
 		}
 	}
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b5a58d4..7a0c25d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -221,12 +221,12 @@ static inline int bad_range(struct zone *zone, struct page *page)
 static void bad_page(struct page *page)
 {
 	printk(KERN_EMERG "Bad page state in process '%s'\n"
-		KERN_EMERG "page:%p flags:0x%0*lx mapping:%p mapcount:%d count:%d\n"
+		KERN_EMERG "page:%p flags:0x%0*lx mapping:%p mapcount:%d count:%d (%s)\n"
 		KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
 		KERN_EMERG "Backtrace:\n",
 		current->comm, page, (int)(2*sizeof(unsigned long)),
 		(unsigned long)page->flags, page->mapping,
-		page_mapcount(page), page_count(page));
+		page_mapcount(page), page_count(page), print_tainted());
 	dump_stack();
 	page->flags &= ~(1 << PG_lru	|
 			1 << PG_private |
diff --git a/mm/slab.c b/mm/slab.c
index 2e338a5..e5627f9 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1846,8 +1846,8 @@ static void check_poison_obj(struct kmem_cache *cachep, void *objp)
 			/* Print header */
 			if (lines == 0) {
 				printk(KERN_ERR
-					"Slab corruption: %s start=%p, len=%d\n",
-					cachep->name, realobj, size);
+					"Slab corruption (%s): %s start=%p, len=%d\n",
+					print_tainted(), cachep->name, realobj, size);
 				print_objinfo(cachep, objp, 0);
 			}
 			/* Hexdump the affected line */
@@ -2935,8 +2935,8 @@ static void check_slabp(struct kmem_cache *cachep, struct slab *slabp)
 	if (entries != cachep->num - slabp->inuse) {
 bad:
 		printk(KERN_ERR "slab: Internal list corruption detected in "
-				"cache '%s'(%d), slabp %p(%d). Hexdump:\n",
-			cachep->name, cachep->num, slabp, slabp->inuse);
+				"cache '%s'(%d), slabp %p(%d). Tainted(%s). Hexdump:\n",
+			cachep->name, cachep->num, slabp, slabp->inuse, print_tainted());
 		for (i = 0;
 		     i < sizeof(*slabp) + cachep->num * sizeof(kmem_bufctl_t);
 		     i++) {
diff --git a/mm/slub.c b/mm/slub.c
index 9c1d9f3..e11d58d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -451,7 +451,7 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...)
 	va_end(args);
 	printk(KERN_ERR "========================================"
 			"=====================================\n");
-	printk(KERN_ERR "BUG %s: %s\n", s->name, buf);
+	printk(KERN_ERR "BUG %s (%s): %s\n", s->name, print_tainted(), buf);
 	printk(KERN_ERR "----------------------------------------"
 			"-------------------------------------\n\n");
 }
-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Jeremy Fitzhardinge]

Dave Jones wrote:
> On Fri, Dec 14, 2007 at 01:03:50AM +0100, Adrian Bunk wrote:
>
>  > >  #ifndef HAVE_ARCH_BUG
>  > >  #define BUG() do { \
>  > > -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
>  > > +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
>  > > +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
>  > >  	panic("BUG!"); \
>  > >  } while (0)
>  > >  #endif
>  > >...
>  > 
>  > Note that this only changes a handful of architectures and most likely 
>  > not the ones you are interested in.
>
> Hmm, it appears that I was mistaken, and we never did patch x86.
> Which leaves me wondering if its worth it or not to  patch BUG()
> Anyways, here's the latest rev with the out-of-line changes as
> suggested by Andi.
>
> init/main.c may not be the best place for the ool variant. suggestions?
>   

lib/bug.c would be the place for architectures using
CONFIG_GENERIC_BUG.  x86 could be converted to use the BUG-trapping
mechanism for WARN_ON like Power does, so it would be inherently out of
line anyway.

    J

Re: Print taint info in more places.

[Dave Jones]

On Thu, Dec 13, 2007 at 11:25:06PM -0800, Jeremy Fitzhardinge wrote:
 > Dave Jones wrote:
 > > On Fri, Dec 14, 2007 at 01:03:50AM +0100, Adrian Bunk wrote:
 > >
 > >  > >  #ifndef HAVE_ARCH_BUG
 > >  > >  #define BUG() do { \
 > >  > > -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 > >  > > +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
 > >  > > +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 > >  > >  	panic("BUG!"); \
 > >  > >  } while (0)
 > >  > >  #endif
 > >  > >...
 > >  > 
 > >  > Note that this only changes a handful of architectures and most likely 
 > >  > not the ones you are interested in.
 > >
 > > Hmm, it appears that I was mistaken, and we never did patch x86.
 > > Which leaves me wondering if its worth it or not to  patch BUG()
 > > Anyways, here's the latest rev with the out-of-line changes as
 > > suggested by Andi.
 > >
 > > init/main.c may not be the best place for the ool variant. suggestions?
 > >   
 > 
 > lib/bug.c would be the place for architectures using
 > CONFIG_GENERIC_BUG.  x86 could be converted to use the BUG-trapping
 > mechanism for WARN_ON like Power does, so it would be inherently out of
 > line anyway.

The BUG()/WARN_ON() parts of my patch are becoming something of a rathole,
which I don't really have time to dig into right now, so I'm going to split
this up I think into mm/ additions, spinlockdebug additions, and bugon/warnon foo.
The three should be independant, and blocking the first two until I get
time to look at the third seems pointless.

	Dave

-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Matt Mackall]

On Thu, Dec 13, 2007 at 08:30:41PM -0500, Dave Jones wrote:
>  #ifndef HAVE_ARCH_BUG
>  #define BUG() do { \
> -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
> +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
> +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
>  	panic("BUG!"); \
>  } while (0)

Might as well make this inline too, no?

>  #endif
> @@ -32,13 +37,11 @@ struct bug_entry {
>  #endif
>  
>  #ifndef HAVE_ARCH_WARN_ON
> +void out_of_line_warnon(char *file, unsigned int line, const char *func);

How about __warn_on?

> diff --git a/init/main.c b/init/main.c
> index 80b04b6..b1fad76 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -855,3 +855,11 @@ static int __init kernel_init(void * unused)
>  	init_post();
>  	return 0;
>  }
> +
> +void out_of_line_warnon(char *file, unsigned int line, const char *func)

Might as well make *file const too.

> diff --git a/kernel/panic.c b/kernel/panic.c
> index 6f6e03e..198fc58 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -173,6 +173,7 @@ const char *print_tainted(void)
>  		snprintf(buf, sizeof(buf), "Not tainted");
>  	return(buf);
>  }
> +EXPORT_SYMBOL(print_tainted);

Looks like you've got two patches here..

-- 
Mathematics is the supreme nostalgia of our time.

Re: Print taint info in more places.

[Dave Jones]

On Fri, Dec 14, 2007 at 09:38:44AM -0600, Matt Mackall wrote:
 > On Thu, Dec 13, 2007 at 08:30:41PM -0500, Dave Jones wrote:
 > >  #ifndef HAVE_ARCH_BUG
 > >  #define BUG() do { \
 > > -	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
 > > +	printk(KERN_ERR "BUG: failure at %s:%d/%s()! (%s)\n",
 > > +		__FILE__, __LINE__, __FUNCTION__, print_tainted()); \
 > >  	panic("BUG!"); \
 > >  } while (0)
 > 
 > Might as well make this inline too, no?

Maybe.

 > > +void out_of_line_warnon(char *file, unsigned int line, const char *func);
 > 
 > How about __warn_on?

I'm easily swayed :)

 > > +void out_of_line_warnon(char *file, unsigned int line, const char *func)
 > 
 > Might as well make *file const too.

ok

 > > diff --git a/kernel/panic.c b/kernel/panic.c
 > > index 6f6e03e..198fc58 100644
 > > --- a/kernel/panic.c
 > > +++ b/kernel/panic.c
 > > @@ -173,6 +173,7 @@ const char *print_tainted(void)
 > >  		snprintf(buf, sizeof(buf), "Not tainted");
 > >  	return(buf);
 > >  }
 > > +EXPORT_SYMBOL(print_tainted);
 > 
 > Looks like you've got two patches here..

no, up top we added a print_tainted() call to BUG()
BUG() gets used in modules == needs its ancillary functions
also exported.

	Dave

-- 
http://www.codemonkey.org.uk

Re: Print taint info in more places.

[Jon Masters]

On Thu, 2007-12-13 at 17:49 -0500, Dave Jones wrote:
> We've found in the past that various bug reports have had minimal
> information, just a few printk's rather than a complete oops,
> and it's taken several round-trips with the bug reporter before
> we've discovered they had some proprietary module loaded.
> 
> The patches below adds dumping of the tainted state to some
> extra parts of the kernel that report 'bad things' happening.
> These patches have been in the Fedora kernel for some time now,
> and have proved useful often.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

In my opinion, this is a very good idea :-)

Jon.