Skip to contentSkip to conversation

Fix race condition in early drop #2

PATCH In review Pablo Neira Ayuso 2 participants 2 messages Aug 24, 2006 – Aug 25, 2006

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 3/3 Fix race condition in early drop #2

Hi,

This patch follows from:
http://patchwork.netfilter.org/netfilter-devel/patch.pl?id=3722

I have rework the patch based on Yasuyuki's and Patrick's comments.

Signature · 1 line
The dawn of the fourth age of Linux firewalling is coming; a time of great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

CONNTRACK] Fix race condition in early drop

On SMP environments the maximum number of conntracks can be overpassed under heavy stress situations due to an existing race condition.

CPU A CPU B
atomic_read() ...
early_drop() ...
... atomic_read()
allocate conntrack allocate conntrack
atomic_inc() atomic_inc()

This patch moves the counter incrementation before the early drop stage.

Signed-off-by Pablo Neira Ayuso <[email protected]>
net/ipv4/netfilter/ip_conntrack_core.c +5 -2
Index: net-2.6/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
@@ -641,11 +641,15 @@ struct ip_conntrack *ip_conntrack_alloc(
 		ip_conntrack_hash_rnd_initted = 1;
 	}
 
+	/* We don't want any race condition at early drop stage */
+	atomic_inc(&ip_conntrack_count);
+
 	if (ip_conntrack_max
 	    && atomic_read(&ip_conntrack_count) >= ip_conntrack_max) {
 		unsigned int hash = hash_conntrack(orig);
 		/* Try dropping from this hash chain. */
 		if (!early_drop(&ip_conntrack_hash[hash])) {
+			atomic_dec(&ip_conntrack_count);
 			if (net_ratelimit())
 				printk(KERN_WARNING
 				       "ip_conntrack: table full, dropping"

@@ -657,6 +661,7 @@ struct ip_conntrack *ip_conntrack_alloc(
 	conntrack = kmem_cache_alloc(ip_conntrack_cachep, GFP_ATOMIC);
 	if (!conntrack) {
 		DEBUGP("Can't allocate conntrack.\n");
+		atomic_dec(&ip_conntrack_count);
 		return ERR_PTR(-ENOMEM);
 	}
 

@@ -670,8 +675,6 @@ struct ip_conntrack *ip_conntrack_alloc(
 	conntrack->timeout.data = (unsigned long)conntrack;
 	conntrack->timeout.function = death_by_timeout;
 
-	atomic_inc(&ip_conntrack_count);
-
 	return conntrack;
 }
net/netfilter/nf_conntrack_core.c +7 -1
Index: net-2.6/net/netfilter/nf_conntrack_core.c
===================================================================
@@ -867,11 +867,15 @@ __nf_conntrack_alloc(const struct nf_con
 		nf_conntrack_hash_rnd_initted = 1;
 	}
 
+	/* We don't want any race condition at early drop stage */
+	atomic_inc(&nf_conntrack_count);
+
 	if (nf_conntrack_max
 	    && atomic_read(&nf_conntrack_count) >= nf_conntrack_max) {
 		unsigned int hash = hash_conntrack(orig);
 		/* Try dropping from this hash chain. */
 		if (!early_drop(&nf_conntrack_hash[hash])) {
+			atomic_dec(&nf_conntrack_count);
 			if (net_ratelimit())
 				printk(KERN_WARNING
 				       "nf_conntrack: table full, dropping"

@@ -922,10 +926,12 @@ __nf_conntrack_alloc(const struct nf_con
 	init_timer(&conntrack->timeout);
 	conntrack->timeout.data = (unsigned long)conntrack;
 	conntrack->timeout.function = death_by_timeout;
+	read_unlock_bh(&nf_ct_cache_lock);
 
-	atomic_inc(&nf_conntrack_count);
+	return conntrack;
 out:
 	read_unlock_bh(&nf_ct_cache_lock);
+	atomic_dec(&nf_conntrack_count);
 	return conntrack;
 }
#1 lore raw

net/ipv4/netfilter/ip_conntrack_core.c struct ip_conntrack *ip_conntrack_alloc( · line 648

Quoted 13 lines · Pablo Neira Ayuso

Index: net-2.6/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================

net/ipv4/netfilter/ip_conntrack_core.c +3 -0
@@ -641,11 +641,15 @@ struct ip_conntrack *ip_conntrack_alloc(
 		ip_conntrack_hash_rnd_initted = 1;
 	}
 
+	/* We don't want any race condition at early drop stage */
+	atomic_inc(&ip_conntrack_count);
+
 	if (ip_conntrack_max
 	    && atomic_read(&ip_conntrack_count) >= ip_conntrack_max) {

This should become ">", no? No need to send a new patch, I can change it before applying.