Fix race condition on conntrack creation
Subsystems (2)
Pablo Neira Ayuso
maintainerPATCH 3/10 Fix race condition on conntrack creation
Current conntrack creation path can run into rare race conditions, make the creation process atomic.
As side-effect, this patch simplifies the conntrack core API.
This patch depends on [PATCH 4/10] and [PATCH 5/10]
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, make the creation process atomic.
net/ipv4/netfilter/ip_conntrack_netlink.c +5 -6
@@ -1059,13 +1059,12 @@ 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); +/* we do no want any races on hash insertion */ +write_lock_bh (&ip_conntrack_lock); + 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); +write_unlock_bh (&ip_conntrack_lock);DEBUGP ("conntrack with id %u inserted\n" , ct->id);return 0 ;
net/netfilter/nf_conntrack_netlink.c +5 -1
@@ -1079,8 +1079,12 @@ ctnetlink_create_conntrack(struct nfattr ct->mark =ntohl (*(u_int32_t *)NFA_DATA (cda[CTA_MARK -1 ]));#endif -add_timer (&ct->timeout); +/* we do no want any races on hash insertion */ +write_lock_bh (&nf_conntrack_lock); + ct->helper =nf_conntrack_helper_find (rtuple);nf_conntrack_hash_insert (ct); +add_timer (&ct->timeout); +write_unlock_bh (&nf_conntrack_lock);DEBUGP ("conntrack with id %u inserted\n" , ct->id);return 0 ;
Quoted 8 lines · Pablo Neira Ayuso
Current conntrack creation path can run into rare race conditions, make the creation process atomic.
As side-effect, this patch simplifies the conntrack core API.
This patch depends on [PATCH 4/10] and [PATCH 5/10]
Signed-off-by Pablo Neira Ayuso <[email protected]>
Commenting on this and the next two at once, since they belong together. First of all, they are ordered incorrectly, the code should compile and stay working between all patches, otherwise it makes understanding and testing individual patches harder and people doing bisections will have unpleasant surprises. This patch uses a function that is only introduced two patches later and introduces a deadlock by only removing the lock inside ip_conntrack_hash_insert one patch later. All this looks like it belongs in a single patch.
Quoted 6 lines
------------------------------------------------------------------------
[CTNETLINK] Fix race condition on conntrack creation
Current conntrack creation path can run into rare race conditions, make the creation process atomic.
In patch 5 you refer to timer activation and hash insertion. Why does helper lookup needs to be done inside the lock?
Quoted 16 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>Index:
net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================net/ipv4/netfilter/ip_conntrack_netlink.c +2
-3@@ -1059,13 +1059,12 @@ 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); +/* we do no want any races on hash insertion */ +write_lock_bh (&ip_conntrack_lock);
net/ipv4/netfilter/ip_conntrack_netlink.c ctnetlink_create_conntrack(struct nfattr · line 1070
Quoted 10 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +3
-3+ 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); +write_unlock_bh (&ip_conntrack_lock);DEBUGP ("conntrack with id %u inserted\n" , ct->id);return 0 ;
I also see another race, the caller of create_conntrack does a lookup within locks, drops the locks and we reaquire them here. The entire lookup-insertion needs to be atomic.
Quoted 3 lines
From patch 5:
---
net-2.6.orig/net/ipv4/netfilter/ip_conntrack_core.c2006-07-06
23:27:55.000000000 +0200
Quoted 1 line
+++
net-2.6/net/ipv4/netfilter/ip_conntrack_core.c2006-07-06
23:28:41.000000000 +0200
net/ipv4/netfilter/ip_conntrack_netlink.c void ip_conntrack_hash_insert(struct ip_ · line 437
Quoted 13 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +2
-2@@ -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); }
Since we already have the hash values in the caller of create_conntrack and should hold the locks across the lookup-insertion anyway I think this obsoletes this entire function.
Pablo Neira Ayuso
maintainerQuoted 12 lines · Patrick McHardy
Quoted 8 lines · Pablo Neira Ayuso
Current conntrack creation path can run into rare race conditions, make the creation process atomic.
As side-effect, this patch simplifies the conntrack core API.
This patch depends on [PATCH 4/10] and [PATCH 5/10]
Signed-off-by Pablo Neira Ayuso <[email protected]>
Quoted 8 lines
Commenting on this and the next two at once, since they belong together. First of all, they are ordered incorrectly, the code should compile and stay working between all patches, otherwise it makes understanding and testing individual patches harder and people doing bisections will have unpleasant surprises. This patch uses a function that is only introduced two patches later and introduces a deadlock by only removing the lock inside
ip_conntrack_hash_insertone patch later. All this looks like it belongs in a single patch.
Yup, this should go in a single patch, I'll merge them and resend. Sorry.
Quoted 10 lines
Quoted 6 lines
------------------------------------------------------------------------
[CTNETLINK] Fix race condition on conntrack creation
Current conntrack creation path can run into rare race conditions, make the creation process atomic.
In patch 5 you refer to timer activation and hash insertion. Why does helper lookup needs to be done inside the lock?
On helper module removal, the conntracks in the hashes are unhelped but the conntrack that we are about to create is not yet in hashes, and it will be refering to a unexistant helper. To overcome this Harald added the function helper_find_get() that uses appropiate module refcounting, so the helper can not vanish in the ctnetlink conntrack creation path. At the time that I was cooking the patch, I realised that we don't need find_get() anymore if we can move the helper lookup inside the locked section.
Quoted 23 lines
Quoted 23 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>Index:
net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================net/ipv4/netfilter/ip_conntrack_netlink.c +5
-6@@ -1059,13 +1059,12 @@ 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); +/* we do no want any races on hash insertion */ +write_lock_bh (&ip_conntrack_lock); + 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); +write_unlock_bh (&ip_conntrack_lock);
Quoted 7 lines
net/ipv4/netfilter/ip_conntrack_netlink.c ctnetlink_create_conntrack(struct nfattr · line 1070
Quoted 2 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +0
-0DEBUGP ("conntrack with id %u inserted\n" , ct->id);return 0 ;I also see another race, the caller of
create_conntrackdoes a lookup within locks, drops the locks and we reaquire them here. The entire lookup-insertion needs to be atomic.
Not really, the lookup-update must be atomic, as it is now, since the conntrack is in the hashes, but for the lookup-creation-insertion I don't see why we would need to. The conntrack that we are creating is not yet in the hashes, therefore we only need the locking in the step of timer activation and insertion.
Quoted 20 lines
From patch 5:
Quoted 1 line
---
net-2.6.orig/net/ipv4/netfilter/ip_conntrack_core.c2006-07-0623:27:55.000000000 +0200
Quoted 1 line
+++
net-2.6/net/ipv4/netfilter/ip_conntrack_core.c2006-07-0623:28:41.000000000 +0200
Quoted 9 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +2
-0@@ -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);
Quoted 9 lines
net/ipv4/netfilter/ip_conntrack_netlink.c void ip_conntrack_hash_insert(struct ip_ · line 437
Quoted 4 lines
net/ipv4/netfilter/ip_conntrack_netlink.c +0
-2-write_lock_bh (&ip_conntrack_lock);__ip_conntrack_hash_insert (ct, hash, repl_hash); -write_unlock_bh (&ip_conntrack_lock); }Since we already have the hash values in the caller of
create_conntrackand should hold the locks across the lookup-insertion anyway I think this obsoletes this entire function.
Same comment as above.
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
Quoted 6 lines · Pablo Neira Ayuso
Quoted 2 lines · Patrick McHardy
In patch 5 you refer to timer activation and hash insertion. Why does helper lookup needs to be done inside the lock?
Quoted 8 lines
On helper module removal, the conntracks in the hashes are unhelped but the conntrack that we are about to create is not yet in hashes, and it will be refering to a unexistant helper. To overcome this Harald added the function
helper_find_get()that uses appropiate module refcounting, so the helper can not vanish in the ctnetlink conntrack creation path. At the time that I was cooking the patch, I realised that we don't needfind_get()anymore if we can move the helper lookup inside the locked section.
Yes, that seems right.
Quoted 35 lines
Quoted 25 lines
Quoted 25 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>Index:
net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
---net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c2006-07-07 00:15:14.000000000 +0200+++
net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c2006-07-07
01:52:14.000000000 +0200net/ipv4/netfilter/ip_conntrack_netlink.c +5
-6@@ -1059,13 +1059,12 @@ 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); +/* we do no want any races on hash insertion */ +write_lock_bh (&ip_conntrack_lock); + 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); +write_unlock_bh (&ip_conntrack_lock);Quoted 8 lines
Review ctnetlink_create_conntrack(struct nfattr · line 1070
Quoted 2 lines
(quoted diff) +0
-0DEBUGP("conntrack with id %u inserted\n", ct->id); return 0;I also see another race, the caller of
create_conntrackdoes a lookup within locks, drops the locks and we reaquire them here. The entire lookup-insertion needs to be atomic.
Quoted 5 lines
Not really, the lookup-update must be atomic, as it is now, since the conntrack is in the hashes, but for the lookup-creation-insertion I don't see why we would need to. The conntrack that we are creating is not yet in the hashes, therefore we only need the locking in the step of timer activation and insertion.
The insertion doesn't check for existance again, but it might have already been created asynchrously between the lookup and the insertion, in which case we end up with two identical conntracks in the hash table.