[PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[Samuel Jean]

Name: avoid data corruption on checkentry
Status: Untested but compiles on Linux 2.6.10.
         Patch against 2.6.11-rc2-bk10
Signed-off-by: Samuel Jean <sjean@cookinglinux.org>

While processing checkentry(), we first get a lock, search for entry
and drop the lock. If we found the object, that's fine.
However, in the other case, we start creating the object, get a lock,
add the object and then release the lock. By this time, one could
already failed to find the same object and start creating it too.
This could lead to data corruption.

This patch fixes it by re-searching the object before adding it.
Also note that I dropped the rwlock as I think it's unecessary here.
Please someone correct me if I am wrong.

--- net/ipv4/netfilter/ipt_hashlimit.c.bk10     2005-02-01 20:41:33.000000000 -0500
+++ net/ipv4/netfilter/ipt_hashlimit.c  2005-02-01 22:49:48.000000000 -0500
@@ -97,7 +97,7 @@ struct ipt_hashlimit_htable {
         struct list_head hash[0];       /* hashtable itself */
  };

-static DECLARE_RWLOCK(hashlimit_lock); /* protects htables list */
+static DECLARE_LOCK(hashlimit_lock);   /* protects htables list */
  static LIST_HEAD(hashlimit_htables);
  static kmem_cache_t *hashlimit_cachep;

@@ -174,12 +174,17 @@ __dsthash_free(struct ipt_hashlimit_htab
         atomic_dec(&ht->count);
  }
  static void htable_gc(unsigned long htlong);
-
-static int htable_create(struct ipt_hashlimit_info *minfo)
+static struct ipt_hashlimit_htable *htable_find_get(char *name);
+static void htable_put(struct ipt_hashlimit_htable *hinfo);
+
+static struct ipt_hashlimit_htable *
+htable_create(struct ipt_hashlimit_info *minfo)
  {
         int i;
         unsigned int size;
-       struct ipt_hashlimit_htable *hinfo;
+       struct ipt_hashlimit_htable *hinfo, *s_hinfo;
+
+       MUST_BE_UNLOCKED(&hashlimit_lock);

         if (minfo->cfg.size)
                 size = minfo->cfg.size;
@@ -196,9 +201,8 @@ static int htable_create(struct ipt_hash
                         + (sizeof(struct list_head) * size));
         if (!hinfo) {
                 printk(KERN_ERR "ipt_hashlimit: Unable to create hashtable\n");
-               return -1;
+               return NULL;
         }
-       minfo->hinfo = hinfo;

         /* copy match config into hashtable config */
         memcpy(&hinfo->cfg, &minfo->cfg, sizeof(hinfo->cfg));
@@ -215,10 +219,12 @@ static int htable_create(struct ipt_hash
         atomic_set(&hinfo->use, 1);
         hinfo->rnd = 0;
         spin_lock_init(&hinfo->lock);
+
+       /* FIXME: This could lead to duplicated proc entries, no ? --peejix */
         hinfo->pde = create_proc_entry(minfo->name, 0, hashlimit_procdir);
         if (!hinfo->pde) {
                 vfree(hinfo);
-               return -1;
+               return NULL;
         }
         hinfo->pde->proc_fops = &dl_file_ops;
         hinfo->pde->data = hinfo;
@@ -229,11 +235,22 @@ static int htable_create(struct ipt_hash
         hinfo->timer.function = htable_gc;
         add_timer(&hinfo->timer);

-       WRITE_LOCK(&hashlimit_lock);
+       LOCK_BH(&hashlimit_lock);
+       if ((s_hinfo = htable_find_get(minfo->name))) {
+               /* avoid data corruption */
+               UNLOCK_BH(&hashlimit_lock);
+               remove_proc_entry(minfo->name, hashlimit_procdir);
+               if (timer_pending(&hinfo->timer))
+                       del_timer(&hinfo->timer);
+
+               vfree(hinfo);
+               return s_hinfo;
+       }
+
         list_add(&hinfo->list, &hashlimit_htables);
-       WRITE_UNLOCK(&hashlimit_lock);
+       UNLOCK_BH(&hashlimit_lock);

-       return 0;
+       return hinfo;
  }

  static int select_all(struct ipt_hashlimit_htable *ht, struct dsthash_ent *he)
@@ -295,15 +312,14 @@ static struct ipt_hashlimit_htable *htab
  {
         struct ipt_hashlimit_htable *hinfo;

-       READ_LOCK(&hashlimit_lock);
+       MUST_BE_LOCKED(&hashlimit_lock);
+
         list_for_each_entry(hinfo, &hashlimit_htables, list) {
                 if (!strcmp(name, hinfo->pde->name)) {
                         atomic_inc(&hinfo->use);
-                       READ_UNLOCK(&hashlimit_lock);
                         return hinfo;
                 }
         }
-       READ_UNLOCK(&hashlimit_lock);

         return NULL;
  }
@@ -311,9 +327,9 @@ static struct ipt_hashlimit_htable *htab
  static void htable_put(struct ipt_hashlimit_htable *hinfo)
  {
         if (atomic_dec_and_test(&hinfo->use)) {
-               WRITE_LOCK(&hashlimit_lock);
+               LOCK_BH(&hashlimit_lock);
                 list_del(&hinfo->list);
-               WRITE_UNLOCK(&hashlimit_lock);
+               UNLOCK_BH(&hashlimit_lock);
                 htable_destroy(hinfo);
         }
  }
@@ -531,8 +547,11 @@ hashlimit_checkentry(const char *tablena
         if (!r->cfg.expire)
                 return 0;

+       LOCK_BH(&hashlimit_lock);
         r->hinfo = htable_find_get(r->name);
-       if (!r->hinfo && (htable_create(r) != 0)) {
+       UNLOCK_BH(&hashlimit_lock);
+
+       if (!r->hinfo && ((r->hinfo = htable_create(r)) == NULL)) {
                 return 0;
         }

Re: [PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[Samuel Jean]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

Samuel Jean wrote:
> --- net/ipv4/netfilter/ipt_hashlimit.c.bk10     2005-02-01 
> 20:41:33.000000000 -0500
> +++ net/ipv4/netfilter/ipt_hashlimit.c  2005-02-01 22:49:48.000000000 -0500
> @@ -97,7 +97,7 @@ struct ipt_hashlimit_htable {
>         struct list_head hash[0];       /* hashtable itself */
>  };
> 
> -static DECLARE_RWLOCK(hashlimit_lock); /* protects htables list */
> +static DECLARE_LOCK(hashlimit_lock);   /* protects htables list */
>  static LIST_HEAD(hashlimit_htables);
>  static kmem_cache_t *hashlimit_cachep;
> 
> @@ -174,12 +174,17 @@ __dsthash_free(struct ipt_hashlimit_htab
>         atomic_dec(&ht->count);
>  }
>  static void htable_gc(unsigned long htlong);
> -
> -static int htable_create(struct ipt_hashlimit_info *minfo)
> +static struct ipt_hashlimit_htable *htable_find_get(char *name);

> +static void htable_put(struct ipt_hashlimit_htable *hinfo);
Please remove this ^^
or get the attached patch. Sorry, bad code review..

[-- Attachment #2: ipt_hashlimit.c-avoid-data-corruption.patch --]
[-- Type: text/x-patch, Size: 3603 bytes --]

--- net/ipv4/netfilter/ipt_hashlimit.c.bk10	2005-02-01 20:41:33.000000000 -0500
+++ net/ipv4/netfilter/ipt_hashlimit.c	2005-02-01 23:12:02.000000000 -0500
@@ -97,7 +97,7 @@ struct ipt_hashlimit_htable {
 	struct list_head hash[0];	/* hashtable itself */
 };
 
-static DECLARE_RWLOCK(hashlimit_lock);	/* protects htables list */
+static DECLARE_LOCK(hashlimit_lock);	/* protects htables list */
 static LIST_HEAD(hashlimit_htables);
 static kmem_cache_t *hashlimit_cachep;
 
@@ -174,12 +174,16 @@ __dsthash_free(struct ipt_hashlimit_htab
 	atomic_dec(&ht->count);
 }
 static void htable_gc(unsigned long htlong);
-
-static int htable_create(struct ipt_hashlimit_info *minfo)
+static struct ipt_hashlimit_htable *htable_find_get(char *name);
+	
+static struct ipt_hashlimit_htable *
+htable_create(struct ipt_hashlimit_info *minfo)
 {
 	int i;
 	unsigned int size;
-	struct ipt_hashlimit_htable *hinfo;
+	struct ipt_hashlimit_htable *hinfo, *s_hinfo;
+
+	MUST_BE_UNLOCKED(&hashlimit_lock);
 
 	if (minfo->cfg.size)
 		size = minfo->cfg.size;
@@ -196,9 +200,8 @@ static int htable_create(struct ipt_hash
 			+ (sizeof(struct list_head) * size));
 	if (!hinfo) {
 		printk(KERN_ERR "ipt_hashlimit: Unable to create hashtable\n");
-		return -1;
+		return NULL;
 	}
-	minfo->hinfo = hinfo;
 
 	/* copy match config into hashtable config */
 	memcpy(&hinfo->cfg, &minfo->cfg, sizeof(hinfo->cfg));
@@ -215,10 +218,12 @@ static int htable_create(struct ipt_hash
 	atomic_set(&hinfo->use, 1);
 	hinfo->rnd = 0;
 	spin_lock_init(&hinfo->lock);
+
+	/* FIXME: This could lead to duplicated proc entries, no ? --peejix */
 	hinfo->pde = create_proc_entry(minfo->name, 0, hashlimit_procdir);
 	if (!hinfo->pde) {
 		vfree(hinfo);
-		return -1;
+		return NULL;
 	}
 	hinfo->pde->proc_fops = &dl_file_ops;
 	hinfo->pde->data = hinfo;
@@ -229,11 +234,22 @@ static int htable_create(struct ipt_hash
 	hinfo->timer.function = htable_gc;
 	add_timer(&hinfo->timer);
 
-	WRITE_LOCK(&hashlimit_lock);
+	LOCK_BH(&hashlimit_lock);
+	if ((s_hinfo = htable_find_get(minfo->name))) {
+		/* avoid data corruption */
+		UNLOCK_BH(&hashlimit_lock);
+		remove_proc_entry(minfo->name, hashlimit_procdir);
+		if (timer_pending(&hinfo->timer))
+			del_timer(&hinfo->timer);
+		
+		vfree(hinfo);
+		return s_hinfo;
+	}
+	
 	list_add(&hinfo->list, &hashlimit_htables);
-	WRITE_UNLOCK(&hashlimit_lock);
+	UNLOCK_BH(&hashlimit_lock);
 
-	return 0;
+	return hinfo;
 }
 
 static int select_all(struct ipt_hashlimit_htable *ht, struct dsthash_ent *he)
@@ -295,15 +311,14 @@ static struct ipt_hashlimit_htable *htab
 {
 	struct ipt_hashlimit_htable *hinfo;
 
-	READ_LOCK(&hashlimit_lock);
+	MUST_BE_LOCKED(&hashlimit_lock);
+	
 	list_for_each_entry(hinfo, &hashlimit_htables, list) {
 		if (!strcmp(name, hinfo->pde->name)) {
 			atomic_inc(&hinfo->use);
-			READ_UNLOCK(&hashlimit_lock);
 			return hinfo;
 		}
 	}
-	READ_UNLOCK(&hashlimit_lock);
 
 	return NULL;
 }
@@ -311,9 +326,9 @@ static struct ipt_hashlimit_htable *htab
 static void htable_put(struct ipt_hashlimit_htable *hinfo)
 {
 	if (atomic_dec_and_test(&hinfo->use)) {
-		WRITE_LOCK(&hashlimit_lock);
+		LOCK_BH(&hashlimit_lock);
 		list_del(&hinfo->list);
-		WRITE_UNLOCK(&hashlimit_lock);
+		UNLOCK_BH(&hashlimit_lock);
 		htable_destroy(hinfo);
 	}
 }
@@ -531,8 +546,11 @@ hashlimit_checkentry(const char *tablena
 	if (!r->cfg.expire)
 		return 0;
 
+	LOCK_BH(&hashlimit_lock);
 	r->hinfo = htable_find_get(r->name);
-	if (!r->hinfo && (htable_create(r) != 0)) {
+	UNLOCK_BH(&hashlimit_lock);
+	
+	if (!r->hinfo && ((r->hinfo = htable_create(r)) == NULL)) {
 		return 0;
 	}
 

Re: [PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 1198 bytes --]

On Tue, Feb 01, 2005 at 11:07:52PM -0500, Samuel Jean wrote:
 
> While processing checkentry(), we first get a lock, search for entry
> and drop the lock. If we found the object, that's fine.
> However, in the other case, we start creating the object, get a lock,
> add the object and then release the lock. By this time, one could
> already failed to find the same object and start creating it too.
> This could lead to data corruption.

Thanks for pointing it out, I was under the (wrong) impression that
checkentry() (called via translate_table()) was already synchronized by
ipt_mutex.

Please consider / comment the following (untested) patch.

Not really sure whether nesting the rwlock inside the mutex is that much
of a good idea... but as the comment indicates, I think it's the best we
can get.


-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #1.2: linux-2.6.11-rc4-hashlimit_checkentry_race.patch --]
[-- Type: text/plain, Size: 1128 bytes --]

--- c/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-13 13:52:06.000000000 +0100
+++ d/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-15 01:49:39.086401888 +0100
@@ -98,6 +98,7 @@
 };
 
 static DECLARE_RWLOCK(hashlimit_lock);	/* protects htables list */
+static DECLARE_MUTEX(hlimit_mutex);	/* additional checkentry protection */
 static LIST_HEAD(hashlimit_htables);
 static kmem_cache_t *hashlimit_cachep;
 
@@ -531,10 +532,19 @@
 	if (!r->cfg.expire)
 		return 0;
 
+	/* This is the best we've got: We cannot release and re-grab lock,
+	 * since checkentry() is called before ip_tables.c grabs ipt_mutex.  
+	 * We also cannot grab the hashtable spinlock, since htable_create will 
+	 * call vmalloc, and that can sleep.  And we cannot just re-search
+	 * the list of htable's in htable_create(), since then we would
+	 * create duplicate proc files. -HW */
+	down(&hlimit_mutex);
 	r->hinfo = htable_find_get(r->name);
 	if (!r->hinfo && (htable_create(r) != 0)) {
+		up(&hlimit_mutex);
 		return 0;
 	}
+	up(&hlimit_mutex);
 
 	/* Ugly hack: For SMP, we only want to use one set */
 	r->u.master = r;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[Samuel Jean]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

*Sorry if you get it twice. Looks like I sent from wrong email.*

Harald Welte wrote:

> Please consider / comment the following (untested) patch.

Yes. That's much less complicated than the other proposed patch.

> 
> Not really sure whether nesting the rwlock inside the mutex is that much
> of a good idea... but as the comment indicates, I think it's the best we
> can get.

I think the best we actually can get, is also replacing those rwlock's with
simple lock's. As you said on IRC, that's a bit overkill.

Incremental (also untested, but compiles) patch to your is attached.




[-- Attachment #2: linux-2.6.11-rc4-bk2-incremental_get_rid_rwlock.patch --]
[-- Type: text/x-patch, Size: 1738 bytes --]

#
#Signed-off-by: Samuel Jean <sjean@cookinglinux.org>
#
--- d/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-14 20:48:33.000000000 -0500
+++ e/net/ipv4/netfilter/ipt_hashlimit.c	2005-02-14 21:00:07.000000000 -0500
@@ -97,7 +97,7 @@ struct ipt_hashlimit_htable {
 	struct list_head hash[0];	/* hashtable itself */
 };
 
-static DECLARE_RWLOCK(hashlimit_lock);	/* protects htables list */
+static DECLARE_LOCK(hashlimit_lock);	/* protects htables list */
 static DECLARE_MUTEX(hlimit_mutex);	/* additional checkentry protection */
 static LIST_HEAD(hashlimit_htables);
 static kmem_cache_t *hashlimit_cachep;
@@ -230,9 +230,9 @@ static int htable_create(struct ipt_hash
 	hinfo->timer.function = htable_gc;
 	add_timer(&hinfo->timer);
 
-	WRITE_LOCK(&hashlimit_lock);
+	LOCK_BH(&hashlimit_lock);
 	list_add(&hinfo->list, &hashlimit_htables);
-	WRITE_UNLOCK(&hashlimit_lock);
+	UNLOCK_BH(&hashlimit_lock);
 
 	return 0;
 }
@@ -296,15 +296,15 @@ static struct ipt_hashlimit_htable *htab
 {
 	struct ipt_hashlimit_htable *hinfo;
 
-	READ_LOCK(&hashlimit_lock);
+	LOCK_BH(&hashlimit_lock);
 	list_for_each_entry(hinfo, &hashlimit_htables, list) {
 		if (!strcmp(name, hinfo->pde->name)) {
 			atomic_inc(&hinfo->use);
-			READ_UNLOCK(&hashlimit_lock);
+			UNLOCK_BH(&hashlimit_lock);
 			return hinfo;
 		}
 	}
-	READ_UNLOCK(&hashlimit_lock);
+	UNLOCK_BH(&hashlimit_lock);
 
 	return NULL;
 }
@@ -312,9 +312,9 @@ static struct ipt_hashlimit_htable *htab
 static void htable_put(struct ipt_hashlimit_htable *hinfo)
 {
 	if (atomic_dec_and_test(&hinfo->use)) {
-		WRITE_LOCK(&hashlimit_lock);
+		LOCK_BH(&hashlimit_lock);
 		list_del(&hinfo->list);
-		WRITE_UNLOCK(&hashlimit_lock);
+		UNLOCK_BH(&hashlimit_lock);
 		htable_destroy(hinfo);
 	}
 }

Re: [PATCH] ipt_hashlimit.c: avoid data corruption on checkentry

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Mon, Feb 14, 2005 at 11:04:19PM -0500, Samuel Jean wrote:

> Incremental (also untested, but compiles) patch to your is attached.

thanks, applied to svn and pending for kernel submission.
-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]