list_head debugging
PATCH 2/16 list_head debugging
A common and very subtle bug is to use list_heads which aren't on any lists. It causes kernel memory corruption which is observed long after the offending code has executed.
The patch nulls out the dangling pointers so we get a nice oops at the site of the buggy code.
=====================================
include/linux/list.h renamed from include/linux/list.h~list-debug +5 -0
@@ -94,6 +94,11 @@ static __inline__ void __list_del(structstatic __inline__void list_del (struct list_head *entry) {__list_del (entry->prev, entry->next); +/* +* This is debug. Remove it when the kernel has no bugs ;) +*/ + entry->next =0 ; + entry->prev =0 ; }/**
-
Em Sat, Jun 01, 2002 at 01:40:03AM -0700, Andrew Morton escreveu:
Quoted 12 lines
A common and very subtle bug is to use
list_headswhich aren't on any lists. It causes kernel memory corruption which is observed long after the offending code has executed.The patch nulls out the dangling pointers so we get a nice oops at the site of the buggy code.
=====================================
include/linux/list.h static __inline__ void __list_del(struct · line 96
Quoted 3 lines
include/linux/list.h renamed from include/linux/list.h~list-debug +0
-0@@ -94,6 +94,11 @@ static __inline__ void __list_del(structstatic __inline__void list_del (struct list_head *entry) {__list_del (entry->prev, entry->next);
#ifdef CONFIG_DEBUG_LIST_DEL_NULLIFY
include/linux/list.h static __inline__ void __list_del(struct · line 101
Quoted 2 lines
include/linux/list.h renamed from include/linux/list.h~list-debug +2
-0+ entry->next =0 ; + entry->prev =0 ;
#endif
include/linux/list.h static __inline__ void __list_del(struct · line 99
Quoted 1 line
include/linux/list.h renamed from include/linux/list.h~list-debug +0
-0}
8) And get this configured in the Debug section of make *config
The kernel will always have bugs ;)
- Arnaldo
Quoted 2 lines · Andrew Morton
The patch nulls out the dangling pointers so we get a nice oops at the site of the buggy code.
...
include/linux/list.h static __inline__ void __list_del(struct · line 102
Quoted 9 lines
static
__inline__voidlist_del(struct list_head *entry){include/linux/list.h renamed from include/linux/list.h~list-debug +5
-0__list_del (entry->prev, entry->next); +/* +* This is debug. Remove it when the kernel has no bugs ;) +*/ + entry->next =0 ; + entry->prev =0 ; }
We've had this before, and it breaks some code that removes items from lists as follows,
list_for_each(p, list) if (condition) list_del(p);
These would have to either use __list_del, or need to do,
for(p = list.next; p != &list;) {
struct list_head *n = p->next;
if (condition)
list_del(p);
p = n;
}I'm not sure how many places are using the list_for_each/list_del construction, but there were a couple when this was in the tree previously. Converting most places that use list_del to use list_del_init should fix the same bugs, but not cause problems for existing code.
Just did a grep for list_del and didn't find any obvious places where we are doing the above construction, except for drivers/isdn/capi/capilib.c and maybe drivers/hotplug/pcihp_skeleton.c, but it could be hidden in many more places by a macro or function call (or a larger loop than my 3 line context was showing). But there are not that many places where we're calling list_del while not immediately re-initializing or adding the unlinked list_head to another list.
You could probably also add list_move
list_move(entry, head)
{
if (!list_empty(entry))
__list_del(entry->prev, entry->next);
list_add(entry, head);
}And just delete list_del completely, because all existing places where list_del is currently used should probably use either list_del_init, or list_move.
Jan
Quoted 4 lines · Jan Harkes
Quoted 4 lines
static
__inline__voidlist_del(struct list_head *entry){include/linux/list.h renamed from include/linux/list.h~list-debug +1
-0__list_del (entry->prev, entry->next); +/*
Quoted 8 lines
include/linux/list.h static __inline__ void __list_del(struct · line 102
Quoted 5 lines
include/linux/list.h renamed from include/linux/list.h~list-debug +4
-0+* This is debug. Remove it when the kernel has no bugs ;) +*/ + entry->next =0 ; + entry->prev =0 ; }We've had this before, and it breaks some code that removes items from lists as follows,
Such code is probably not SMP safe anyway.
regards,
Rik
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Quoted 4 lines · Rik van Riel
Quoted 2 lines
We've had this before, and it breaks some code that removes items from lists as follows,
Such code is probably not SMP safe anyway.
Where are you coming from with that comment?
down(&semaphore);
list_for_each(p, list)
if (condition) list_del(p);
up(&semaphore);
Should be completely SMP safe, or we have more serious problems than I even imagined. And it worked just fine before the 'zero pointers in list_del'-patch was included and is used in _at least_ two places in the existing kernel, probably more.
And list_for_each_safe might work when list_del is zero-ing pointers, but imho has an ugly interface, it still doesn't fix SMP issues. You still need to prevent concurrent modifications, otherwise someone could just as well remove the curr->next object and you corrupt/crash on dereferencing the saved next pointer.
In fact this saved next pointer will far more likely to lead to obscure and hard to debug crashes compared to leaving prev/next as they are in the existing list_del function, just because people will think it is a 'safe' list iterator and forget about correctly locking their lists down.
Jan
Quoted 10 lines · Jan Harkes
Quoted 4 lines · Rik van Riel
Quoted 2 lines
We've had this before, and it breaks some code that removes items from lists as follows,
Such code is probably not SMP safe anyway.
Where are you coming from with that comment?
down(&semaphore);
Quoted 7 lines
list_for_each(p, list)if (condition) list_del(p);up(&semaphore);Should be completely SMP safe,
Not if 'p' comes from the slab cache.
In that case 'p' can be re-allocated on another CPU before we dereference ->next ...
regards,
Rik
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Quoted 15 lines · Jan Harkes
Quoted 2 lines · Andrew Morton
The patch nulls out the dangling pointers so we get a nice oops at the site of the buggy code.
...
include/linux/list.h static __inline__ void __list_del(struct · line 102
Quoted 9 lines
static
__inline__voidlist_del(struct list_head *entry){ __list_del(entry->prev, entry->next);include/linux/list.h renamed from include/linux/list.h~list-debug +5
-0+/* +* This is debug. Remove it when the kernel has no bugs ;) +*/ + entry->next =0 ; + entry->prev =0 ; }
Quoted 6 lines
We've had this before, and it breaks some code that removes items from lists as follows,
list_for_each(p, list) if (condition) list_del(p);
hmm. I suppose that's sane.
Quoted 8 lines
These would have to either use
__list_del, or need to do,for(p = list.next; p != &list;) { struct list_head *n = p->next; if (condition) list_del(p); p = n; }
list_for_each_safe() does this.