Fix race condition on conntrack creation

PATCH Applied Pablo Neira Ayuso 4 participants 5 messages Jul 25, 2006 – Aug 8, 2006

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2006-07-25 13:18 UTC #0 lore raw

PATCH 4/8 Fix race condition on conntrack creation

Current conntrack creation path can run into rare race conditions, the insertion into hashes and timer activation must be done atomically.

This patch also:
- remove functions helper_[find_get|put] that have no clients anymore
- rework get_features facility to avoid a softlockup

Signed-off-by Pablo Neira Ayuso <[email protected]>

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

[CTNETLINK] Fix race condition on conntrack creation

Current conntrack creation path can run into rare race conditions, the insertion into hashes and timer activation must be done atomically.

This patch also:
- remove functions helper_[find_get|put] that have no clients anymore
- rework get_features facility to avoid a softlockup

Signed-off-by Pablo Neira Ayuso <[email protected]>

net/ipv4/netfilter/ip_conntrack_netlink.c +3 -7

Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
@@ -1059,13 +1059,9 @@ ctnetlink_create_conntrack(struct nfattr
 		ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));
 #endif
 
-	ct->helper = ip_conntrack_helper_find_get(rtuple);
-
-	add_timer(&ct->timeout);
+	ct->helper = ip_conntrack_helper_find(rtuple);
 	ip_conntrack_hash_insert(ct);
-
-	if (ct->helper)
-		ip_conntrack_helper_put(ct->helper);
+	add_timer(&ct->timeout);
 
 	DEBUGP("conntrack with id %u inserted\n", ct->id);
 	return 0;

