Skip to contentSkip to conversation

Oops in nf_nat_core.c:find_appropriate_src(), kernel 2.6.25.4

DISCUSSION Chuck Ebbert 4 participants 11 messages Jun 7, 2008 – Jun 17, 2008

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

Chuck Ebbert

author
#0 lore raw

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

#1 lore raw
Quoted 8 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {
Quoted 6 lines

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

Anyways, does this patch help? When reallocating storage for a conntrack, it is replaced in the list before assigning the nat->ct pointer.

net/ipv4/netfilter/nf_nat_core.c +1 -1
index 0457859..945da60 100644
@@ -570,8 +570,8 @@ static void nf_nat_move_storage(void *new, void *old)
 		return;
 
 	spin_lock_bh(&nf_nat_lock);
-	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	new_nat->ct = ct;
+	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	spin_unlock_bh(&nf_nat_lock);
 }
#2 lore raw
Quoted 16 lines · Patrick McHardy
Quoted 8 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {
Quoted 6 lines

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

Quoted 7 lines

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

Anyways, does this patch help? When reallocating storage for a conntrack, it is replaced in the list before assigning the nat->ct pointer.

I'm afraid we also need this one on top - when reallocating an extension, we must not free the old storage since it may still be used in a RCU read side.

include/net/netfilter/nf_conntrack_extend.h +1 -0
index f736e84..f80c0ed 100644
@@ -15,6 +15,7 @@ enum nf_ct_ext_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
+	struct rcu_head rcu;
 	u8 offset[NF_CT_EXT_NUM];
 	u8 len;
 	char data[0];
net/netfilter/nf_conntrack_extend.c +8 -1
index bcc19fa..90d4a74 100644
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
 	return (void *)(*ext) + off;
 }
 
+static void __nf_ct_ext_destroy_rcu(struct rcu_head *head)
+{
+	struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
+	kfree(ext);
+}
+
 void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {
 	struct nf_ct_ext *new;

@@ -106,7 +113,7 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
-		kfree(ct->ext);
+		call_rcu(&ct->ext->rcu, __nf_ct_ext_destroy_rcu);
 		ct->ext = new;
 	}
