Re: memory corruptor in .18rc1-git

Re: memory corruptor in .18rc1-git

[Chuck Ebbert]

In-Reply-To: <20060714043112.GA20478@redhat.com>

On Fri, 14 Jul 2006 00:31:12 -0400, Dave Jones wrote:

> +/**
> + * list_add - add a new entry
> + * @new: new entry to be added
> + * @head: list head to add it after
> + *
> + * Insert a new entry after the specified head.
> + * This is good for implementing stacks.
> + */
> +void list_add(struct list_head *new, struct list_head *head)
> +{
> +     if (head->next->prev != head) {
> +             printk("List corruption. next->prev should be %p, but was %p\n",
> +                     head, head->next->prev);
> +             BUG();
> +     }
> +     if (head->prev->next != head) {
> +             printk("List corruption. prev->next should be %p, but was %p\n",
> +                     head, head->prev->next);
> +             BUG();
> +     }
> +
> +     __list_add(new, head, head->next);
> +}
> +EXPORT_SYMBOL(list_add);
> +
> +/**
> + * list_del - deletes entry from list.
> + * @entry: the element to delete from the list.
> + * Note: list_empty on entry does not return true after this, the entry is
> + * in an undefined state.
> + */
> +void list_del(struct list_head *entry)
> +{
> +     if (entry->prev->next != entry) {
> +             printk("List corruption. prev->next should be %p, but was %p\n",
> +                     entry, entry->prev->next);
> +             BUG();
> +     }
> +     if (entry->next->prev != entry) {
> +             printk("List corruption. next->prev should be %p, but was %p\n",
> +                     entry, entry->next->prev);
> +             BUG();
> +     }
> +     __list_del(entry->prev, entry->next);
> +     entry->next = LIST_POISON1;
> +     entry->prev = LIST_POISON2;
> +}
> +EXPORT_SYMBOL(list_del);
> +
>

Shouldn't those four 'if' statements use unlikely()?  There's no sense
causing more slowdown than necessary, even in debug code.

And I'd change the messages slightly, e.g.:

        "list_add: corruption: next->prev should be %p, was %p\n"

Some people build (accidentally?) without verbose debug info and
don't get line numbers.

-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: memory corruptor in .18rc1-git

[Dave Jones]

On Sat, Jul 15, 2006 at 03:54:35PM -0400, Chuck Ebbert wrote:

 > Shouldn't those four 'if' statements use unlikely()?  There's no sense
 > causing more slowdown than necessary, even in debug code.

good point.

 > And I'd change the messages slightly, e.g.:
 > 
 >         "list_add: corruption: next->prev should be %p, was %p\n"
 > 
 > Some people build (accidentally?) without verbose debug info and
 > don't get line numbers.
 
Ok, you're the second person to ask for this, so I'll make the change.

Andrew,Linus, here's the latest incarnation.
(Not build-tested yet, willdo after packing for OLS, but the changes
 should be trivial enough not to blow up).

		Dave

Debug variants of linked-list manipulation macros.

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

diff --git a/include/linux/list.h b/include/linux/list.h
index 6b74adf..5617c77 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -39,6 +39,7 @@ static inline void INIT_LIST_HEAD(struct
  * This is only for internal list manipulation where we know
  * the prev/next entries already!
  */
+#ifndef CONFIG_DEBUG_LIST
 static inline void __list_add(struct list_head *new,
 			      struct list_head *prev,
 			      struct list_head *next)
@@ -48,6 +49,11 @@ static inline void __list_add(struct lis
 	new->prev = prev;
 	prev->next = new;
 }
+#else
+extern void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next);
+#endif
 
 /**
  * list_add - add a new entry
@@ -57,10 +63,15 @@ static inline void __list_add(struct lis
  * Insert a new entry after the specified head.
  * This is good for implementing stacks.
  */
+#ifndef CONFIG_DEBUG_LIST
 static inline void list_add(struct list_head *new, struct list_head *head)
 {
 	__list_add(new, head, head->next);
 }
+#else
+extern void list_add(struct list_head *new, struct list_head *head);
+#endif
+
 
 /**
  * list_add_tail - add a new entry
@@ -153,12 +164,16 @@ static inline void __list_del(struct lis
  * Note: list_empty on entry does not return true after this, the entry is
  * in an undefined state.
  */
