netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse

PATCH In review billbonaparte 4 participants 9 messages Nov 4, 2014 – Nov 14, 2014

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

billbonaparte

author

2014-11-04 01:49 UTC #0 lore raw

(sorry to send this e-mail again, last mail is rejected by server due to non-acceptable content)

Quoted 6 lines · Florian Westphal [mailto:[email protected]] wrote:

Correct. This is broken since the central spin lock removal, since nf_conntrack_lock no longer protects both get_next_corpse and conntrack_confirm.
Please send a patch, moving dying check after removal of conntrack from the percpu list,

Since unconfirmed conntrack is stored in unconfirmed-list which is per-cpu list and protected by per-cpu spin-lock, we can remove it from uncomfirmed-list and insert it into ct-hash-table separately. that is to say, we can remove it from uncomfirmed-list without holding corresponding hash-lock, then check if it is dying.
if it is dying, we add it to the dying-list, then quit __nf_conntrack_confirm. we do this to follow the rules that the conntrack must alternatively at unconfirmed-list or dying-list when it is abort to be destroyed.

Quoted 4 lines

Quoted 2 lines
2. operation on ct->status should be atomic, because it race aginst
get_next_corpse.
Alternatively we could also get rid of the unconfirmed list handling in

get_next_corpse,

Quoted 2 lines

it looks to me as if its simply not worth the trouble to also caring about

unconfirmed lists.

yes, I think so.
if there is a race at operating ct->status, there will be in alternative case:
1) IPS_DYING bit which set in get_next_corpse override other bits (e.g. IPS_SRC_NAT_DONE_BIT), or
2) other bits (e.g. IPS_SRC_NAT_DONE_BIT) which set in nf_nat_setup_info override IPS_DYING bit.
but, any case seems to be okay.

Quoted 1 line

From c454ca5a96f5b6f815fe29cc2c91c92d719d7b95 Mon Sep 17 00:00:00 2001

From: bill bonaparte <[email protected]>
Date: Mon, 3 Nov 2014 17:13:51 +0800
Subject: [PATCH] netfilter: nf_conntrack: fix a race in
__nf_conntrack_confirm against nf_ct_get_next_corpse

After we remove central spinlock nf_conntrack_lock, we get the race against nf_ct_get_next_corpse again, to get rid of this race, we should remove the conntrack from the unconfirmed-list (which is per-cpu list) firstly, then check if it is dying.

---
 net/netfilter/nf_conntrack_core.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

net/netfilter/nf_conntrack_core.c +16 ~~-12~~

index 5016a69..5d54a18 100644
@@ -589,6 +589,22 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 
 	zone = nf_ct_zone(ct);
 	local_bh_disable();
