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

[Samuel Jean <sj-netfilter@cookinglinux.org>]

[-- 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);
 	}
 }