+#ifndef CONFIG_DEBUG_LIST
 static inline void list_del(struct list_head *entry)
 {
 	__list_del(entry->prev, entry->next);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;
 }
+#else
+extern void list_del(struct list_head *entry);
+#endif
 
 /**
  * list_del_rcu - deletes entry from list without re-initialization
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e5889b1..bb17ce3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -311,6 +311,15 @@ config DEBUG_VM
 
 	  If unsure, say N.
 
+config DEBUG_LIST
+	bool "Debug linked list manipulation"
+	depends on DEBUG_KERNEL
+	help
+	  Enable this to turn on extended checks in the linked-list
+	  walking routines.
+
+	  If unsure, say N.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	depends on DEBUG_KERNEL && (X86 || CRIS || M68K || M68KNOMMU || FRV || UML || S390)
diff --git a/lib/Makefile b/lib/Makefile
index be9719a..7d7da98 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -28,6 +28,7 @@ lib-$(CONFIG_GENERIC_HWEIGHT) += hweight
 obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o
 obj-$(CONFIG_PLIST) += plist.o
 obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
+obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 
 ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
   lib-y += dec_and_lock.o
diff --git a/lib/list_debug.c b/lib/list_debug.c
new file mode 100644
index 0000000..0b48b95
--- /dev/null
+++ b/lib/list_debug.c
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2006, Red Hat, Inc., Dave Jones
+ * Released under the General Public License (GPL).
+ *
+ * This file contains the linked list implementations for
+ * DEBUG_LIST.
+ */
+
+#include <linux/module.h>
+#include <linux/list.h>
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+
+void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	if (unlikely(next->prev != prev)) {
+		printk("list_add corruption. next->prev should be %p, but was %p\n",
+			prev, next->prev);
+		BUG();
+	}
+	if (unlikely(prev->next != next)) {
+		printk("list_add corruption. prev->next should be %p, but was %p\n",
+			next, prev->next);
+		BUG();
+	}
+	next->prev = new;
+	new->next = next;
+	new->prev = prev;
+	prev->next = new;
+}
+EXPORT_SYMBOL(__list_add);
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+void list_add(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head, head->next);
+}
+EXPORT_SYMBOL(list_add);
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+void list_del(struct list_head *entry)
+{
+	if (unlikely(entry->prev->next != entry)) {
+		printk("list_del corruption. prev->next should be %p, but was %p\n",
+			entry, entry->prev->next);
+		BUG();
+	}
+	if (unlikely(entry->next->prev != entry)) {
+		printk("list_del corruption. next->prev should be %p, but was %p\n",
+			entry, entry->next->prev);
+		BUG();
+	}
+	__list_del(entry->prev, entry->next);
+	entry->next = LIST_POISON1;
+	entry->prev = LIST_POISON2;
+}
+EXPORT_SYMBOL(list_del);
+
-- 
http://www.codemonkey.org.uk

memory corruptor in .18rc1-git

[Dave Jones]

Three times in the last week, I've had a box running the -git-du-jour
spontaneously reboot.  It just happened again, this time I had a serial
console hooked up, but it rebooted before transferring much data.
The one thing it did spew however was "List corruption. prev->ne",
which came from the patch below which I had in my tree.
(The latter half is likely irrelevant, and came from chasing a different bug)

Things in common at all three times it happened..
reading email, and listening to oggs with rhythmbox.
Another ALSA bug maybe ?

I've up'd the speed of the serial console, in the hope that more chars
make it over the wire before reboot should this happen again.

		Dave


--- linux-2.6/include/linux/list.h~	2005-08-08 15:34:50.000000000 -0400
+++ linux-2.6/include/linux/list.h	2005-08-08 15:35:22.000000000 -0400
@@ -5,7 +5,9 @@
 
 #include <linux/stddef.h>
 #include <linux/prefetch.h>
+#include <linux/kernel.h>
 #include <asm/system.h>