#3 lore raw
Quoted 21 lines · Patrick McHardy
Quoted 20 lines · Patrick McHardy
Quoted 8 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {
Quoted 6 lines

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

Quoted 8 lines
Quoted 3 lines

Anyways, does this patch help? When reallocating storage for a conntrack, it is replaced in the list before assigning the nat->ct pointer.

I'm afraid we also need this one on top - when reallocating an extension, we must not free the old storage since it may still be used in a RCU read side.

It does not help here: 2.6.26-rc5 plus two above patches still crashes.

Best regards,

Krzysztof Olędzki

#4 lore raw
Quoted 15 lines · Krzysztof Oledzki
Quoted 13 lines · Patrick McHardy
Quoted 12 lines · Patrick McHardy
Quoted 11 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

We found the reason for that crash and I've queued these two patches. Please let me know whether they also fix the problem from the redhat bugzilla.

netfilter: nf_conntrack: fix ctnetlink related crash in nf_nat_setup_info()

When creation of a new conntrack entry in ctnetlink fails after having set up the NAT mappings, the conntrack has an extension area allocated that is not getting properly destroyed when freeing the conntrack again. This means the NAT extension is still in the bysource hash, causing a crash when walking over the hash chain the next time:

BUG: unable to handle kernel paging request at 00120fbd
IP: [<c03d394b>] nf_nat_setup_info+0x221/0x58a
*pde = 00000000
Oops: 0000 [#1] PREEMPT SMP

Pid: 2795, comm: conntrackd Not tainted (2.6.26-rc5 #1)
EIP: 0060:[<c03d394b>] EFLAGS: 00010206 CPU: 1
EIP is at nf_nat_setup_info+0x221/0x58a
EAX: 00120fbd EBX: 00120fbd ECX: 00000001 EDX: 00000000
ESI: 0000019e EDI: e853bbb4 EBP: e853bbc8 ESP: e853bb78
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process conntrackd (pid: 2795, ti=e853a000 task=f7de10f0 task.ti=e853a000)

Stack: 00000000 e853bc2c e85672ec 00000008 c0561084 63c1db4a 00000000 00000000
       00000000 0002e109 61d2b1c3 00000000 00000000 00000000 01114e22 61d2b1c3
       00000000 00000000 f7444674 e853bc04 00000008 c038e728 0000000a f7444674
Call Trace:
 [<c038e728>] nla_parse+0x5c/0xb0
 [<c0397c1b>] ctnetlink_change_status+0x190/0x1c6
 [<c0397eec>] ctnetlink_new_conntrack+0x189/0x61f
 [<c0119aee>] update_curr+0x3d/0x52
 [<c03902d1>] nfnetlink_rcv_msg+0xc1/0xd8
 [<c0390228>] nfnetlink_rcv_msg+0x18/0xd8
 [<c0390210>] nfnetlink_rcv_msg+0x0/0xd8
 [<c038d2ce>] netlink_rcv_skb+0x2d/0x71
 [<c0390205>] nfnetlink_rcv+0x19/0x24
 [<c038d0f5>] netlink_unicast+0x1b3/0x216
 ...

Move invocation of the extension destructors to nf_conntrack_free() to fix this problem.

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=10875

Reported-and-Tested-by Krzysztof Piotr Oledzki <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>

---
commit 8a3d245effe6e699e587133a3f8ea700bd47842d
tree 38676f126c592455747598b6d56bccf9550d0214
parent 21fa91adce646ad0449e898a64edaa828ca131e7
author Patrick McHardy <[email protected]> Tue, 10 Jun 2008 10:56:29 +0200 committer Patrick McHardy <[email protected]> Tue, 10 Jun 2008 10:56:29 +0200

 net/netfilter/nf_conntrack_core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_core.c +1 -2
index c4b1799..662c1cc 100644
@@ -196,8 +196,6 @@ destroy_conntrack(struct nf_conntrack *nfct)
 	if (l4proto && l4proto->destroy)
 		l4proto->destroy(ct);
 
-	nf_ct_ext_destroy(ct);
-
 	rcu_read_unlock();
 
 	spin_lock_bh(&nf_conntrack_lock);

@@ -520,6 +518,7 @@ static void nf_conntrack_free_rcu(struct rcu_head *head)
 
 void nf_conntrack_free(struct nf_conn *ct)
 {
+	nf_ct_ext_destroy(ct);
 	call_rcu(&ct->rcu, nf_conntrack_free_rcu);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_free);

netfilter: nf_nat: fix RCU races

Fix three ct_extend/NAT extension related races:

- When cleaning up the extension area and removing it from the bysource hash, the nat->ct pointer must not be set to NULL since it may still be used in a RCU read side

- When replacing a NAT extension area in the bysource hash, the nat->ct pointer must be assigned before performing the replacement

- When reallocating extension storage in ct_extend, the old memory must not be freed immediately since it may still be used by a RCU read side

Possibly fixes https://bugzilla.redhat.com/show_bug.cgi?id=449315
and/or http://bugzilla.kernel.org/show_bug.cgi?id=10875

Signed-off-by Patrick McHardy <[email protected]>

---
commit f4efed322f3d3a30df1bb5fc33403e84aca66d8e
tree 72d463aa289ab27850f40c76b68a24e91af7a6b0
parent 8a3d245effe6e699e587133a3f8ea700bd47842d
author Patrick McHardy <[email protected]> Tue, 10 Jun 2008 11:00:35 +0200 committer Patrick McHardy <[email protected]> Tue, 10 Jun 2008 11:00:35 +0200

 include/net/netfilter/nf_conntrack_extend.h |    1 +
 net/ipv4/netfilter/nf_nat_core.c            |    3 +--
 net/netfilter/nf_conntrack_extend.c         |    9 ++++++++-
 3 files changed, 10 insertions(+), 3 deletions(-)
include/net/netfilter/nf_conntrack_extend.h +1 -0
index f736e84..f80c0ed 100644
@@ -15,6 +15,7 @@ enum nf_ct_ext_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
+	struct rcu_head rcu;
 	u8 offset[NF_CT_EXT_NUM];
 	u8 len;
 	char data[0];
net/ipv4/netfilter/nf_nat_core.c +1 -2
index 0457859..d2a887f 100644
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 
 	spin_lock_bh(&nf_nat_lock);
 	hlist_del_rcu(&nat->bysource);
-	nat->ct = NULL;
 	spin_unlock_bh(&nf_nat_lock);
 }
 

@@ -570,8 +569,8 @@ static void nf_nat_move_storage(void *new, void *old)
 		return;
 
 	spin_lock_bh(&nf_nat_lock);
-	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	new_nat->ct = ct;
+	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	spin_unlock_bh(&nf_nat_lock);
 }
net/netfilter/nf_conntrack_extend.c +8 -1
index bcc19fa..8a3f8b3 100644
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
 	return (void *)(*ext) + off;
 }
 
