Skip to contentSkip to conversation

fix double helper assignation for NAT'ed conntracks

PATCH Applied Pablo Neira Ayuso 2 participants 7 messages Aug 7, 2008 – Aug 13, 2008

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 1/3 fix double helper assignation for NAT'ed conntracks

If we create a conntrack that has NAT handlings and a helper, the helper is assigned twice. This happens because nf_nat_setup_info() - via nf_conntrack_alter_reply() - sets the helper before ctnetlink, which indeed does not check if the conntrack already has a helper as it thinks that it is a brand new conntrack.

The fix moves the helper assignation before the set of the status flags. This avoids a bogus assertion in __nf_ct_ext_add (if netfilter assertions are enabled) which checks that the conntrack must not be confirmed.

This problem was introduced in 2.6.23 with the netfilter extension infrastructure.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 net/netfilter/nf_conntrack_netlink.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)
net/netfilter/nf_conntrack_netlink.c +19 -15
index dd23339..a984b1c 100644
@@ -1139,16 +1139,33 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
 	ct->status |= IPS_CONFIRMED;
 
+	rcu_read_lock();
+	helper = __nf_ct_helper_find(rtuple);
+	if (helper) {
+		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+		if (help == NULL) {
+			rcu_read_unlock();
+			err = -ENOMEM;
+			goto err;
+		}
+		/* not in hash table yet so not strictly necessary */
+		rcu_assign_pointer(help->helper, helper);
+	}
+
 	if (cda[CTA_STATUS]) {
 		err = ctnetlink_change_status(ct, cda);
-		if (err < 0)
+		if (err < 0) {
+			rcu_read_unlock();
 			goto err;
+		}
 	}
 
 	if (cda[CTA_PROTOINFO]) {
 		err = ctnetlink_change_protoinfo(ct, cda);
-		if (err < 0)
+		if (err < 0) {
+			rcu_read_unlock();
 			goto err;
+		}
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)

@@ -1156,19 +1173,6 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
 #endif
 
-	rcu_read_lock();
-	helper = __nf_ct_helper_find(rtuple);
-	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
-		if (help == NULL) {
-			rcu_read_unlock();
-			err = -ENOMEM;
-			goto err;
-		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
-	}
-
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
 		__set_bit(IPS_EXPECTED_BIT, &ct->status);
author maintainer
#1 lore raw

PATCH 2/3 fix sleep in read-side lock section

Fix allocation with GFP_KERNEL in ctnetlink_create_conntrack() under read-side lock sections.

This problem was introduced in 2.6.25.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 net/netfilter/nf_conntrack_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/netfilter/nf_conntrack_netlink.c +1 -1
index a984b1c..7707cbb 100644
@@ -1142,7 +1142,7 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 	rcu_read_lock();
 	helper = __nf_ct_helper_find(rtuple);
 	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 		if (help == NULL) {
 			rcu_read_unlock();
 			err = -ENOMEM;
Applied
#2 lore raw

Re: [PATCH 2/3] [PATCH] fix sleep in read-side lock section

Quoted 4 lines · Pablo Neira Ayuso

Fix allocation with GFP_KERNEL in ctnetlink_create_conntrack() under read-side lock sections.

This problem was introduced in 2.6.25.

Applied, thanks.

author maintainer
#3 lore raw

PATCH 3/3 fix sleepable allocation with spin lock bh

In reply to Pablo Neira Ayuso

This patch removes a GFP_KERNEL allocation while holding a spin lock with bottom halves disabled in ctnetlink_change_helper().

This problem was introduced in 2.6.23 with the netfilter extension infrastructure.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---

 net/netfilter/nf_conntrack_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/netfilter/nf_conntrack_netlink.c +1 -1
index 7707cbb..35e61e4 100644
@@ -971,7 +971,7 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
 		/* need to zero data of old helper */
 		memset(&help->help, 0, sizeof(help->help));
 	} else {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 		if (help == NULL)
 			return -ENOMEM;
 	}
#4 lore raw

Re: [PATCH 3/3] [PATCH] fix sleepable allocation with spin lock bh

Quoted 5 lines · Pablo Neira Ayuso

This patch removes a GFP_KERNEL allocation while holding a spin lock with bottom halves disabled in ctnetlink_change_helper().

This problem was introduced in 2.6.23 with the netfilter extension infrastructure.

Also applied, thanks. I'll push all three to -stable once they are upstream.

author maintainer
#5 lore raw

Re: [PATCH 3/3] [PATCH] fix sleepable allocation with spin lock bh

Quoted 9 lines · Patrick McHardy
Quoted 5 lines · Pablo Neira Ayuso

This patch removes a GFP_KERNEL allocation while holding a spin lock with bottom halves disabled in ctnetlink_change_helper().

This problem was introduced in 2.6.23 with the netfilter extension infrastructure.

Also applied, thanks. I'll push all three to -stable once they are upstream.

Thanks Patrick. Let me know if you need any backport of the patches.

Signature · 1 line
"Los honestos son inadaptados sociales" -- Les Luthiers
Applied
#6 lore raw
In reply to Pablo Neira Ayuso
Quoted 6 lines · Pablo Neira Ayuso

If we create a conntrack that has NAT handlings and a helper, the helper is assigned twice. This happens because nf_nat_setup_info() - via nf_conntrack_alter_reply() - sets the helper before ctnetlink, which indeed does not check if the conntrack already has a helper as it thinks that it is a brand new conntrack.

Quoted 6 lines

The fix moves the helper assignation before the set of the status flags. This avoids a bogus assertion in __nf_ct_ext_add (if netfilter assertions are enabled) which checks that the conntrack must not be confirmed.

This problem was introduced in 2.6.23 with the netfilter extension infrastructure.

Applied, thanks.