[PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Joerg Marx]

Because nobody commented this so far on the 'linux-netdev' list, now a
re-post on the 'netfilter' list with a kind request for confirmation
and/or discussion...

###################################################################


Hi,

we encountered a weird problem of a 'stalled' connection when using
'conntrack -F' on a box with heavy network load. 'conntrack -L' gave us
sometimes a [UNREPLIED] entry for the traffic in question, but no traffic
flow, no matching of packets in or out, only the timer went down from 600
to 0 (it was ESP traffic - the default generic timeout = 600 seconds).
After the entry vanished by timeout (or doing a 'conntrack -F' once more),
all worked normal again.

The reason we finally found, is a race window in 'nf_conntrack_confirm'
when calling '__nf_conntrack_confirm':

In 'nf_conntrack_confirm' is checked (without holding a lock), if the entry
to be confirmed is possibly dying: !nf_ct_is_dying(ct).
If not, then __nf_conntrack_confirm will do some sanity checking, grab a
spin_lock_bh and insert the 'ct' into the lookup cache.

Now consider the following scenario:

1. a connection has seen the first packet already -> state is UNREPLIED
2. now the answer is to be sent, conntrack wants to confirm the connection
3. the !nf_ct_is_dying(ct) check is passed, __nf_conntrack_confirm is just
started
4. in a user context a 'conntrack -F' command is running right now e.g. on
another CPU
5. this will flag all unconfirmed connections as 'dying' in
get_next_corpse(...), including the entry going to be confirmed!
6. now the already 'dying' entry is included into the hash cache in
__nf_conntrack_confirm - BOOM!

After this step the connection in question is dead, because no packets are
forwarded until the entry is purged from hash cache. This was a big blocker
for us, because each dead IPsec tunnel is a dead branch network for 10
minutes...

For every packet from now on 'nf_conntrack_find_get' will ignore the entry,
because it is dying and because __nf_conntrack_confirm finds the hash in
the cache already, it will NF_DROP the packet.

The key for finding this was 'NF_CT_STAT_INC(net, insert_failed)' in
__nf_conntrack_confirm.

The suggested solution is to check for '!nf_ct_is_dying(ct)' again, _after_
the spin_lock_bh is grabbed in __nf_conntrack_confirm. So it is clear, that
no other softirq or user context can set that 'evil' dying flag ;-)
The return value in this case should be NF_ACCEPT, so we loose no packets
then, this is also important for us.


---
 net/netfilter/nf_conntrack_core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c
b/net/netfilter/nf_conntrack_core.c
index 1374179..e2c8bfe 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -413,6 +413,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)

 	spin_lock_bh(&nf_conntrack_lock);

+	if (unlikely(nf_ct_is_dying(ct))) {
+		spin_unlock_bh(&nf_conntrack_lock);
+		return NF_ACCEPT;
+	}
+
 	/* See if there's one in the list already, including reverse:
 	   NAT could have grabbed it without realizing, since we're
 	   not in the hash.  If there is, we lost race. */
-- 1.5.6.5

Best regards
Joerg.


-- 

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Pablo Neira Ayuso]

Hi Joerg,

I'm cc'ing netfilter-devel instead, that is used for development stuff.