+	
+	/* We have to check the DYING flag after unlink the conntrack
+	   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 */
+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
+	
+	if (unlikely(nf_ct_is_dying(ct))) {
+	    /* let's follow the rules that the conntrack must
+	       alternatively at the unconfirmed-list or dying-list
+	       when it is abort to be destoryed 
+	     */
+		nf_ct_add_to_dying_list(ct);
+		local_bh_enable();
+		return NF_ACCEPT;
+	}
 
 	do {
 		sequence = read_seqcount_begin(&net->ct.generation);

@@ -611,16 +627,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	 */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
 	pr_debug("Confirming conntrack %p\n", ct);
-	/* 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))) {
-		nf_conntrack_double_unlock(hash, reply_hash);
-		local_bh_enable();
-		return NF_ACCEPT;
-	}
 
 	/* See if there's one in the list already, including reverse:
 	   NAT could have grabbed it without realizing, since we're

@@ -636,8 +642,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 		    zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)))
 			goto out;
 
-	nf_ct_del_from_dying_or_unconfirmed_list(ct);
-
 	/* Timer relative to confirmation time, not original
 	   setting time, otherwise we'd get timer wrap in
 	   weird delay cases. */

Signature · 1 line

1.7.5.4

Jesper Dangaard Brouer

2014-11-06 13:00 UTC #1 lore raw

Quoted 2 lines · On Tue, 4 Nov 2014 09:48:32 +0800 "billbonaparte" <[email protected]>…

(sorry to send this e-mail again, last mail is rejected by server due to non-acceptable content)

There is several issues with your submission. I'll take care of resubmitting a patch in your name (so you will get credit in the git log).

If you care to know, issues are:
1. you are not sending to the appropriate mailing lists,
2. patch is as an attachment (should be inlined),
3. the patch have style and white-space issues.

Quoted 8 lines

Quoted 6 lines · Florian Westphal [mailto:[email protected]] wrote:
Correct. This is broken since the central spin lock removal, since nf_conntrack_lock no longer protects both get_next_corpse and conntrack_confirm.
Please send a patch, moving dying check after removal of conntrack from the percpu list,

Quoted 9 lines

Since unconfirmed conntrack is stored in unconfirmed-list which is per-cpu list and protected by per-cpu spin-lock, we can remove it from uncomfirmed-list and insert it into ct-hash-table separately. that is to say, we can remove it from uncomfirmed-list without holding corresponding hash-lock, then check if it is dying.
if it is dying, we add it to the dying-list, then quit __nf_conntrack_confirm. we do this to follow the rules that the conntrack must alternatively at unconfirmed-list or dying-list when it is abort to be destroyed.

In the resubmit. I'll take a slightly more conservative approach, by keeping the DYING check under the hash-lock, as it is currently. I guess we could do it without holding the hash-lock, but I want to keep the fix as simple as possible.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
2. operation on ct->status should be atomic, because it race aginst
get_next_corpse.

[...]

Quoted 6 lines

if there is a race at operating ct->status, there will be in alternative case:
1) IPS_DYING bit which set in get_next_corpse override other bits (e.g. IPS_SRC_NAT_DONE_BIT), or
2) other bits (e.g. IPS_SRC_NAT_DONE_BIT) which set in nf_nat_setup_info override IPS_DYING bit.

Notice the set_bit() is atomic, so we don't have these issues (of bits getting overridden).

Signature · 5 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Jesper Dangaard Brouer

2014-11-06 13:36 UTC #2 lore raw

PATCH 1/1 netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

In reply to billbonaparte

From: bill bonaparte <[email protected]>

After removal of the central spinlock nf_conntrack_lock, in commit 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock"), it is possible to race against get_next_corpse().

The race is against the get_next_corpse() cleanup on the "unconfirmed" list (a per-cpu list with seperate locking), which set the DYING bit.

Fix this race, in __nf_conntrack_confirm(), by removing the CT from unconfirmed list before checking the DYING bit. In case race occured, re-add the CT to the dying list.

Fixes 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")

Reported-by bill bonaparte <[email protected]>

Signed-off-by bill bonaparte <[email protected]>

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---

 net/netfilter/nf_conntrack_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

net/netfilter/nf_conntrack_core.c +4 -3

index 5016a69..1072650 100644
@@ -611,12 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	 */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
 	pr_debug("Confirming conntrack %p\n", ct);
-	/* We have to check the DYING flag inside the lock to prevent
+
+	/* We have to check the DYING flag after unlink 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 */
+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
 
 	if (unlikely(nf_ct_is_dying(ct))) {
+		nf_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;

@@ -636,8 +639,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 		    zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)))
 			goto out;
 
-	nf_ct_del_from_dying_or_unconfirmed_list(ct);
-
 	/* Timer relative to confirmation time, not original
 	   setting time, otherwise we'd get timer wrap in
 	   weird delay cases. */

Pablo Neira Ayuso

maintainer

2014-11-10 16:52 UTC #3 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

Quoted 7 lines · Jesper Dangaard Brouer

From: bill bonaparte <[email protected]>
After removal of the central spinlock nf_conntrack_lock, in commit 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock"), it is possible to race against get_next_corpse().

Quoted 7 lines

