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

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

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