Re: Resend: One bug in the xt_hashlimit.c

Re: Resend: One bug in the xt_hashlimit.c

[Jan Engelhardt]

On Sunday 2012-12-16 16:19, Feng Gao wrote:
>
>Hi Harald & Jan,
>
>Because there is no any response for this issue,so I send the egmail again a
>s a reminder.

Sorry to not have responded. This sort of went under the radar since
netfilter-devel was initially not Cced. More answers to the patch
below.

>The following codes are from function "hashlimit_mt".
>dh = dsthash_find(hinfo, &dst);
>if (dh == NULL) {
>dh = dsthash_alloc_init(hinfo, &dst);
>
>When two or more threads invoke dsthash_find(hinfo, &dst) at the same time
>and fail to find the dh, then all of them will enter the dsthash_alloc_init
>to create one new node.
>As a result, it will casue that these multiple threads create multle nodes
>with same IP. It is not expected behavior.

>--- net/netfilter/xt_hashlimit.c.orig	2012-12-02 13:54:45.376382165 +0800
>+++ net/netfilter/xt_hashlimit.c	2012-12-02 13:59:32.083381142 +0800
>@@ -157,11 +157,20 @@ dsthash_find(const struct xt_hashlimit_h
> /* allocate dsthash_ent, initialize dst, put in htable and lock it */
> static struct dsthash_ent *
> dsthash_alloc_init(struct xt_hashlimit_htable *ht,
>-		   const struct dsthash_dst *dst)
>+		   const struct dsthash_dst *dst,
>+		   bool *new_node)
> {
> 	struct dsthash_ent *ent;
> 
> 	spin_lock(&ht->lock);
>+
>+	ent = dsthash_find(ht, dst);

Hm. In hashlimit_mt(), dsthash_find() has already been called once
(within a RCU_bh section), and now you are doing the lookup again
(within spin_lock section).

>+	if (ent) {        

You have trailing squatspaces (remove them). I suggest that you
use `mcedit` or `git log -1 -p`, as both these tools highlight
such unwanted trailing spaces.

>@@ -539,7 +555,7 @@ hashlimit_mt(const struct sk_buff *skb, 
> 		dh->expires = now + msecs_to_jiffies(hinfo->cfg.expire);
> 		rateinfo_recalc(dh, now);
> 	}
>-
>+	
> 	if (dh->rateinfo.credit >= dh->rateinfo.cost) {
> 		/* below the limit */
> 		dh->rateinfo.credit -= dh->rateinfo.cost;

Same spacing issue here.

====

What would you think of the following patch?

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 26a668a..0d17032 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -591,9 +591,11 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		goto hotdrop;
 
 	rcu_read_lock_bh();
+	spin_lock_bh();
 	dh = dsthash_find(hinfo, &dst);
 	if (dh == NULL) {
 		dh = dsthash_alloc_init(hinfo, &dst);
+		spin_unlock_bh();
 		if (dh == NULL) {
 			rcu_read_unlock_bh();
 			goto hotdrop;
@@ -601,6 +603,7 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		dh->expires = jiffies + msecs_to_jiffies(hinfo->cfg.expire);
 		rateinfo_init(dh, hinfo);
 	} else {
+		spin_unlock_bh();
 		/* update expiration timeout */
 		dh->expires = now + msecs_to_jiffies(hinfo->cfg.expire);
 		rateinfo_recalc(dh, now, hinfo->cfg.mode);

Re: Resend: One bug in the xt_hashlimit.c

[Pablo Neira Ayuso]

On Sun, Dec 16, 2012 at 06:39:56PM +0100, Jan Engelhardt wrote:
[...]
> What would you think of the following patch?
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 26a668a..0d17032 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -591,9 +591,11 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		goto hotdrop;
>  
>  	rcu_read_lock_bh();
> +	spin_lock_bh();
>  	dh = dsthash_find(hinfo, &dst);
>  	if (dh == NULL) {
>  		dh = dsthash_alloc_init(hinfo, &dst);
> +		spin_unlock_bh();

This is sub-optimal and RCU becomes almost useless with this approach.

The way to fix this is to follow a similar approach to what
nf_conntrack does to avoid this race.

>  		if (dh == NULL) {
>  			rcu_read_unlock_bh();
>  			goto hotdrop;
> @@ -601,6 +603,7 @@ hashlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		dh->expires = jiffies + msecs_to_jiffies(hinfo->cfg.expire);
>  		rateinfo_init(dh, hinfo);
>  	} else {
> +		spin_unlock_bh();
>  		/* update expiration timeout */
>  		dh->expires = now + msecs_to_jiffies(hinfo->cfg.expire);
>  		rateinfo_recalc(dh, now, hinfo->cfg.mode);