+#include <asm/bug.h>
 
 /*
  * These are non-NULL pointers that will result in page faults
@@ -52,6 +52,16 @@ static inline void __list_add(struct lis
 			      struct list_head *prev,
 			      struct list_head *next)
 {
+	if (next->prev != prev) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+				prev, next->prev);
+		BUG();
+	}
+	if (prev->next != next) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+				next, prev->next);
+		BUG();
+	}
 	next->prev = new;
 	new->next = next;
 	new->prev = prev;
@@ -162,6 +162,16 @@ static inline void __list_del(struct lis
  */
 static inline void list_del(struct list_head *entry)
 {
+	if (entry->prev->next != entry) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+				entry, entry->prev->next);
+		BUG();
+	}
+	if (entry->next->prev != entry) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+				entry, entry->next->prev);
+		BUG();
+	}
 	__list_del(entry->prev, entry->next);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;



It _may_ give more information about exactly which wait-queue (or rather, 
what the poll function that was related to that wait-queue) seems to be 
corrupted. 

		Linus

---
diff --git a/fs/select.c b/fs/select.c
index a8109ba..8cd6dc3 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -23,6 +23,7 @@ #include <linux/personality.h> /* for ST
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/rcupdate.h>
+#include <linux/kallsyms.h>
 
 #include <asm/uaccess.h>
 
@@ -65,7 +66,8 @@ EXPORT_SYMBOL(poll_initwait);
 
 static void free_poll_entry(struct poll_table_entry *entry)
 {
-	remove_wait_queue(entry->wait_address,&entry->wait);
+	if (remove_wait_queue(entry->wait_address,&entry->wait) < 0)
+		print_symbol("bad poll-entry for %s", (unsigned long) entry->filp->f_op->poll);
 	fput(entry->filp);
 }
 
diff --git a/include/linux/wait.h b/include/linux/wait.h
index d285182..4c1d74e 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -115,7 +115,7 @@ #define is_sync_wait(wait)	(!(wait) || (
 
 extern void FASTCALL(add_wait_queue(wait_queue_head_t *q, wait_queue_t * wait));
 extern void FASTCALL(add_wait_queue_exclusive(wait_queue_head_t *q, wait_queue_t * wait));
-extern void FASTCALL(remove_wait_queue(wait_queue_head_t *q, wait_queue_t * wait));
+extern int FASTCALL(remove_wait_queue(wait_queue_head_t *q, wait_queue_t * wait));
 
 static inline void __add_wait_queue(wait_queue_head_t *head, wait_queue_t *new)
 {
diff --git a/kernel/wait.c b/kernel/wait.c
index 791681c..5cdf169 100644
--- a/kernel/wait.c
+++ b/kernel/wait.c
@@ -33,13 +33,35 @@ void fastcall add_wait_queue_exclusive(w
 }
 EXPORT_SYMBOL(add_wait_queue_exclusive);
 
-void fastcall remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait)
+int fastcall remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait)
 {
 	unsigned long flags;
+	struct list_head *list;
+	int seen, retval;
 
 	spin_lock_irqsave(&q->lock, flags);
-	__remove_wait_queue(q, wait);
+	list = &q->task_list;
+	seen = 0;
+	retval = -1;
+
+	do {
+		struct list_head *next;
+		if (list == &wait->task_list)
+			seen++;
+		next = list->next;
+		if (next->prev != list) {
+			seen += 2;
+			break;
+		}
+		list = next;
+	} while (list != &q->task_list);
+
+	if (seen == 1) {
+		__remove_wait_queue(q, wait);
+		retval = 0;
+	}
 	spin_unlock_irqrestore(&q->lock, flags);
+	return retval;
 }
 EXPORT_SYMBOL(remove_wait_queue);
 

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

Re: memory corruptor in .18rc1-git

[Andrew Morton]

On Thu, 13 Jul 2006 18:13:30 -0400
Dave Jones <davej@redhat.com> wrote:

> Three times in the last week, I've had a box running the -git-du-jour
> spontaneously reboot.  It just happened again, this time I had a serial
> console hooked up, but it rebooted before transferring much data.
> The one thing it did spew however was "List corruption. prev->ne",
> which came from the patch below which I had in my tree.
> (The latter half is likely irrelevant, and came from chasing a different bug)
> 
> Things in common at all three times it happened..
> reading email, and listening to oggs with rhythmbox.
> Another ALSA bug maybe ?
> 
> I've up'd the speed of the serial console, in the hope that more chars
> make it over the wire before reboot should this happen again.

Are you using SMP?  We have a known slab locking bug.