+static void __nf_ct_ext_free_rcu(struct rcu_head *head)
+{
+	struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
+	kfree(ext);
+}
+
 void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {
 	struct nf_ct_ext *new;

@@ -106,7 +113,7 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
-		kfree(ct->ext);
+		call_rcu(&ct->ext->rcu, __nf_ct_ext_free_rcu);
 		ct->ext = new;
 	}
#5 lore raw
Quoted 20 lines · Patrick McHardy
Quoted 19 lines · Krzysztof Oledzki
Quoted 17 lines · Patrick McHardy
Quoted 16 lines · Patrick McHardy
Quoted 14 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

        hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
                ct = nat->ct;
                if (same_src(ct, tuple)) {

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

We found the reason for that crash and I've queued these two patches. Please let me know whether they also fix the problem from the redhat bugzilla.

There is a one thing that still bugs me. This patch removes setting the nat->ct pointer to NULL:

net/ipv4/netfilter/nf_nat_core.c +0 -1
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 
        spin_lock_bh(&nf_nat_lock);
        hlist_del_rcu(&nat->bysource);
-       nat->ct = NULL;
        spin_unlock_bh(&nf_nat_lock);
 }

After this patch the whole function looks like this:

/* Noone using conntrack by the time this called. */ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)

{
        struct nf_conn_nat *nat = nf_ct_ext_find(ct, NF_CT_EXT_NAT);

        if (nat == NULL || nat->ct == NULL)
                return;

        NF_CT_ASSERT(nat->ct->status & IPS_NAT_DONE_MASK);

        spin_lock_bh(&nf_nat_lock);
        hlist_del_rcu(&nat->bysource);
        spin_unlock_bh(&nf_nat_lock);
}

As you can see we still check if nat->ct is NULL here. So, or the check is now unnecessary, or it is still possible that nat->ct may become NULL. If the second statement is true than we may need to check ct before calling same_src in the find_appropriate_src function.

Best regards,

Krzysztof Olędzki

#6 lore raw
Quoted 36 lines · Krzysztof Oledzki
Quoted 3 lines · Patrick McHardy

We found the reason for that crash and I've queued these two patches. Please let me know whether they also fix the problem from the redhat bugzilla.

There is a one thing that still bugs me. This patch removes setting the nat->ct pointer to NULL:

net/ipv4/netfilter/nf_nat_core.c +0 -1
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn 
 *ct)
 
        spin_lock_bh(&nf_nat_lock);
        hlist_del_rcu(&nat->bysource);
-       nat->ct = NULL;
        spin_unlock_bh(&nf_nat_lock);
 }

After this patch the whole function looks like this:

/* Noone using conntrack by the time this called. */ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)

{
        struct nf_conn_nat *nat = nf_ct_ext_find(ct, NF_CT_EXT_NAT);

        if (nat == NULL || nat->ct == NULL)
                return;

        NF_CT_ASSERT(nat->ct->status & IPS_NAT_DONE_MASK);

        spin_lock_bh(&nf_nat_lock);
        hlist_del_rcu(&nat->bysource);
        spin_unlock_bh(&nf_nat_lock);
}
Quoted 1 line

As you can see we still check if nat->ct is NULL here. So, or the check is now unnecessary, or it is still possible that nat->ct may become NULL. If the second statement is true than we may need to check ct before calling same_src in the find_appropriate_src function.

No, the nf_nat_cleanup_nat function can be called for a NAT extension that isn't in the hash yet (and thus has nat->ct == NULL) when the nf_conntrack_alter_reply() call in nf_nat_setup_info() allocates a helper extension and need to realloc the NAT extension space.

#7 lore raw

On Tue, 10 Jun 2008, Patrick McHardy wrote:
<CUT>

Quoted 6 lines
Quoted 1 line

As you can see we still check if nat->ct is NULL here. So, or the check is now unnecessary, or it is still possible that nat->ct may become NULL. If the second statement is true than we may need to check ct before calling same_src in the find_appropriate_src function.

No, the nf_nat_cleanup_nat function can be called for a NAT extension that isn't in the hash yet (and thus has nat->ct == NULL) when the nf_conntrack_alter_reply() call in nf_nat_setup_info() allocates a helper extension and need to realloc the NAT extension space.

OK, thank you for the explanation.

Best regards,

Krzysztof Olędzki

#8 lore raw
In reply to Patrick McHardy
Quoted 114 lines · Patrick McHardy
Quoted 15 lines · Krzysztof Oledzki
Quoted 13 lines · Patrick McHardy
Quoted 12 lines · Patrick McHardy
Quoted 11 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