Joerg Marx wrote:
> Because nobody commented this so far on the 'linux-netdev' list, now a
> re-post on the 'netfilter' list with a kind request for confirmation
> and/or discussion...
> 
> ###################################################################
> 
> 
> Hi,
> 
> we encountered a weird problem of a 'stalled' connection when using
> 'conntrack -F' on a box with heavy network load. 'conntrack -L' gave us
> sometimes a [UNREPLIED] entry for the traffic in question, but no traffic
> flow, no matching of packets in or out, only the timer went down from 600
> to 0 (it was ESP traffic - the default generic timeout = 600 seconds).
> After the entry vanished by timeout (or doing a 'conntrack -F' once more),
> all worked normal again.
> 
> The reason we finally found, is a race window in 'nf_conntrack_confirm'
> when calling '__nf_conntrack_confirm':
> 
> In 'nf_conntrack_confirm' is checked (without holding a lock), if the entry
> to be confirmed is possibly dying: !nf_ct_is_dying(ct).
> If not, then __nf_conntrack_confirm will do some sanity checking, grab a
> spin_lock_bh and insert the 'ct' into the lookup cache.
> 
> Now consider the following scenario:
> 
> 1. a connection has seen the first packet already -> state is UNREPLIED
> 2. now the answer is to be sent, conntrack wants to confirm the connection
> 3. the !nf_ct_is_dying(ct) check is passed, __nf_conntrack_confirm is just
> started
> 4. in a user context a 'conntrack -F' command is running right now e.g. on
> another CPU
> 5. this will flag all unconfirmed connections as 'dying' in
> get_next_corpse(...), including the entry going to be confirmed!
> 6. now the already 'dying' entry is included into the hash cache in
> __nf_conntrack_confirm - BOOM!
> 
> After this step the connection in question is dead, because no packets are
> forwarded until the entry is purged from hash cache. This was a big blocker
> for us, because each dead IPsec tunnel is a dead branch network for 10
> minutes...
> 
> For every packet from now on 'nf_conntrack_find_get' will ignore the entry,
> because it is dying and because __nf_conntrack_confirm finds the hash in
> the cache already, it will NF_DROP the packet.
> 
> The key for finding this was 'NF_CT_STAT_INC(net, insert_failed)' in
> __nf_conntrack_confirm.
> 
> The suggested solution is to check for '!nf_ct_is_dying(ct)' again, _after_
> the spin_lock_bh is grabbed in __nf_conntrack_confirm. So it is clear, that
> no other softirq or user context can set that 'evil' dying flag ;-)
> The return value in this case should be NF_ACCEPT, so we loose no packets
> then, this is also important for us.
> 
> 
> ---
>  net/netfilter/nf_conntrack_core.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c
> b/net/netfilter/nf_conntrack_core.c
> index 1374179..e2c8bfe 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -413,6 +413,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)
> 
>  	spin_lock_bh(&nf_conntrack_lock);
> 
> +	if (unlikely(nf_ct_is_dying(ct))) {
> +		spin_unlock_bh(&nf_conntrack_lock);
> +		return NF_ACCEPT;
> +	}
> +
>  	/* See if there's one in the list already, including reverse:
>  	   NAT could have grabbed it without realizing, since we're
>  	   not in the hash.  If there is, we lost race. */
> -- 1.5.6.5

I think this patch is fine. Patrick, would you apply it and pass it to
-stable, please? Thanks!

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

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Patrick McHardy]

Pablo Neira Ayuso wrote:
>> we encountered a weird problem of a 'stalled' connection when using
>> 'conntrack -F' on a box with heavy network load. 'conntrack -L' gave us
>> sometimes a [UNREPLIED] entry for the traffic in question, but no traffic
>> flow, no matching of packets in or out, only the timer went down from 600
>> to 0 (it was ESP traffic - the default generic timeout = 600 seconds).
>> After the entry vanished by timeout (or doing a 'conntrack -F' once more),
>> all worked normal again.
>>
>> The reason we finally found, is a race window in 'nf_conntrack_confirm'
>> when calling '__nf_conntrack_confirm':
>>
>> In 'nf_conntrack_confirm' is checked (without holding a lock), if the entry
>> to be confirmed is possibly dying: !nf_ct_is_dying(ct).
>> If not, then __nf_conntrack_confirm will do some sanity checking, grab a
>> spin_lock_bh and insert the 'ct' into the lookup cache.
>>
>> Now consider the following scenario:
>>
>> 1. a connection has seen the first packet already -> state is UNREPLIED
>> 2. now the answer is to be sent, conntrack wants to confirm the connection
>> 3. the !nf_ct_is_dying(ct) check is passed, __nf_conntrack_confirm is just
>> started
>> 4. in a user context a 'conntrack -F' command is running right now e.g. on
>> another CPU
>> 5. this will flag all unconfirmed connections as 'dying' in
>> get_next_corpse(...), including the entry going to be confirmed!
>> 6. now the already 'dying' entry is included into the hash cache in
>> __nf_conntrack_confirm - BOOM!

Just for the record: the conntrack will be confirmed once the first
packet passed through all the hooks, so the incorrect hash insertion
will happen on the first packet, but the race condition you describe
is correct.