There have been a couple of slab.c patches committed today, but neither of
them appear to actually fix the bug.

The below should fix it, and testing this (disable lockdep) would be
useful.

It's going to take a bit of work to unpickle it all now.

diff -puN mm/slab.c~revert-slabc-lockdep-locking-change mm/slab.c
--- a/mm/slab.c~revert-slabc-lockdep-locking-change
+++ a/mm/slab.c
@@ -3100,16 +3100,7 @@ static void free_block(struct kmem_cache
 		if (slabp->inuse == 0) {
 			if (l3->free_objects > l3->free_limit) {
 				l3->free_objects -= cachep->num;
-				/*
-				 * It is safe to drop the lock. The slab is
-				 * no longer linked to the cache. cachep
-				 * cannot disappear - we are using it and
-				 * all destruction of caches must be
-				 * serialized properly by the user.
-				 */
-				spin_unlock(&l3->list_lock);
 				slab_destroy(cachep, slabp);
-				spin_lock(&l3->list_lock);
 			} else {
 				list_add(&slabp->list, &l3->slabs_free);
 			}
_

Re: memory corruptor in .18rc1-git

[Dave Jones]

On Thu, Jul 13, 2006 at 03:24:25PM -0700, Andrew Morton wrote:

 > > I've up'd the speed of the serial console, in the hope that more chars
 > > make it over the wire before reboot should this happen again.
 > 
 > Are you using SMP?  We have a known slab locking bug.

Yes, dual EM64T's with HT.

 > There have been a couple of slab.c patches committed today, but neither of
 > them appear to actually fix the bug.
 > 
 > The below should fix it, and testing this (disable lockdep) would be
 > useful.

I can give it a shot, but as it takes a while for this to manifest, I may
not be able to say for certain whether it fixes it or not.

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

Re: memory corruptor in .18rc1-git

[Willy Tarreau]

On Thu, Jul 13, 2006 at 06:30:29PM -0400, Dave Jones wrote:
> On Thu, Jul 13, 2006 at 03:24:25PM -0700, Andrew Morton wrote:
> 
>  > > I've up'd the speed of the serial console, in the hope that more chars
>  > > make it over the wire before reboot should this happen again.
>  > 
>  > Are you using SMP?  We have a known slab locking bug.
> 
> Yes, dual EM64T's with HT.
> 
>  > There have been a couple of slab.c patches committed today, but neither of
>  > them appear to actually fix the bug.
>  > 
>  > The below should fix it, and testing this (disable lockdep) would be
>  > useful.
> 
> I can give it a shot, but as it takes a while for this to manifest, I may
> not be able to say for certain whether it fixes it or not.

Then you might consider slightly changing the debug messages, because they
are identical in list_add and list_del. Having a way to differenciate
between the two functions might give one more indication.

> 		Dave

Willy

Re: memory corruptor in .18rc1-git

[Dave Jones]

On Fri, Jul 14, 2006 at 06:12:54AM +0200, Willy Tarreau wrote:
 
 > > I can give it a shot, but as it takes a while for this to manifest, I may
 > > not be able to say for certain whether it fixes it or not.
 > 
 > Then you might consider slightly changing the debug messages, because they
 > are identical in list_add and list_del. Having a way to differenciate
 > between the two functions might give one more indication.

BUG() gives a line number.

		Dave

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

Re: memory corruptor in .18rc1-git

[Willy Tarreau]

On Fri, Jul 14, 2006 at 12:20:40AM -0400, Dave Jones wrote:
> On Fri, Jul 14, 2006 at 06:12:54AM +0200, Willy Tarreau wrote:
>  
>  > > I can give it a shot, but as it takes a while for this to manifest, I may
>  > > not be able to say for certain whether it fixes it or not.
>  > 
>  > Then you might consider slightly changing the debug messages, because they
>  > are identical in list_add and list_del. Having a way to differenciate
>  > between the two functions might give one more indication.
> 
> BUG() gives a line number.

oops! sorry, I did not notice it just after the printk(). Next time I will
not post before coffee :-)

> 		Dave

Willy

Re: memory corruptor in .18rc1-git

[Dave Jones]

On Fri, Jul 14, 2006 at 06:25:25AM +0200, Willy Tarreau wrote:
 > > > Then you might consider slightly changing the debug messages, because they
 > > > are identical in list_add and list_del. Having a way to differenciate
 > > > between the two functions might give one more indication.
 > > 
 > > BUG() gives a line number.
 > 
 > oops! sorry, I did not notice it just after the printk(). Next time I will
 > not post before coffee :-)