The race is against the get_next_corpse() cleanup on the "unconfirmed" list (a per-cpu list with seperate locking), which set the DYING bit.
Fix this race, in __nf_conntrack_confirm(), by removing the CT from unconfirmed list before checking the DYING bit. In case race occured, re-add the CT to the dying list.

This seems correct to me, some side comments.

Quoted 14 lines

Fixes 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
Reported-by bill bonaparte <[email protected]>
Signed-off-by bill bonaparte <[email protected]>
Signed-off-by Jesper Dangaard Brouer <[email protected]>
---
 net/netfilter/nf_conntrack_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
net/netfilter/nf_conntrack_core.c +0 -0
index 5016a69..1072650 100644

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

Quoted 9 lines

net/netfilter/nf_conntrack_core.c +2 -1

@@ -611,12 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	 */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
 	pr_debug("Confirming conntrack %p\n", ct);
-	/* We have to check the DYING flag inside the lock to prevent
+
+	/* We have to check the DYING flag after unlink 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 */

While at this, I think it would be good to fix comment style to:

        /* We have ...
         * ...
         */

I can fix this here, no need to resend, just let me know.

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

Quoted 7 lines

net/netfilter/nf_conntrack_core.c +2 -0

+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
 
 	if (unlikely(nf_ct_is_dying(ct))) {
+		nf_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;

Not directly related to your patch, but I don't find a good reason why we're accepting this packet.

If the conntrack from the unconfirmed list is dying, then the object will be released by when the packet leaves the stack to its destination. With stateful filtering depending in place, the follow up packet in the reply direction will likely be considered invalid (if tcp tracking is on). Fortunately for us, the origin will likely retransmit the syn again, so the ct will be setup accordingly.

So, why should we allow this to go through?

This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against nf_ct_get_next_corpse()") btw.

Jesper Dangaard Brouer

2014-11-12 07:35 UTC #4 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

Quoted 44 lines · On Mon, 10 Nov 2014 17:54:39 +0100 Pablo Neira Ayuso <[email protected]>…

Quoted 7 lines · Jesper Dangaard Brouer
From: bill bonaparte <[email protected]>
After removal of the central spinlock nf_conntrack_lock, in commit 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock"), it is possible to race against get_next_corpse().
Quoted 7 lines
The race is against the get_next_corpse() cleanup on the "unconfirmed" list (a per-cpu list with seperate locking), which set the DYING bit.
Fix this race, in __nf_conntrack_confirm(), by removing the CT from unconfirmed list before checking the DYING bit. In case race occured, re-add the CT to the dying list.
This seems correct to me, some side comments.
Quoted 14 lines
Fixes 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
Reported-by bill bonaparte <[email protected]>
Signed-off-by bill bonaparte <[email protected]>
Signed-off-by Jesper Dangaard Brouer <[email protected]>
---
 net/netfilter/nf_conntrack_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
net/netfilter/nf_conntrack_core.c +0 -0
index 5016a69..1072650 100644
net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb) · line 618
Quoted 9 lines
net/netfilter/nf_conntrack_core.c +2 -1
@@ -611,12 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb)
 	 */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
 	pr_debug("Confirming conntrack %p\n", ct);
-	/* We have to check the DYING flag inside the lock to prevent
+
+	/* We have to check the DYING flag after unlink 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 */
While at this, I think it would be good to fix comment style to:

Quoted 5 lines

        /* We have ...
         * ...
         */
I can fix this here, no need to resend, just let me know.

Okay, I was just trying to keep the changes as minimal as possible, if this should go into a stable-kernel. Your choice.

Quoted 11 lines

