Re: [patch 2/16] list_head debugging

[Andrew Morton]

Bernd Jendrissek wrote:
> [sorry for the nonexistent In-Reply-To/whatever headers - cutting&pasting]
> 
> Andrew Morton wrote:
> 
>>  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.
> 
> 
> I'm not current with the kernel tree, but will one such oops occur in
> netfilter?  See
> 
> http://lists.samba.org/pipermail/netfilter-announce/2002/000010.html
> 
> Hmm, no.  A DoS maybe?
> 

An oops, actually.  This code:

         /* Remove from both hash lists: must not NULL out next ptrs,
            otherwise we'll look unconfirmed.  Fortunately, LIST_DELETE
            doesn't do this. --RR */
         LIST_DELETE(&ip_conntrack_hash
                     [hash_conntrack(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple)],
                     &ct->tuplehash[IP_CT_DIR_ORIGINAL]);
         LIST_DELETE(&ip_conntrack_hash
                     [hash_conntrack(&ct->tuplehash[IP_CT_DIR_REPLY].tuple)],
                     &ct->tuplehash[IP_CT_DIR_REPLY]);

I think what is needed is:

--- 2.5.20/net/ipv4/netfilter/ip_conntrack_core.c~ipconntrack-lists	Fri Jun  7 11:26:38 2002
+++ 2.5.20-akpm/net/ipv4/netfilter/ip_conntrack_core.c	Fri Jun  7 11:26:42 2002
@@ -210,17 +210,22 @@ static void destroy_expectations(struct
  static void
  clean_from_lists(struct ip_conntrack *ct)
  {
+ 
struct list_head *l1;
+ 
struct list_head *l2;
+
  	DEBUGP("clean_from_lists(%p)\n", ct);
  	MUST_BE_WRITE_LOCKED(&ip_conntrack_lock);
- 
/* Remove from both hash lists: must not NULL out next ptrs,
-           otherwise we'll look unconfirmed.  Fortunately, LIST_DELETE
-           doesn't do this. --RR */
+
+ 
l1 = &ct->tuplehash[IP_CT_DIR_ORIGINAL];
+ 
l2 = &ct->tuplehash[IP_CT_DIR_REPLY];
+
  	LIST_DELETE(&ip_conntrack_hash
  		    [hash_conntrack(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple)],
- 
	    &ct->tuplehash[IP_CT_DIR_ORIGINAL]);
- 
LIST_DELETE(&ip_conntrack_hash
- 
	    [hash_conntrack(&ct->tuplehash[IP_CT_DIR_REPLY].tuple)],
- 
	    &ct->tuplehash[IP_CT_DIR_REPLY]);
+ 
	    l1);
+ 
if (l1 != l2)
+ 
	LIST_DELETE(&ip_conntrack_hash
+ 
		    [hash_conntrack(&ct->tuplehash[IP_CT_DIR_REPLY].tuple)],
+ 
		    l2);

  	/* Destroy all un-established, pending expectations */
  	destroy_expectations(ct);

-