Re: Resend: One bug in the xt_hashlimit.c

[Pablo Neira Ayuso <pablo@netfilter.org>]

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);