net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb) · line 625
Quoted 7 lines
net/netfilter/nf_conntrack_core.c +2 -0
+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
 
 	if (unlikely(nf_ct_is_dying(ct))) {
+		nf_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;
Not directly related to your patch, but I don't find a good reason why we're accepting this packet.

Quoted 8 lines

If the conntrack from the unconfirmed list is dying, then the object will be released by when the packet leaves the stack to its destination. With stateful filtering depending in place, the follow up packet in the reply direction will likely be considered invalid (if tcp tracking is on). Fortunately for us, the origin will likely retransmit the syn again, so the ct will be setup accordingly.
So, why should we allow this to go through?

True, it also seems strange to me that we accept this packet.

Quoted 3 lines

This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against nf_ct_get_next_corpse()") btw.

And the commit does not argue why NF_ACCEPT was chosen...

Signature · 5 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Jörg Marx

2014-11-12 10:57 UTC #5 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

On 12.11.2014 08:35, Jesper Dangaard Brouer wrote:

Hi,

I wrote the patch in 2010, so find some arguments below:

Quoted 18 lines

Quoted 18 lines
Quoted 18 lines
Quoted 7 lines
net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb) · line 625
Quoted 7 lines
net/netfilter/nf_conntrack_core.c +2 -0
+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
 
 	if (unlikely(nf_ct_is_dying(ct))) {
+		nf_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;
Not directly related to your patch, but I don't find a good reason why we're accepting this packet.
If the conntrack from the unconfirmed list is dying, then the object will be released by when the packet leaves the stack to its destination. With stateful filtering depending in place, the follow up packet in the reply direction will likely be considered invalid (if tcp tracking is on). Fortunately for us, the origin will likely retransmit the syn again, so the ct will be setup accordingly.

Quoted 2 lines

Quoted 1 line
Quoted 1 line
So, why should we allow this to go through?
True, it also seems strange to me that we accept this packet.

The raise was triggered in a scenario when we tested high-load IPsec tunnels and flushed the conntrack hashs from userspace.

For me there is little difference in choosing DROP or ACCEPT as verdict. The packet/skb belongs to a formerly allowed connection, most likely this connection is still allowed (but the conntrack hash entry is about to be removed due to userspace is flushing the conntrack table).

To minimize the impact (lost packets -> retransmit) I decided to allow the skb in flight, so were is no lost packet at this place.

When the connection is not allowed anymore (but was allowed up to now, because the hash entry exists), the impact is one last packet 'slipping through'.

Today I would still decide the way I did in 2010.

Quoted 9 lines

Quoted 4 lines
Quoted 4 lines
This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against nf_ct_get_next_corpse()") btw.
And the commit does not argue why NF_ACCEPT was chosen...
-- Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn:
http://www.linkedin.com/in/brouer

best regards
Jörg Marx

Signature · 4 lines

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2014-11-13 12:06 UTC #6 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

Quoted 27 lines · Jörg Marx

On 12.11.2014 08:35, Jesper Dangaard Brouer wrote:
Hi,
I wrote the patch in 2010, so find some arguments below:
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 7 lines
net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb) · line 625
Quoted 7 lines
net/netfilter/nf_conntrack_core.c +2 -0
+	nf_ct_del_from_dying_or_unconfirmed_list(ct);
 
 	if (unlikely(nf_ct_is_dying(ct))) {
+		nf_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;
Not directly related to your patch, but I don't find a good reason why we're accepting this packet.
If the conntrack from the unconfirmed list is dying, then the object will be released by when the packet leaves the stack to its destination. With stateful filtering depending in place, the follow up packet in the reply direction will likely be considered invalid (if tcp tracking is on). Fortunately for us, the origin will likely retransmit the syn again, so the ct will be setup accordingly.
Quoted 2 lines
Quoted 1 line
Quoted 1 line
So, why should we allow this to go through?
True, it also seems strange to me that we accept this packet.

Quoted 7 lines

The raise was triggered in a scenario when we tested high-load IPsec tunnels and flushed the conntrack hashs from userspace.
For me there is little difference in choosing DROP or ACCEPT as verdict. The packet/skb belongs to a formerly allowed connection, most likely this connection is still allowed (but the conntrack hash entry is about to be removed due to userspace is flushing the conntrack table).

__nf_conntrack_confirm() is only called for the first packet that we see in a flow. If you just invoked the flush command once (which should be the common case), then this is likely to be the first packet of the flow (unless you already called flush anytime soon in the past).

Quoted 2 lines

To minimize the impact (lost packets -> retransmit) I decided to allow the skb in flight, so were is no lost packet at this place.

I understand your original motivation was to be conservative.

Quoted 3 lines

When the connection is not allowed anymore (but was allowed up to now, because the hash entry exists), the impact is one last packet 'slipping through'.

The general policy in conntrack is to not drop packets, but in this case we'll leave things in inconsistent state (ie. we will likely receive a reply packet in response to the original packet that has no conntrack yet).

Thanks.

Jörg Marx

2014-11-13 14:33 UTC #7 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

Quoted 9 lines · Pablo Neira Ayuso

Quoted 4 lines
For me there is little difference in choosing DROP or ACCEPT as verdict.
Quoted 3 lines
The packet/skb belongs to a formerly allowed connection, most likely this connection is still allowed (but the conntrack hash entry is about to be removed due to userspace is flushing the conntrack table).
__nf_conntrack_confirm() is only called for the first packet that we see in a flow. If you just invoked the flush command once (which should be the common case), then this is likely to be the first packet of the flow (unless you already called flush anytime soon in the past).

Yes, you are right. As far as I remember it was very hard to trigger that critical moment, when the first packet triggered the insertion into the hash table. But the test and production systems showed this strange behaviour, that no traffic was allowed to flow for exactly 600 seconds.

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
To minimize the impact (lost packets -> retransmit) I decided to allow the skb in flight, so were is no lost packet at this place.
I understand your original motivation was to be conservative.

Yes.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
When the connection is not allowed anymore (but was allowed up to now, because the hash entry exists), the impact is one last packet 'slipping through'.

Feel free to change the verdict, IMHO it doesn't matter at all as long as the hash table is in a consistent state. The higher protocol layers will deal with the missing packet.

Quoted 4 lines

The general policy in conntrack is to not drop packets, but in this case we'll leave things in inconsistent state (ie. we will likely receive a reply packet in response to the original packet that has no conntrack yet).

Under heavy load this can happen anyway I guess?

Thanks and best regards
Jörg

Signature · 24 lines

Dipl.-Inform.
Jörg Marx
Bereichsleiter
Entwicklung Client- & Netzwerksicherheit
Geschäftsbereich Public Sector
secunet Security Networks AG
Ammonstr. 74
D-01067 Dresden, Germany 
Telefon +49 201 54 54-3517
Telefax +49 201 54 54-1323
[email protected]
www.secunet.com

secunet Security Networks AG
Kronprinzenstr. 30
45128 Essen, Germany 
Amtsgericht Essen HRB 13615

Vorstand: Dr. Rainer Baumgart (Vors.), Thomas Pleines
Aufsichtsratsvorsitzender: Dr. Peter Zattler
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

maintainer

2014-11-14 16:39 UTC #8 lore raw

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

In reply to Jesper Dangaard Brouer

Quoted 9 lines · Jesper Dangaard Brouer

Quoted 9 lines
net/netfilter/nf_conntrack_core.c __nf_conntrack_confirm(struct sk_buff *skb) · line 618
Quoted 6 lines
net/netfilter/nf_conntrack_core.c +2 -1
-	/* We have to check the DYING flag inside the lock to prevent
+
+	/* We have to check the DYING flag after unlink 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 */
While at this, I think it would be good to fix comment style to:

Quoted 8 lines

Quoted 5 lines
        /* We have ...
         * ...
         */
I can fix this here, no need to resend, just let me know.
Okay, I was just trying to keep the changes as minimal as possible, if this should go into a stable-kernel. Your choice.

I'm going to take this patch including the comment style fix, I would like to avoid specific patches to fix coding style issues, and the first line of this comment is updated. I think the patch will be still small to fulfill -stable rules.

I'll send a follow a patch to change the return verdict to NF_DROP to not mix up different things.

Thanks!