xt_hashlimit: restore per-rule effectiveness

xt_hashlimit: restore per-rule effectiveness

[Jan Engelhardt]

The following changes since commit eecc545856c8a0f27783a440d25f4ceaa1f95ce8:

  netfilter: add missing xt_log.h file (2010-10-04 23:24:21 +0200)

are available in the git repository at:
  git://dev.medozas.de/linux hashlimit

Jan Engelhardt (1):
      netfilter: xt_hashlimit: restore per-rule effectiveness

 net/netfilter/xt_hashlimit.c |   50 ++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 23 deletions(-)

[PATCH] netfilter: xt_hashlimit: restore per-rule effectiveness

[Jan Engelhardt]

When adding a second hashlimit rule with the same name, its parameters
had no effect, because it had used a copy of the first one's inner
state.

References: http://bugzilla.netfilter.org/show_bug.cgi?id=650
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/netfilter/xt_hashlimit.c |   50 ++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 9228ee0..c3ae0e3 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -97,7 +97,9 @@ struct xt_hashlimit_htable {
 	u_int8_t family;
 	bool rnd_initialized;
 
-	struct hashlimit_cfg1 cfg;	/* config */
+	struct {
+		u_int32_t size, max, gc_interval, expire;
+	} cfg;
 
 	/* used internally */
 	spinlock_t lock;		/* lock for list_head */
@@ -233,8 +235,10 @@ static int htable_create(struct net *net, struct xt_hashlimit_mtinfo1 *minfo,
 	minfo->hinfo = hinfo;
 
 	/* copy match config into hashtable config */
-	memcpy(&hinfo->cfg, &minfo->cfg, sizeof(hinfo->cfg));
-	hinfo->cfg.size = size;
+	hinfo->cfg.size        = size;
+	hinfo->cfg.max         = minfo->cfg.max;
+	hinfo->cfg.gc_interval = minfo->cfg.gc_interval;
+	hinfo->cfg.expire      = minfo->cfg.expire;
 	if (hinfo->cfg.max == 0)
 		hinfo->cfg.max = 8 * hinfo->cfg.size;
 	else if (hinfo->cfg.max < hinfo->cfg.size)
@@ -442,7 +446,7 @@ static void hashlimit_ipv6_mask(__be32 *i, unsigned int p)
 #endif
 
 static int
-hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
+hashlimit_init_dst(const struct xt_hashlimit_mtinfo1 *info, u_int8_t family,
 		   struct dsthash_dst *dst,
 		   const struct sk_buff *skb, unsigned int protoff)
 {
@@ -452,34 +456,34 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
 
 	memset(dst, 0, sizeof(*dst));
 
-	switch (hinfo->family) {
+	switch (family) {
 	case NFPROTO_IPV4:
-		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_DIP)
+		if (info->cfg.mode & XT_HASHLIMIT_HASH_DIP)
 			dst->ip.dst = maskl(ip_hdr(skb)->daddr,
-			              hinfo->cfg.dstmask);
-		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_SIP)
+			              info->cfg.dstmask);
+		if (info->cfg.mode & XT_HASHLIMIT_HASH_SIP)
 			dst->ip.src = maskl(ip_hdr(skb)->saddr,
-			              hinfo->cfg.srcmask);
+			              info->cfg.srcmask);
 
-		if (!(hinfo->cfg.mode &
+		if (!(info->cfg.mode &
 		      (XT_HASHLIMIT_HASH_DPT | XT_HASHLIMIT_HASH_SPT)))
 			return 0;
 		nexthdr = ip_hdr(skb)->protocol;
 		break;
 #if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
 	case NFPROTO_IPV6:
-		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_DIP) {
+		if (info->cfg.mode & XT_HASHLIMIT_HASH_DIP) {
 			memcpy(&dst->ip6.dst, &ipv6_hdr(skb)->daddr,
 			       sizeof(dst->ip6.dst));
-			hashlimit_ipv6_mask(dst->ip6.dst, hinfo->cfg.dstmask);
+			hashlimit_ipv6_mask(dst->ip6.dst, info->cfg.dstmask);
 		}
-		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_SIP) {
+		if (info->cfg.mode & XT_HASHLIMIT_HASH_SIP) {
 			memcpy(&dst->ip6.src, &ipv6_hdr(skb)->saddr,
 			       sizeof(dst->ip6.src));
-			hashlimit_ipv6_mask(dst->ip6.src, hinfo->cfg.srcmask);
+			hashlimit_ipv6_mask(dst->ip6.src, info->cfg.srcmask);
 		}
 
-		if (!(hinfo->cfg.mode &
+		if (!(info->cfg.mode &
 		      (XT_HASHLIMIT_HASH_DPT | XT_HASHLIMIT_HASH_SPT)))
 			return 0;
 		nexthdr = ipv6_hdr(skb)->nexthdr;
@@ -503,9 +507,9 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
 	}
 	if (!ports)
 		return -1;
-	if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_SPT)
+	if (info->cfg.mode & XT_HASHLIMIT_HASH_SPT)
 		dst->src_port = ports[0];
-	if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_DPT)
+	if (info->cfg.mode & XT_HASHLIMIT_HASH_DPT)
 		dst->dst_port = ports[1];
 	return 0;
 }
@@ -519,7 +523,7 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	struct dsthash_ent *dh;
 	struct dsthash_dst dst;
 
-	if (hashlimit_init_dst(hinfo, &dst, skb, par->thoff) < 0)
+	if (hashlimit_init_dst(info, par->family, &dst, skb, par->thoff) < 0)
 		goto hotdrop;
 
 	rcu_read_lock_bh();
@@ -532,11 +536,11 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		}
 		dh->expires = jiffies + msecs_to_jiffies(hinfo->cfg.expire);
 		dh->rateinfo.prev = jiffies;
