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

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

[Josh Hunt]

Below is a first pass attempt at fixing a problem we've come across when
trying to do an iptables-restore where the hashlimit name stays the same, but
one of the hashlimit parameters changes but does not take affect.

For ex, if you have an existing hashlimit rule, do an iptables-save, change the
rate for that rule, and then do an iptables-restore the new rate will not be
enforced.

This appears to be due to a problem where hashlimit only checks for existing
hashes by name and family and does not consider any of the other config
parameters.

I've attempted to fix this by having it check for all hashlimit config params,
this way it doesn't accidentally match just on name. This brought up an issue
of having to make hashlimit aware of how many references there are to its
proc entry.

I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach
and if there's possibly a better way of resolving this problem. My handling of
the proc "problem" is pretty messy right now and possibly incomplete, but the
patch below allows the case I described above to pass now. I hope to clean up
the proc handling in a v2.

Any feedback is greatly appreciated.

Thanks
Josh
---

    netfilter: xt_hashlimit: handle iptables-restore of hash with same name

    We encountered a problem when trying to do an iptables-restore of a ruleset
    which included a hashlimit match where the name stays the same and one of the
    other config parameters changed, for ex:

    from:
    -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT

    to:
    -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 1000/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT

    Currently when we do do this the new parameters are not enforced.

    The problem stems from how htable_find_get() only checks by name and family.
    This causes htable_find_get() to return the old hash, instead of creating a
    new one.

    I've attempted to resolve this by having htable_find_get() check all config
    parameters b/f it returns a match. In addition, we have to keep the proc
    file around since it doesn't change b/c the hashes have the same name.

    Here's an example of the rules file:
        *filter
        :INPUT ACCEPT [1:1108]
        :FORWARD ACCEPT [0:0]
        :OUTPUT ACCEPT [0:0]
        -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT
        COMMIT

    Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 net/netfilter/xt_hashlimit.c |   87 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index a3910fc..5518018 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -215,6 +215,39 @@ dsthash_free(struct xt_hashlimit_htable *ht, struct dsthash_ent *ent)
 }
 static void htable_gc(unsigned long htlong);
 
+static int htable_count_name(struct net *net,
+                             const char *name,
+                             u_int8_t family)
+{
+	struct hashlimit_net *hashlimit_net = hashlimit_pernet(net);
+	struct xt_hashlimit_htable *hinfo;
+	int count = 0;
+
+	hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) {
+		if (!strcmp(name, hinfo->name) &&
+			family == hinfo->family) {
+			count++;
+		}
+	}
+	return count;
+}
+
+static struct proc_dir_entry *htable_get_pde(struct net *net,
+                                       const char *name,
+                                       u_int8_t family)
+{
+	struct hashlimit_net *hashlimit_net = hashlimit_pernet(net);
+	struct xt_hashlimit_htable *hinfo;
+
+	hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) {
+		if (!strcmp(name, hinfo->name) &&
+			family == hinfo->family) {
+			return hinfo->pde;
+		}
+	}
+	return NULL;
+}
+
 static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo,
 			 u_int8_t family)
 {
@@ -262,10 +295,13 @@ static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo,
 	}
 	spin_lock_init(&hinfo->lock);
 
-	hinfo->pde = proc_create_data(minfo->name, 0,
-		(family == NFPROTO_IPV4) ?
-		hashlimit_net->ipt_hashlimit : hashlimit_net->ip6t_hashlimit,
-		&dl_file_ops, hinfo);
+	hinfo->pde = htable_get_pde(net, minfo->name, family);
+	if (hinfo->pde == NULL) {
+		hinfo->pde = proc_create_data(minfo->name, 0,
+			(family == NFPROTO_IPV4) ?
+			hashlimit_net->ipt_hashlimit : hashlimit_net->ip6t_hashlimit,
+			&dl_file_ops, hinfo);
+	}
 	if (hinfo->pde == NULL) {
 		kfree(hinfo->name);
 		vfree(hinfo);
@@ -339,25 +375,50 @@ static void htable_remove_proc_entry(struct xt_hashlimit_htable *hinfo)
 		remove_proc_entry(hinfo->name, parent);
 }
 
-static void htable_destroy(struct xt_hashlimit_htable *hinfo)
+static void htable_destroy(struct xt_hashlimit_htable *hinfo, bool remove_proc)
 {
 	del_timer_sync(&hinfo->timer);
-	htable_remove_proc_entry(hinfo);
+	if (remove_proc)
+		htable_remove_proc_entry(hinfo);
 	htable_selective_cleanup(hinfo, select_all);
 	kfree(hinfo->name);
 	vfree(hinfo);
 }
 