>> After this step the connection in question is dead, because no packets are
>> forwarded until the entry is purged from hash cache. This was a big blocker
>> for us, because each dead IPsec tunnel is a dead branch network for 10
>> minutes...
>>
>> For every packet from now on 'nf_conntrack_find_get' will ignore the entry,
>> because it is dying and because __nf_conntrack_confirm finds the hash in
>> the cache already, it will NF_DROP the packet.
>>
>> The key for finding this was 'NF_CT_STAT_INC(net, insert_failed)' in
>> __nf_conntrack_confirm.
>>
>> The suggested solution is to check for '!nf_ct_is_dying(ct)' again, _after_
>> the spin_lock_bh is grabbed in __nf_conntrack_confirm. So it is clear, that
>> no other softirq or user context can set that 'evil' dying flag ;-)
>> The return value in this case should be NF_ACCEPT, so we loose no packets
>> then, this is also important for us.

I think this should be fine since the race you describe only affects
unconfirmed conntracks, but it took me a while to realize that all
the other spots where the DYING bit is set are fine without holding
the conntrack lock.

Could you please add a comment to the check in __nf_conntrack_confirm()
stating that the dying check is supposed to prevent races against
nf_ct_get_next_corpse()? The semantic of the DYING bit is unfortunately
a bit overloaded.

Also, since the condition unconfirmed + dying in nf_conntrack_confirm()
is highly unlikely, I'd suggest to remove the dying check there and only
perform it in __nf_conntrack_confirm().

>> diff --git a/net/netfilter/nf_conntrack_core.c
>> b/net/netfilter/nf_conntrack_core.c
>> index 1374179..e2c8bfe 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -413,6 +413,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>>
>>  	spin_lock_bh(&nf_conntrack_lock);
>>
>> +	if (unlikely(nf_ct_is_dying(ct))) {
>> +		spin_unlock_bh(&nf_conntrack_lock);
>> +		return NF_ACCEPT;
>> +	}
>> +
>>  	/* See if there's one in the list already, including reverse:
>>  	   NAT could have grabbed it without realizing, since we're
>>  	   not in the hash.  If there is, we lost race. */
>> -- 1.5.6.5
> 
> I think this patch is fine. Patrick, would you apply it and pass it to
> -stable, please? Thanks!
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Joerg Marx]

On 05/10/2010 05:23 PM, Patrick McHardy wrote:

...
> 
> I think this should be fine since the race you describe only affects
> unconfirmed conntracks, but it took me a while to realize that all
> the other spots where the DYING bit is set are fine without holding
> the conntrack lock.
> 
> Could you please add a comment to the check in __nf_conntrack_confirm()
> stating that the dying check is supposed to prevent races against
> nf_ct_get_next_corpse()? The semantic of the DYING bit is unfortunately
> a bit overloaded.
> 
> Also, since the condition unconfirmed + dying in nf_conntrack_confirm()
> is highly unlikely, I'd suggest to remove the dying check there and only
> perform it in __nf_conntrack_confirm().
> 

I hope the comment is clearly pointing to the (solved) problem now. I
also removed the obsolete check in nf_conntrack_confirm.

diff --git a/include/net/netfilter/nf_conntrack_core.h
b/include/net/netfilter/nf_conntrack_core.h
index dffde8e..3d7524f 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -61,7 +61,7 @@ static inline int nf_conntrack_confirm(struct sk_buff
*skb)
 	int ret = NF_ACCEPT;

 	if (ct && ct != &nf_conntrack_untracked) {
-		if (!nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct))
+		if (!nf_ct_is_confirmed(ct))
 			ret = __nf_conntrack_confirm(skb);
 		if (likely(ret == NF_ACCEPT))
 			nf_ct_deliver_cached_events(ct);
diff --git a/net/netfilter/nf_conntrack_core.c
b/net/netfilter/nf_conntrack_core.c
index 0c9bbe9..7ff9a40 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -422,6 +422,16 @@ __nf_conntrack_confirm(struct sk_buff *skb)

 	spin_lock_bh(&nf_conntrack_lock);

+	/* We have to check the DYING flag inside the lock to prevent
+	   a race against nf_ct_get_next_corpse() possibly called from
+	   user context, else we insert an already 'dead' hash, blocking
+	   further use of that particular connection -JM */
+
+	if (unlikely(nf_ct_is_dying(ct))) {
+		spin_unlock_bh(&nf_conntrack_lock);
+		return NF_ACCEPT;
+	}
+
 	/* See if there's one in the list already, including reverse:
 	   NAT could have grabbed it without realizing, since we're
 	   not in the hash.  If there is, we lost race. */