In find_appropriate_src():

       hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
               ct = nat->ct;
               if (same_src(ct, tuple)) {

Dereference of ct in same_src() causes the oops. This only seems to happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines

The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67 ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause of the problem.

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

We found the reason for that crash and I've queued these two patches. Please let me know whether they also fix the problem from the redhat bugzilla.

netfilter: nf_conntrack: fix ctnetlink related crash in nf_nat_setup_info()

When creation of a new conntrack entry in ctnetlink fails after having set up the NAT mappings, the conntrack has an extension area allocated that is not getting properly destroyed when freeing the conntrack again. This means the NAT extension is still in the bysource hash, causing a crash when walking over the hash chain the next time:

BUG: unable to handle kernel paging request at 00120fbd
IP: [<c03d394b>] nf_nat_setup_info+0x221/0x58a
*pde = 00000000
Oops: 0000 [#1] PREEMPT SMP

Pid: 2795, comm: conntrackd Not tainted (2.6.26-rc5 #1)
EIP: 0060:[<c03d394b>] EFLAGS: 00010206 CPU: 1
EIP is at nf_nat_setup_info+0x221/0x58a
EAX: 00120fbd EBX: 00120fbd ECX: 00000001 EDX: 00000000
ESI: 0000019e EDI: e853bbb4 EBP: e853bbc8 ESP: e853bb78
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process conntrackd (pid: 2795, ti=e853a000 task=f7de10f0 task.ti=e853a000)

Stack: 00000000 e853bc2c e85672ec 00000008 c0561084 63c1db4a 00000000 00000000
       00000000 0002e109 61d2b1c3 00000000 00000000 00000000 01114e22 61d2b1c3
       00000000 00000000 f7444674 e853bc04 00000008 c038e728 0000000a f7444674
Call Trace:
 [<c038e728>] nla_parse+0x5c/0xb0
 [<c0397c1b>] ctnetlink_change_status+0x190/0x1c6
 [<c0397eec>] ctnetlink_new_conntrack+0x189/0x61f
 [<c0119aee>] update_curr+0x3d/0x52
 [<c03902d1>] nfnetlink_rcv_msg+0xc1/0xd8
 [<c0390228>] nfnetlink_rcv_msg+0x18/0xd8
 [<c0390210>] nfnetlink_rcv_msg+0x0/0xd8
 [<c038d2ce>] netlink_rcv_skb+0x2d/0x71
 [<c0390205>] nfnetlink_rcv+0x19/0x24
 [<c038d0f5>] netlink_unicast+0x1b3/0x216
 ...

Move invocation of the extension destructors to nf_conntrack_free() to fix this problem.

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=10875

Reported-and-Tested-by Krzysztof Piotr Oledzki <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>

---
commit 8a3d245effe6e699e587133a3f8ea700bd47842d
tree 38676f126c592455747598b6d56bccf9550d0214
parent 21fa91adce646ad0449e898a64edaa828ca131e7
author Patrick McHardy <[email protected]> Tue, 10 Jun 2008 10:56:29 +0200 committer Patrick McHardy <[email protected]> Tue, 10 Jun 2008 10:56:29 +0200

 net/netfilter/nf_conntrack_core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_core.c +1 -2
index c4b1799..662c1cc 100644
@@ -196,8 +196,6 @@ destroy_conntrack(struct nf_conntrack *nfct)
 	if (l4proto && l4proto->destroy)
 		l4proto->destroy(ct);
 
-	nf_ct_ext_destroy(ct);
-
 	rcu_read_unlock();
 
 	spin_lock_bh(&nf_conntrack_lock);

@@ -520,6 +518,7 @@ static void nf_conntrack_free_rcu(struct rcu_head *head)
 
 void nf_conntrack_free(struct nf_conn *ct)
 {
+	nf_ct_ext_destroy(ct);
 	call_rcu(&ct->rcu, nf_conntrack_free_rcu);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_free);

netfilter: nf_nat: fix RCU races

Fix three ct_extend/NAT extension related races:

- When cleaning up the extension area and removing it from the bysource hash, the nat->ct pointer must not be set to NULL since it may still be used in a RCU read side

Quoted 8 lines

- When replacing a NAT extension area in the bysource hash, the nat->ct pointer must be assigned before performing the replacement

- When reallocating extension storage in ct_extend, the old memory must not be freed immediately since it may still be used by a RCU read side

Possibly fixes https://bugzilla.redhat.com/show_bug.cgi?id=449315
and/or http://bugzilla.kernel.org/show_bug.cgi?id=10875

One question and one nit below.

Thanx, Paul

Quoted 39 lines
Signed-off-by Patrick McHardy <[email protected]>

---
commit f4efed322f3d3a30df1bb5fc33403e84aca66d8e
tree 72d463aa289ab27850f40c76b68a24e91af7a6b0
parent 8a3d245effe6e699e587133a3f8ea700bd47842d
author Patrick McHardy <[email protected]> Tue, 10 Jun 2008 11:00:35 +0200 committer Patrick McHardy <[email protected]> Tue, 10 Jun 2008 11:00:35 +0200

 include/net/netfilter/nf_conntrack_extend.h |    1 +
 net/ipv4/netfilter/nf_nat_core.c            |    3 +--
 net/netfilter/nf_conntrack_extend.c         |    9 ++++++++-
 3 files changed, 10 insertions(+), 3 deletions(-)
include/net/netfilter/nf_conntrack_extend.h +1 -0
index f736e84..f80c0ed 100644
@@ -15,6 +15,7 @@ enum nf_ct_ext_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
+	struct rcu_head rcu;
 	u8 offset[NF_CT_EXT_NUM];
 	u8 len;
 	char data[0];
net/ipv4/netfilter/nf_nat_core.c +0 -1
index 0457859..d2a887f 100644
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 
 	spin_lock_bh(&nf_nat_lock);
 	hlist_del_rcu(&nat->bysource);
-	nat->ct = NULL;
 	spin_unlock_bh(&nf_nat_lock);
 }

net/ipv4/netfilter/nf_nat_core.c static void nf_nat_move_storage(void *new, void *old) · line 573

Quoted 6 lines
net/ipv4/netfilter/nf_nat_core.c +1 -1
@@ -570,8 +569,8 @@ static void nf_nat_move_storage(void *new, void *old)
 		return;
 
 	spin_lock_bh(&nf_nat_lock);
-	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	new_nat->ct = ct;
+	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);

The intent is to ensure that new_nat->ct is initialized before any readers can find new_nat, right? If so, OK.

net/netfilter/nf_conntrack_extend.c nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp) · line 62

