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

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

net/netfilter/nf_conntrack_core.c +0 -0

index 1374179..e2c8bfe 100644
@@ -413,6 +413,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 
 	spin_lock_bh(&nf_conntrack_lock);

Quoted 18 lines

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:

Quoted 10 lines

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!

Quoted 9 lines

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.

Quoted 8 lines

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.

Quoted 8 lines

net/netfilter/nf_conntrack_core.c +0 -0

index 1374179..e2c8bfe 100644
@@ -413,6 +413,11 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 
 	spin_lock_bh(&nf_conntrack_lock);

net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb)

Quoted 10 lines

net/netfilter/nf_conntrack_core.c +5 -1

 
+	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 <[email protected]>

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().

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

...

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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!

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.