-		dh->rateinfo.credit = user2credits(hinfo->cfg.avg *
-		                      hinfo->cfg.burst);
-		dh->rateinfo.credit_cap = user2credits(hinfo->cfg.avg *
-		                          hinfo->cfg.burst);
-		dh->rateinfo.cost = user2credits(hinfo->cfg.avg);
+		dh->rateinfo.credit = user2credits(info->cfg.avg *
+		                      info->cfg.burst);
+		dh->rateinfo.credit_cap = user2credits(info->cfg.avg *
+		                          info->cfg.burst);
+		dh->rateinfo.cost = user2credits(info->cfg.avg);
 	} else {
 		/* update expiration timeout */
 		dh->expires = now + msecs_to_jiffies(hinfo->cfg.expire);
-- 
1.7.1

Re: [PATCH] netfilter: xt_hashlimit: restore per-rule effectiveness

[Patrick McHardy]

Am 06.10.2010 08:28, schrieb Jan Engelhardt:
> When adding a second hashlimit rule with the same name, its parameters
> had no effect, because it had used a copy of the first one's inner
> state.

I'm not sure we can change this behaviour at this point. There's at
least one change in your patch that changes the default behaviour,
you can currently create a second rule for a table witout specifying
the mode and it will re-use the mode from the first rule. With your
patch this will result in an error due to the unfortunate fact that
the kernel doesn't check for valid modes. Having entries for rules
with different modes also would require to initalize all members
since a different rule might want to match on them. This again
doesn't work with the way masking is currently implemented.

> @@ -452,34 +456,34 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
>  
>  	memset(dst, 0, sizeof(*dst));
>  
> -	switch (hinfo->family) {
> +	switch (family) {

This also looks problematic, the entries don't include the family
themselves, so you're allowing tables to contain entries of multiple
families, which might cause mismatches.

>  	case NFPROTO_IPV4:
> -		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_DIP)
> +		if (info->cfg.mode & XT_HASHLIMIT_HASH_DIP)
>  			dst->ip.dst = maskl(ip_hdr(skb)->daddr,
> -			              hinfo->cfg.dstmask);
> -		if (hinfo->cfg.mode & XT_HASHLIMIT_HASH_SIP)
> +			              info->cfg.dstmask);
> +		if (info->cfg.mode & XT_HASHLIMIT_HASH_SIP)
>  			dst->ip.src = maskl(ip_hdr(skb)->saddr,
> -			              hinfo->cfg.srcmask);
> +			              info->cfg.srcmask);
>  
> -		if (!(hinfo->cfg.mode &
> +		if (!(info->cfg.mode &
>  		      (XT_HASHLIMIT_HASH_DPT | XT_HASHLIMIT_HASH_SPT)))
>  			return 0;
>  		nexthdr = ip_hdr(skb)->protocol;
>  		break;

Re: [PATCH] netfilter: xt_hashlimit: restore per-rule effectiveness

[Jan Engelhardt]

On Wednesday 2010-10-06 15:54, Patrick McHardy wrote:

>Am 06.10.2010 08:28, schrieb Jan Engelhardt:
>> When adding a second hashlimit rule with the same name, its parameters
>> had no effect, because it had used a copy of the first one's inner
>> state.
>
>I'm not sure we can change this behaviour at this point. There's at
>least one change in your patch that changes the default behaviour,
>you can currently create a second rule for a table witout specifying
>the mode

I don't think that works. iptables does not know how many hashlimit 
rules there are, thus it always enforces the presence of 
--hashlimit-name, --hashlimit-mode and so on.

>> @@ -452,34 +456,34 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
>>  
>>  	memset(dst, 0, sizeof(*dst));
>>  
>> -	switch (hinfo->family) {
>> +	switch (family) {
>
>This also looks problematic, the entries don't include the family
>themselves, so you're allowing tables to contain entries of multiple
>families, which might cause mismatches.

AFAICS, one can already mix v4 and v6 into the same hashlimit bucket
at this time (including side effects).

Re: [PATCH] netfilter: xt_hashlimit: restore per-rule effectiveness

[Patrick McHardy]

On 06.10.2010 17:00, Jan Engelhardt wrote:
> On Wednesday 2010-10-06 15:54, Patrick McHardy wrote:
> 
>> Am 06.10.2010 08:28, schrieb Jan Engelhardt:
>>> When adding a second hashlimit rule with the same name, its parameters
>>> had no effect, because it had used a copy of the first one's inner
>>> state.
>>
>> I'm not sure we can change this behaviour at this point. There's at
>> least one change in your patch that changes the default behaviour,
>> you can currently create a second rule for a table witout specifying
>> the mode
> 
> I don't think that works. iptables does not know how many hashlimit 
> rules there are, thus it always enforces the presence of 
> --hashlimit-name, --hashlimit-mode and so on.

No, revision 1 only checks for limit and name.

>>> @@ -452,34 +456,34 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
>>>  
>>>  	memset(dst, 0, sizeof(*dst));
>>>  
>>> -	switch (hinfo->family) {
>>> +	switch (family) {
>>
>> This also looks problematic, the entries don't include the family
>> themselves, so you're allowing tables to contain entries of multiple
>> families, which might cause mismatches.
> 
> AFAICS, one can already mix v4 and v6 into the same hashlimit bucket
> at this time (including side effects).

No, currently the tables include the family as key. Actually your
patch doesn't allow that either, but it doesn't make sense to change
hashlimit_init_dst to use par->family instead of hinfo->family.