-- 

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Patrick McHardy]

Joerg Marx wrote:
> On 05/10/2010 05:23 PM, Patrick McHardy wrote:
> 
> ...
>> I think this should be fine since the race you describe only affects
>> unconfirmed conntracks, but it took me a while to realize that all
>> the other spots where the DYING bit is set are fine without holding
>> the conntrack lock.
>>
>> Could you please add a comment to the check in __nf_conntrack_confirm()
>> stating that the dying check is supposed to prevent races against
>> nf_ct_get_next_corpse()? The semantic of the DYING bit is unfortunately
>> a bit overloaded.
>>
>> Also, since the condition unconfirmed + dying in nf_conntrack_confirm()
>> is highly unlikely, I'd suggest to remove the dying check there and only
>> perform it in __nf_conntrack_confirm().
>>
> 
> I hope the comment is clearly pointing to the (solved) problem now. I
> also removed the obsolete check in nf_conntrack_confirm.

Thanks, this looks fine. But I need a formal submission, including
a changelog and Signed-off-by: line. Thanks!

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Joerg Marx]

On 05/14/2010 01:05 PM, Patrick McHardy wrote:

...
>>
>> I hope the comment is clearly pointing to the (solved) problem now. I
>> also removed the obsolete check in nf_conntrack_confirm.
> 
> Thanks, this looks fine. But I need a formal submission, including
> a changelog and Signed-off-by: line. Thanks!

From 97913752bc65b2aa4a091a73c3569dc8221a1f25 Mon Sep 17 00:00:00 2001
From: Joerg Marx <joerg.marx@secunet.com>
Date: Mon, 10 May 2010 18:39:47 +0200
Subject: [PATCH] Fix a race in __nf_conntrack_confirm against nf_ct_get_next_corpse()

This race was triggered by a 'conntrack -F' command running in parallel
to the insertion of a hash for a new connection.
Losing this race led to a dead conntrack entry effectively blocking
traffic for a particular connection until timeout or flushing the conntrack
hashes again.
Now the check for an already dying connection is done inside the lock.

Signed-off-by: Joerg Marx <joerg.marx@secunet.com>
---
 include/net/netfilter/nf_conntrack_core.h |    2 +-
 net/netfilter/nf_conntrack_core.c         |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index dffde8e..3d7524f 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -61,7 +61,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
 	int ret = NF_ACCEPT;
 
 	if (ct && ct != &nf_conntrack_untracked) {
-		if (!nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct))
+		if (!nf_ct_is_confirmed(ct))
 			ret = __nf_conntrack_confirm(skb);
 		if (likely(ret == NF_ACCEPT))
 			nf_ct_deliver_cached_events(ct);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 0c9bbe9..7ff9a40 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -422,6 +422,16 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 
 	spin_lock_bh(&nf_conntrack_lock);
 
+	/* We have to check the DYING flag inside the lock to prevent
+	   a race against nf_ct_get_next_corpse() possibly called from
+	   user context, else we insert an already 'dead' hash, blocking
+	   further use of that particular connection -JM */
+
+	if (unlikely(nf_ct_is_dying(ct))) {
+		spin_unlock_bh(&nf_conntrack_lock);
+		return NF_ACCEPT;
+	}
+
 	/* See if there's one in the list already, including reverse:
 	   NAT could have grabbed it without realizing, since we're
 	   not in the hash.  If there is, we lost race. */
-- 
1.5.6.5



-- 

Re: [PATCH] nf_conntrack_core.c: fix for dead connection after flushing conntrack cache

[Patrick McHardy]

Appologies for the delay, I once again had a mail outage :|

Joerg Marx wrote:
> Subject: [PATCH] Fix a race in __nf_conntrack_confirm against nf_ct_get_next_corpse()
> 
> This race was triggered by a 'conntrack -F' command running in parallel
> to the insertion of a hash for a new connection.
> Losing this race led to a dead conntrack entry effectively blocking
> traffic for a particular connection until timeout or flushing the conntrack
> hashes again.
> Now the check for an already dying connection is done inside the lock.

Applied, thanks.