Though in the case I hit this morning, the text from the BUG() wouldn't
have made it over serial. But then, unless the first few chars were different,
not much else would have either. It didn't even complete the printk.

For most cases though, it should suffice. It gives enough clues that
'something stomped on memory' in cases like I saw, and in other cases
where the box still survives, it'll be possible to get the full context
from dmesg.  For info, here's a cleaned up variant of the patch with
some suggestions from Linus.  Andrew also merged an earlier variant into -mm.

		Dave

diff --git a/include/linux/list.h b/include/linux/list.h
index 6b74adf..26e5abc 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -57,10 +57,15 @@ static inline void __list_add(struct lis
  * Insert a new entry after the specified head.
  * This is good for implementing stacks.
  */
+#ifndef CONFIG_DEBUG_LIST
 static inline void list_add(struct list_head *new, struct list_head *head)
 {
 	__list_add(new, head, head->next);
 }
+#else
+extern void list_add(struct list_head *new, struct list_head *head);
+#endif
+
 
 /**
  * list_add_tail - add a new entry
@@ -153,12 +158,16 @@ static inline void __list_del(struct lis
  * Note: list_empty on entry does not return true after this, the entry is
  * in an undefined state.
  */
+#ifndef CONFIG_DEBUG_LIST
 static inline void list_del(struct list_head *entry)
 {
 	__list_del(entry->prev, entry->next);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;
 }
+#else
+extern void list_del(struct list_head *entry);
+#endif
 
 /**
  * list_del_rcu - deletes entry from list without re-initialization
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e5889b1..bb17ce3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -311,6 +311,15 @@ config DEBUG_VM
 
 	  If unsure, say N.
 
+config DEBUG_LIST
+	bool "Debug linked list manipulation"
+	depends on DEBUG_KERNEL
+	help
+	  Enable this to turn on extended checks in the linked-list
+	  walking routines.
+
+	  If unsure, say N.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	depends on DEBUG_KERNEL && (X86 || CRIS || M68K || M68KNOMMU || FRV || UML || S390)
diff --git a/lib/Makefile b/lib/Makefile
index be9719a..7d7da98 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -28,6 +28,7 @@ lib-$(CONFIG_GENERIC_HWEIGHT) += hweight
 obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o
 obj-$(CONFIG_PLIST) += plist.o
 obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
+obj-$(CONFIG_DEBUG_LIST) += list_debug.o
 
 ifneq ($(CONFIG_HAVE_DEC_LOCK),y)
   lib-y += dec_and_lock.o
diff --git a/lib/list_debug.c b/lib/list_debug.c
new file mode 100644
index 0000000..28b2d46
--- /dev/null
+++ b/lib/list_debug.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2006, Red Hat, Inc., Dave Jones
+ * Released under the General Public License (GPL).
+ *
+ * This file contains the linked list implementations for
+ * DEBUG_LIST.
+ */
+
+#include <linux/module.h>
+#include <linux/list.h>
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+void list_add(struct list_head *new, struct list_head *head)
+{
+	if (head->next->prev != head) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+			head, head->next->prev);
+		BUG();
+	}
+	if (head->prev->next != head) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+			head, head->prev->next);
+		BUG();
+	}
+
+	__list_add(new, head, head->next);
+}
+EXPORT_SYMBOL(list_add);
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+void list_del(struct list_head *entry)
+{
+	if (entry->prev->next != entry) {
+		printk("List corruption. prev->next should be %p, but was %p\n",
+			entry, entry->prev->next);
+		BUG();
+	}
+	if (entry->next->prev != entry) {
+		printk("List corruption. next->prev should be %p, but was %p\n",
+			entry, entry->next->prev);
+		BUG();
+	}
+	__list_del(entry->prev, entry->next);
+	entry->next = LIST_POISON1;
+	entry->prev = LIST_POISON2;
+}
+EXPORT_SYMBOL(list_del);
+
-- 
http://www.codemonkey.org.uk