Quoted 12 lines
net/ipv4/netfilter/nf_nat_core.c +0 -0
 	spin_unlock_bh(&nf_nat_lock);
 }
net/netfilter/nf_conntrack_extend.c +1 -0
index bcc19fa..8a3f8b3 100644
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);

Nit: the above is unnecessary.

Quoted 24 lines
net/netfilter/nf_conntrack_extend.c +7 -1
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
 	return (void *)(*ext) + off;
 }
 
+static void __nf_ct_ext_free_rcu(struct rcu_head *head)
+{
+	struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
+	kfree(ext);
+}
+
 void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {
 	struct nf_ct_ext *new;

@@ -106,7 +113,7 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
-		kfree(ct->ext);
+		call_rcu(&ct->ext->rcu, __nf_ct_ext_free_rcu);
 		ct->ext = new;
 	}
#9 lore raw
Quoted 9 lines · Paul E. McKenney
Quoted 2 lines · Patrick McHardy

Possibly fixes https://bugzilla.redhat.com/show_bug.cgi?id=449315
and/or http://bugzilla.kernel.org/show_bug.cgi?id=10875

One question and one nit below.

Quoted 3 lines
(quoted diff) +0 -0
@@ -570,8 +569,8 @@ static void nf_nat_move_storage(void *new, void *old)
 		return;
Quoted 7 lines

net/ipv4/netfilter/nf_nat_core.c static void nf_nat_move_storage(void *new, void *old) · line 573

Quoted 4 lines
net/ipv4/netfilter/nf_nat_core.c +1 -1
 	spin_lock_bh(&nf_nat_lock);
-	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	new_nat->ct = ct;
+	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);

The intent is to ensure that new_nat->ct is initialized before any readers can find new_nat, right? If so, OK.

Correct. Its relying on the smb_wmb() in hlist_replace_rcu(), but that seems OK.

Quoted 11 lines

net/netfilter/nf_conntrack_extend.c nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp) · line 62

Quoted 9 lines
net/netfilter/nf_conntrack_extend.c +1 -0
index bcc19fa..8a3f8b3 100644
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);

Nit: the above is unnecessary.

I think its good style to use explicit initializers without making assumptions about what exactly they do.

Thanks for the review.

#10 lore raw
In reply to Patrick McHardy
Quoted 5 lines · Patrick McHardy
Quoted 5 lines · Patrick McHardy
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines · Chuck Ebbert

Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315

Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

We have a similar looking report, but that one also affects 2.6.24:

http://bugzilla.kernel.org/show_bug.cgi?id=10875

We found the reason for that crash and I've queued these two patches. Please let me know whether they also fix the problem from the redhat bugzilla.

Since there doesn't seem to be any progress, I'm going to send this upstream now so we can get some testing before 2.6.26 is released.

I'm going to hold off sending it to -stable though until there is confirmation that it actually fixes this problem.