+static bool htable_compare(struct xt_hashlimit_mtinfo1 *info,
+				u_int8_t family,
+				struct xt_hashlimit_htable *hinfo)
+{
+	struct hashlimit_cfg1 newcfg = info->cfg;
+	struct hashlimit_cfg1 oldcfg = hinfo->cfg;
+
+	if (!strcmp(info->name, hinfo->name) &&
+		family == hinfo->family &&
+		newcfg.mode == oldcfg.mode &&
+		newcfg.avg == oldcfg.avg &&
+		newcfg.burst == oldcfg.burst &&
+		newcfg.size == oldcfg.size &&
+		newcfg.max == oldcfg.max &&
+		newcfg.gc_interval == oldcfg.gc_interval &&
+		newcfg.expire == oldcfg.expire &&
+		newcfg.srcmask == oldcfg.srcmask &&
+		newcfg.dstmask == oldcfg.dstmask ) {
+
+		return true;
+	}
+
+	return false;
+}
+
 static struct xt_hashlimit_htable *htable_find_get(struct net *net,
-						   const char *name,
+						   struct xt_hashlimit_mtinfo1 *info,
 						   u_int8_t family)
 {
 	struct hashlimit_net *hashlimit_net = hashlimit_pernet(net);
 	struct xt_hashlimit_htable *hinfo;
 
 	hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) {
-		if (!strcmp(name, hinfo->name) &&
-		    hinfo->family == family) {
+		if (htable_compare(info, family, hinfo)) {
 			hinfo->use++;
 			return hinfo;
 		}
@@ -367,10 +428,14 @@ static struct xt_hashlimit_htable *htable_find_get(struct net *net,
 
 static void htable_put(struct xt_hashlimit_htable *hinfo)
 {
+	bool remove_proc = false;
+
 	mutex_lock(&hashlimit_mutex);
 	if (--hinfo->use == 0) {
+		if (htable_count_name(hinfo->net, hinfo->name, hinfo->family) == 1)
+			remove_proc = true;
 		hlist_del(&hinfo->node);
-		htable_destroy(hinfo);
+		htable_destroy(hinfo, remove_proc);
 	}
 	mutex_unlock(&hashlimit_mutex);
 }
@@ -698,7 +763,7 @@ static int hashlimit_mt_check(const struct xt_mtchk_param *par)
 	}
 
 	mutex_lock(&hashlimit_mutex);
-	info->hinfo = htable_find_get(net, info->name, par->family);
+	info->hinfo = htable_find_get(net, info, par->family);
 	if (info->hinfo == NULL) {
 		ret = htable_create(net, info, par->family);
 		if (ret < 0) {
-- 
1.7.9.5

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

[Josh Hunt]

On 07/21/2014 11:54 PM, Josh Hunt wrote:
> Below is a first pass attempt at fixing a problem we've come across when
> trying to do an iptables-restore where the hashlimit name stays the same, but
> one of the hashlimit parameters changes but does not take affect.
>
> For ex, if you have an existing hashlimit rule, do an iptables-save, change the
> rate for that rule, and then do an iptables-restore the new rate will not be
> enforced.
>
> This appears to be due to a problem where hashlimit only checks for existing
> hashes by name and family and does not consider any of the other config
> parameters.
>
> I've attempted to fix this by having it check for all hashlimit config params,
> this way it doesn't accidentally match just on name. This brought up an issue
> of having to make hashlimit aware of how many references there are to its
> proc entry.
>
> I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach
> and if there's possibly a better way of resolving this problem. My handling of
> the proc "problem" is pretty messy right now and possibly incomplete, but the
> patch below allows the case I described above to pass now. I hope to clean up
> the proc handling in a v2.

I just realized that what I'm doing with the proc stuff isn't going to 
work, but feedback on the other portion is still appreciated.

Thanks
Josh

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

[Florian Westphal]

Josh Hunt <johunt@akamai.com> wrote:
> Below is a first pass attempt at fixing a problem we've come across when
> trying to do an iptables-restore where the hashlimit name stays the same, but
> one of the hashlimit parameters changes but does not take affect.
> 
> For ex, if you have an existing hashlimit rule, do an iptables-save, change the
> rate for that rule, and then do an iptables-restore the new rate will not be
> enforced.
> 
> This appears to be due to a problem where hashlimit only checks for existing
> hashes by name and family and does not consider any of the other config
> parameters.
> 
> I've attempted to fix this by having it check for all hashlimit config params,
> this way it doesn't accidentally match just on name. This brought up an issue
> of having to make hashlimit aware of how many references there are to its
> proc entry.
> 
> I'm not submitting this for inclusion yet, but for feedback. Mainly on the approach
> and if there's possibly a better way of resolving this problem. My handling of
> the proc "problem" is pretty messy right now and possibly incomplete, but the
> patch below allows the case I described above to pass now. I hope to clean up
> the proc handling in a v2.
> 
> Any feedback is greatly appreciated.

[ ignoring proc issue you pointed out ]

>     from:
>     -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT
> 
>     to:
>     -A INPUT -s 10.18.40.44/32 -i eth1 -p tcp -m hashlimit --hashlimit-upto 1000/sec --hashlimit-burst 10 --hashlimit-name test -j ACCEPT
> 
>     Currently when we do do this the new parameters are not enforced.

Note that:

-A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10 --hashlimit-name test
-A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10 --hashlimit-name test

doesn't work as expected either (rule #2 uses config options of #1).

I think is behaviour is so unexpected that I would consider this a bug...

Wrt. to the patch, aside from proc issue I only see style/indent nits, f.e.

> +	hlist_for_each_entry(hinfo, &hashlimit_net->htables, node) {
> +		if (!strcmp(name, hinfo->name) &&
> +			family == hinfo->family) {

'family' should align with the 'if (', not the body.

Other than that, it looks good to me.

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

[Patrick McHardy]

On 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote:
>Josh Hunt <johunt@akamai.com> wrote: 
>>     Currently when we do do this the new parameters are not enforced.
>
>Note that:
>
>-A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10
>--hashlimit-name test
>-A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10
>--hashlimit-name test
>
>doesn't work as expected either (rule #2 uses config options of #1).
>
>I think is behaviour is so unexpected that I would consider this a
>bug...

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.

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

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote:
> >Josh Hunt <johunt@akamai.com> wrote: 
> >>     Currently when we do do this the new parameters are not enforced.
> >
> >Note that:
> >
> >-A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10
> >--hashlimit-name test
> >-A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10
> >--hashlimit-name test
> >
> >doesn't work as expected either (rule #2 uses config options of #1).
> >
> >I think is behaviour is so unexpected that I would consider this a
> >bug...
> 
> True, but it's a bug that has existed forever and I've seen scripts that actually rely on this.

Too bad.  In that case it seems like we really cannot fix this 8-(

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

[Josh Hunt]

On 07/24/2014 05:53 AM, Patrick McHardy wrote:
> On 24. Juli 2014 09:49:27 GMT+01:00, Florian Westphal <fw@strlen.de> wrote:
>> Josh Hunt <johunt@akamai.com> wrote:
>>>      Currently when we do do this the new parameters are not enforced.
>>
>> Note that:
>>
>> -A INPUT -m hashlimit --hashlimit-upto 10/sec --hashlimit-burst 10
>> --hashlimit-name test
>> -A INPUT -m hashlimit --hashlimit-upto 1/sec --hashlimit-burst 10
>> --hashlimit-name test
>>
>> doesn't work as expected either (rule #2 uses config options of #1).
>>
>> I think is behaviour is so unexpected that I would consider this a
>> bug...
>
> 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.

In the case I describe with a restore it gives the user what I would 
expect to be undesired behavior since they would have explicitly changed 
the hash's config, but it's not having any affect. In Florian's case the 
user would have a number of rules where they've explicitly made 
different hash configurations, but all of the rules with the same hash 
name would behave the same as the first. Ignoring all of the configs the 
user input.

Thanks
Josh

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

[Holger Eitzenberger]

[-- 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 = {

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

[Jan Engelhardt]

On Thursday 2014-08-14 16:09, Holger Eitzenberger wrote:
>
>For case 2) the behaviour is unexpected: when using iptables-restore
>to update an already existing hashtable <NAME> the updates are
>ignored.

Well, in a way, this is expected. If ruletable A references hashtable
G and you restore ruletable B also referencing G, you don't
necessarily want to clear out G.

The sensible fix is to have atomic replace of the entire ruleset
compassing all ruletables. Then, since all ruletables are going to
get replaced, replacing G with new parameters is also permissible.

At which point you may just question why the archaic concept of
separate ruletables was carried over to nf_tables;
compatibility for iptables to know which chain belongs to which table
is just another label on the object of a (modern) chain.

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

[Holger Eitzenberger]

> >For case 2) the behaviour is unexpected: when using iptables-restore
> >to update an already existing hashtable <NAME> the updates are
> >ignored.
> 
> Well, in a way, this is expected. If ruletable A references hashtable
> G and you restore ruletable B also referencing G, you don't
> necessarily want to clear out G.

I agree when having multiple rules accessing same hashtable.  But on
rule update it is a bug.

I am fine maintaining the patch adressing the rule update, as I am
aware of the change in behaviour for the other case.

 /Holger