Re: [PATCH] netfilter: xt_hashlimit: handle iptables-restore of hash with same name

[Holger Eitzenberger <holger@eitzenberger.org>]

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


Sorry for answering late.  I somehow managed to ignore the thread.

First of all: I have the same problem with my ruleset, and I have
fixed it with the patch attached, fixing my use-case.

> >True, but it's a bug that has existed forever and I've seen scripts that actually rely on this.
> >
> >I'm not sure if we can silently change this behaviour.
> 
> Can you elaborate on what behavior they're relying on? It'd be
> helpful to know in case my approach can't be used we might be able
> to come up with an alternative.

There are two cases to consider: 1) the case Patrick and Florian are
talking about: two rules using the same hashtable <NAME>, but with
different parameters, and 2) updating an existing hashtable <NAME>,
with a single or multiple rules using it.

For case 1) the current behaviour actually makes more sense: the
first rule using hashtable <NAME> determines the parameters actually
being used.  Any subsequent rule using hashtable <NAME> automatically
uses the same parameters, even if specifying different parameters in
iptables-restore.

For case 2) the behaviour is unexpected: when using iptables-restore
to update an already existing hashtable <NAME> the updates are
ignored.

And from your description my understanding is that the latter case
is what you tried to solve with your patch.  And this is also what I
tried to solve with my patch.  In my case I simply have to make sure
when writing each rule that I use each hashtable only once.

Find attached the version I am currently using.

Cheers,

/Holger


[-- Attachment #2: hashlimit-fix-update.diff --]
[-- Type: text/x-diff, Size: 2795 bytes --]

Index: linux-3.8.y/net/netfilter/xt_hashlimit.c
===================================================================
--- linux-3.8.y.orig/net/netfilter/xt_hashlimit.c
+++ linux-3.8.y/net/netfilter/xt_hashlimit.c
@@ -214,6 +214,18 @@ dsthash_free(struct xt_hashlimit_htable
 }
 static void htable_gc(unsigned long htlong);
 
+static void
+htable_update_cfg(struct xt_hashlimit_htable *hinfo,
+		  const struct xt_hashlimit_mtinfo1 *minfo)
+{
+	/* copy match config into hashtable config */
+	memcpy(&hinfo->cfg, &minfo->cfg, sizeof(hinfo->cfg));
+	if (hinfo->cfg.max == 0)
+		hinfo->cfg.max = 8 * hinfo->cfg.size;
+	else if (hinfo->cfg.max < hinfo->cfg.size)
+		hinfo->cfg.max = hinfo->cfg.size;
+}
+
 static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo,
 			 u_int8_t family)
 {
@@ -239,13 +251,8 @@ static int htable_create(struct net *net
 		return -ENOMEM;
 	minfo->hinfo = hinfo;
 
-	/* copy match config into hashtable config */
-	memcpy(&hinfo->cfg, &minfo->cfg, sizeof(hinfo->cfg));
 	hinfo->cfg.size = size;
-	if (hinfo->cfg.max == 0)
-		hinfo->cfg.max = 8 * hinfo->cfg.size;
-	else if (hinfo->cfg.max < hinfo->cfg.size)
-		hinfo->cfg.max = hinfo->cfg.size;
+	htable_update_cfg(hinfo, minfo);
 
 	for (i = 0; i < hinfo->cfg.size; i++)
 		INIT_HLIST_HEAD(&hinfo->hash[i]);
@@ -318,6 +325,27 @@ static void htable_gc(unsigned long htlo
 	add_timer(&ht->timer);
 }
 
+static int
+htable_update(struct xt_hashlimit_mtinfo1 *minfo,
+	      u_int8_t family)
+{
+	struct xt_hashlimit_htable *hinfo = minfo->hinfo;
+
+	if (hinfo == NULL)
+		return -ENOENT;
+
+	if (minfo->cfg.size && hinfo->cfg.size != minfo->cfg.size)
+		return -EBUSY;
+	if (hinfo->family != family)
+		return -EBUSY;
+
+	hinfo->use++;
+	htable_update_cfg(hinfo, minfo);
+	htable_selective_cleanup(hinfo, select_all);
+
+	return 0;
+}
+
 static void htable_destroy(struct xt_hashlimit_htable *hinfo)
 {
 	struct hashlimit_net *hashlimit_net = hashlimit_pernet(hinfo->net);
@@ -691,20 +719,28 @@ static int hashlimit_mt_check(const stru
 	info->hinfo = htable_find_get(net, info->name, par->family);
 	if (info->hinfo == NULL) {
 		ret = htable_create(net, info, par->family);
-		if (ret < 0) {
-			mutex_unlock(&hashlimit_mutex);
-			return ret;
-		}
+		if (ret < 0)
+			goto err_unlock;
+	} else {
+		ret = htable_update(info, par->family);
+		if (ret < 0)
+			goto err_unlock;
 	}
+
 	mutex_unlock(&hashlimit_mutex);
 	return 0;
+
+err_unlock:
+	mutex_unlock(&hashlimit_mutex);
+	return ret;
 }
 
 static void hashlimit_mt_destroy(const struct xt_mtdtor_param *par)
 {
-	const struct xt_hashlimit_mtinfo1 *info = par->matchinfo;
+	struct xt_hashlimit_mtinfo1 *info = par->matchinfo;
 
 	htable_put(info->hinfo);
+	info->hinfo = NULL;
 }
 
 static struct xt_match hashlimit_mt_reg[] __read_mostly = {