[PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

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 <pablo@netfilter.org>

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

[-- Attachment #2: 03racy.patch --]
[-- Type: text/plain, Size: 1861 bytes --]

[CTNETLINK] Fix race condition on conntrack creation

Current conntrack creation path can run into rare race conditions, make
the creation process atomic.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 00:15:14.000000000 +0200
+++ net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 01:52:14.000000000 +0200
@@ -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;
Index: net-2.6/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-2.6.orig/net/netfilter/nf_conntrack_netlink.c	2006-07-07 00:15:14.000000000 +0200
+++ net-2.6/net/netfilter/nf_conntrack_netlink.c	2006-07-07 01:52:32.000000000 +0200
@@ -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;

Re: [PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> 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 <pablo@netfilter.org>

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.

> ------------------------------------------------------------------------
> 
> [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?

> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
> ===================================================================
> --- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 00:15:14.000000000 +0200
> +++ net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 01:52:14.000000000 +0200
> @@ -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;

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.

>From patch 5:

> --- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_core.c 2006-07-06
23:27:55.000000000 +0200
> +++ net-2.6/net/ipv4/netfilter/ip_conntrack_core.c	2006-07-06
23:28:41.000000000 +0200
> @@ -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.

Re: [PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>>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 <pablo@netfilter.org>
> 
> 
> 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.

Yup, this should go in a single patch, I'll merge them and resend. Sorry.

>>------------------------------------------------------------------------
>>
>>[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.

>>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>
>>Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
>>===================================================================
>>--- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 00:15:14.000000000 +0200
>>+++ net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-07-07 01:52:14.000000000 +0200
>>@@ -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;
> 
> 
> 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.

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.

> From patch 5:
> 
> 
>>--- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_core.c 2006-07-06
> 
> 23:27:55.000000000 +0200
> 
>>+++ net-2.6/net/ipv4/netfilter/ip_conntrack_core.c	2006-07-06
> 
> 23:28:41.000000000 +0200
> 
>>@@ -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.

Same comment as above.

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

Re: [PATCH 3/10][CTNETLINK] Fix race condition on conntrack creation

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>> 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.

Yes, that seems right.

>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>>
>>> Index: net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c
>>> ===================================================================
>>> --- net-2.6.orig/net/ipv4/netfilter/ip_conntrack_netlink.c   
>>> 2006-07-07 00:15:14.000000000 +0200
>>> +++ net-2.6/net/ipv4/netfilter/ip_conntrack_netlink.c    2006-07-07
>>> 01:52:14.000000000 +0200
>>> @@ -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;
>>
>>
>>
>> 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.
> 
> 
> 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.