@@ -1107,11 +1103,11 @@ ctnetlink_new_conntrack(struct sock *ctn
 		h = __ip_conntrack_find(&rtuple, NULL);
 
 	if (h == NULL) {
-		write_unlock_bh(&ip_conntrack_lock);
 		DEBUGP("no such conntrack, create new\n");
 		err = -ENOENT;
 		if (nlh->nlmsg_flags & NLM_F_CREATE)
 			err = ctnetlink_create_conntrack(cda, &otuple, &rtuple);
+		write_unlock_bh(&ip_conntrack_lock);
 		return err;
 	}
 	/* implicit 'else' */

net/netfilter/nf_conntrack_netlink.c +6 -2

Index: net-2.6/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -1049,6 +1049,7 @@ ctnetlink_create_conntrack(struct nfattr
 			   struct nf_conntrack_tuple *rtuple)
 {
 	struct nf_conn *ct;
+	struct nf_conn_help *help;
 	int err = -EINVAL;
 
 	DEBUGP("entered %s\n", __FUNCTION__);

@@ -1079,8 +1080,11 @@ ctnetlink_create_conntrack(struct nfattr
 		ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));
 #endif
 
-	add_timer(&ct->timeout);
+	help = nfct_help(ct);
+	if (help)
+		help->helper = nf_ct_helper_find(rtuple);
 	nf_conntrack_hash_insert(ct);
+	add_timer(&ct->timeout);
 
 	DEBUGP("conntrack with id %u inserted\n", ct->id);
 	return 0;

@@ -1124,11 +1128,11 @@ ctnetlink_new_conntrack(struct sock *ctn
 		h = __nf_conntrack_find(&rtuple, NULL);
 
 	if (h == NULL) {
-		write_unlock_bh(&nf_conntrack_lock);
 		DEBUGP("no such conntrack, create new\n");
 		err = -ENOENT;
 		if (nlh->nlmsg_flags & NLM_F_CREATE)
 			err = ctnetlink_create_conntrack(cda, &otuple, &rtuple);
+		write_unlock_bh(&nf_conntrack_lock);
 		return err;
 	}
 	/* implicit 'else' */

include/linux/netfilter_ipv4/ip_conntrack.h +1 -2

Index: net-2.6/include/linux/netfilter_ipv4/ip_conntrack.h
===================================================================
@@ -255,8 +255,7 @@ ip_ct_iterate_cleanup(int (*iter)(struct
 extern struct ip_conntrack_helper *
 __ip_conntrack_helper_find_byname(const char *);
 extern struct ip_conntrack_helper *
-ip_conntrack_helper_find_get(const struct ip_conntrack_tuple *tuple);
-extern void ip_conntrack_helper_put(struct ip_conntrack_helper *helper);
+ip_conntrack_helper_find(const struct ip_conntrack_tuple *tuple);
 
 extern struct ip_conntrack_protocol *
 __ip_conntrack_proto_find(u_int8_t protocol);

include/net/netfilter/nf_conntrack.h +1 -2

Index: net-2.6/include/net/netfilter/nf_conntrack.h
===================================================================
@@ -221,8 +221,7 @@ extern void nf_ct_remove_expectations(st
 extern void nf_conntrack_flush(void);
 
 extern struct nf_conntrack_helper *
-nf_ct_helper_find_get( const struct nf_conntrack_tuple *tuple);
-extern void nf_ct_helper_put(struct nf_conntrack_helper *helper);
+nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
 
 extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);

net/ipv4/netfilter/ip_conntrack_core.c +6 ~~-34~~

Index: net-2.6/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
@@ -428,12 +428,12 @@ void ip_conntrack_hash_insert(struct ip_
 {
 	unsigned int hash, repl_hash;
 
+	ASSERT_WRITE_LOCK(&ip_conntrack_lock);
+	
 	hash = hash_conntrack(&ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
 	repl_hash = hash_conntrack(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
 
-	write_lock_bh(&ip_conntrack_lock);
 	__ip_conntrack_hash_insert(ct, hash, repl_hash);
-	write_unlock_bh(&ip_conntrack_lock);
 }
 
 /* Confirm a connection given skb; places it in hash table */

@@ -566,42 +566,14 @@ static inline int helper_cmp(const struc
 	return ip_ct_tuple_mask_cmp(rtuple, &i->tuple, &i->mask);
 }
 
-static struct ip_conntrack_helper *
-__ip_conntrack_helper_find( const struct ip_conntrack_tuple *tuple)
+struct ip_conntrack_helper *
+ip_conntrack_helper_find(const struct ip_conntrack_tuple *tuple)
 {
 	return LIST_FIND(&helpers, helper_cmp,
 			 struct ip_conntrack_helper *,
 			 tuple);
 }
 
-struct ip_conntrack_helper *
-ip_conntrack_helper_find_get( const struct ip_conntrack_tuple *tuple)
-{
-	struct ip_conntrack_helper *helper;
-
-	/* need ip_conntrack_lock to assure that helper exists until
-	 * try_module_get() is called */
-	read_lock_bh(&ip_conntrack_lock);
-
-	helper = __ip_conntrack_helper_find(tuple);
-	if (helper) {
-		/* need to increase module usage count to assure helper will
-		 * not go away while the caller is e.g. busy putting a
-		 * conntrack in the hash that uses the helper */
-		if (!try_module_get(helper->me))
-			helper = NULL;
-	}
-
-	read_unlock_bh(&ip_conntrack_lock);
-
-	return helper;
-}
-
-void ip_conntrack_helper_put(struct ip_conntrack_helper *helper)
-{
-	module_put(helper->me);
-}
-
 struct ip_conntrack_protocol *
 __ip_conntrack_proto_find(u_int8_t protocol)
 {

@@ -730,7 +702,7 @@ init_conntrack(struct ip_conntrack_tuple
 		nf_conntrack_get(&conntrack->master->ct_general);
 		CONNTRACK_STAT_INC(expect_new);
 	} else {
-		conntrack->helper = __ip_conntrack_helper_find(&repl_tuple);
+		conntrack->helper = ip_conntrack_helper_find(&repl_tuple);
 
 		CONNTRACK_STAT_INC(new);
 	}

@@ -1055,7 +1027,7 @@ void ip_conntrack_alter_reply(struct ip_
 
 	conntrack->tuplehash[IP_CT_DIR_REPLY].tuple = *newreply;
 	if (!conntrack->master && conntrack->expecting == 0)
-		conntrack->helper = __ip_conntrack_helper_find(newreply);
+		conntrack->helper = ip_conntrack_helper_find(newreply);
 	write_unlock_bh(&ip_conntrack_lock);
 }

net/ipv4/netfilter/ip_conntrack_standalone.c +1 -2

Index: net-2.6/net/ipv4/netfilter/ip_conntrack_standalone.c
===================================================================
@@ -954,8 +954,7 @@ EXPORT_SYMBOL_GPL(ip_conntrack_hash_inse
 
 EXPORT_SYMBOL_GPL(ip_ct_remove_expectations);
 
-EXPORT_SYMBOL_GPL(ip_conntrack_helper_find_get);
-EXPORT_SYMBOL_GPL(ip_conntrack_helper_put);
+EXPORT_SYMBOL_GPL(ip_conntrack_helper_find);
 EXPORT_SYMBOL_GPL(__ip_conntrack_helper_find_byname);
 
 EXPORT_SYMBOL_GPL(ip_conntrack_proto_find_get);

net/netfilter/nf_conntrack_core.c +39 ~~-45~~

160 lines not on this page · raw message

net/netfilter/nf_conntrack_standalone.c +1 -2

Index: net-2.6/net/netfilter/nf_conntrack_standalone.c
===================================================================
@@ -889,8 +889,7 @@ EXPORT_SYMBOL(nf_conntrack_alloc);
 EXPORT_SYMBOL(nf_conntrack_free);
 EXPORT_SYMBOL(nf_conntrack_flush);
 EXPORT_SYMBOL(nf_ct_remove_expectations);
-EXPORT_SYMBOL(nf_ct_helper_find_get);
-EXPORT_SYMBOL(nf_ct_helper_put);
+EXPORT_SYMBOL(nf_ct_helper_find);
 EXPORT_SYMBOL(__nf_conntrack_helper_find_byname);
 EXPORT_SYMBOL(__nf_conntrack_find);
 EXPORT_SYMBOL(nf_ct_unlink_expect);

Yasuyuki KOZAKAI

2006-07-28 13:16 UTC #1 lore raw

Hi, Pablo,

From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 25 Jul 2006 15:18:38 +0200

Quoted 1 line

- rework get_features facility to avoid a softlockup

This looks nice cleanup, but __nf_conntrack_alloc() cannot be called while holding nf_conntrack_lock, because it may call early_drop(), which holds nf_contrack_lock and also may call nf_ct_put().

net/netfilter/nf_conntrack_core.c static inline int helper_cmp(const struc · line 863

Quoted 7 lines

static struct nf_conn * __nf_conntrack_alloc(const struct nf_conntrack_tuple *orig,
net/netfilter/nf_conntrack_core.c +2 -1
 		     const struct nf_conntrack_tuple *repl,
-		     const struct nf_conntrack_l3proto *l3proto)
+		     const struct nf_conntrack_l3proto *l3proto,
+		     u_int32_t features)
 {

You've moved "features = l3proto->get_features(orig);" out of this function, then the argument 'l3proto' isn't necessary.

BTW, I think this race is similar situation in init_conntrack(). It doesn't care about race and __nf_conntrack_confirm() does it instead.

One more my concern is recent Patrick's proposal.

https://lists.netfilter.org/pipermail/netfilter-devel/2006-July/025107.html

Quoted 3 lines

- change conntrack to always put connections in the hash immediately and remove them again if the connection is dropped before beeing confirmed.

If we do this, we need to solve the same issue in init_conntrack(). It might be time to consider to organize processing of hash and lock.

-- Yasuyuki Kozkai

Pablo Neira Ayuso

author maintainer

2006-07-31 11:15 UTC #2 lore raw

Hi Yasuyuki,

Quoted 13 lines · Yasuyuki KOZAKAI

From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 25 Jul 2006 15:18:38 +0200
Quoted 1 line
- rework get_features facility to avoid a softlockup
This looks nice cleanup, but __nf_conntrack_alloc() cannot be called while holding nf_conntrack_lock, because it may call early_drop(), which holds nf_contrack_lock and also may call nf_ct_put().

Quoted 11 lines

Quoted 7 lines
static struct nf_conn *
__nf_conntrack_alloc(const struct nf_conntrack_tuple *orig,
const struct nf_conntrack_tuple *repl,
net/netfilter/nf_conntrack_core.c +2 -1
-		     const struct nf_conntrack_l3proto *l3proto)
+		     const struct nf_conntrack_l3proto *l3proto,
+		     u_int32_t features)
{
You've moved "features = l3proto->get_features(orig);" out of this function, then the argument 'l3proto' isn't necessary.

Indeed, I also detected another problem related with the NAT code in ip_conntrack_netlink, so this patch needs to be dropped.

I'm questioning the usefulness of this patch since nfnetlink serializes the creation of two new conntracks.

Quoted 9 lines

BTW, I think this race is similar situation in init_conntrack(). It doesn't care about race and __nf_conntrack_confirm() does it instead.
One more my concern is recent Patrick's proposal.
https://lists.netfilter.org/pipermail/netfilter-devel/2006-July/025107.html

Quoted 6 lines

Quoted 3 lines
- change conntrack to always put connections in the hash immediately and remove them again if the connection is dropped before beeing confirmed.
If we do this, we need to solve the same issue in init_conntrack(). It might be time to consider to organize processing of hash and lock.

About the implementation, we can play around with refcounting, set refcount to 1 if the conntrack is unconfirmed and increase it to 2 once it gets confirmed.

Signature · 2 lines

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

Amin Azez

2006-08-04 14:43 UTC #3 lore raw

* Pablo Neira Ayuso wrote, On 31/07/06 12:15:

Quoted 25 lines

Hi Yasuyuki,
Quoted 22 lines · Yasuyuki KOZAKAI
From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 25 Jul 2006 15:18:38 +0200
Quoted 1 line
- rework get_features facility to avoid a softlockup
This looks nice cleanup, but __nf_conntrack_alloc() cannot be called while holding nf_conntrack_lock, because it may call early_drop(), which holds nf_contrack_lock and also may call nf_ct_put().
Quoted 7 lines
static struct nf_conn *
__nf_conntrack_alloc(const struct nf_conntrack_tuple *orig,
const struct nf_conntrack_tuple *repl,
(quoted diff) +2 -1
-             const struct nf_conntrack_l3proto *l3proto)
+             const struct nf_conntrack_l3proto *l3proto,
+             u_int32_t features)
{

Quoted 8 lines

Quoted 2 lines
You've moved "features = l3proto->get_features(orig);" out of this function, then the argument 'l3proto' isn't necessary.
Indeed, I also detected another problem related with the NAT code in ip_conntrack_netlink, so this patch needs to be dropped.
I'm questioning the usefulness of this patch since nfnetlink serializes the creation of two new conntracks.

I'm finding it hard to drop this patch from the series, having trouble applying patch 7 from the series without this patch.

I find it difficult to be comfortable with dropping some features of this patch. In this fragment, the second chunk does what looks like an important re-ordering of locking and conntrack creation; i.e. of course the lock is retained till after conntrack creation.

So I don't think it is safe to entirely drop this patch; Pablo?

net/ipv4/netfilter/ip_conntrack_netlink.c +2 -7

Index: linux-2.6.17.1/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
@@ -1212,13 +1212,9 @@ ctnetlink_create_conntrack(struct nfattr
                ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));
 #endif
 
-       ct->helper = ip_conntrack_helper_find_get(rtuple);
-
-       add_timer(&ct->timeout);
+       ct->helper = ip_conntrack_helper_find(rtuple);
        ip_conntrack_hash_insert(ct);
-
-       if (ct->helper)
-               ip_conntrack_helper_put(ct->helper);
+       add_timer(&ct->timeout);
 
        DEBUGP("conntrack with id %u inserted\n", ct->id);
        return 0;

@@ -1260,11 +1256,11 @@ ctnetlink_new_conntrack(struct sock *ctn
                h = __ip_conntrack_find(&rtuple, NULL);
 
        if (h == NULL) {
-               write_unlock_bh(&ip_conntrack_lock);
                DEBUGP("no such conntrack, create new\n");
                err = -ENOENT;
                if (nlh->nlmsg_flags & NLM_F_CREATE)
                        err = ctnetlink_create_conntrack(cda, &otuple,

&rtuple);

+               write_unlock_bh(&ip_conntrack_lock);
                return err;
        }
        /* implicit 'else' */

Patrick McHardy

2006-08-08 10:19 UTC #4 lore raw

Quoted 8 lines · Amin Azez

* Pablo Neira Ayuso wrote, On 31/07/06 12:15:
I find it difficult to be comfortable with dropping some features of this patch. In this fragment, the second chunk does what looks like an important re-ordering of locking and conntrack creation; i.e. of course the lock is retained till after conntrack creation.
So I don't think it is safe to entirely drop this patch; Pablo?

I'll drop and and see which of the other ones still make sense.