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

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

[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.

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 <pablo@netfilter.org>
---

 net/netfilter/nf_conntrack_netlink.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index dd23339..a984b1c 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -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);

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

[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.

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

 net/netfilter/nf_conntrack_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index a984b1c..7707cbb 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -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;

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

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Fix allocation with GFP_KERNEL in ctnetlink_create_conntrack() under
> read-side lock sections.
> 
> This problem was introduced in 2.6.25.

Applied, thanks.

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

[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 <pablo@netfilter.org>
---

 net/netfilter/nf_conntrack_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7707cbb..35e61e4 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -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;
 	}

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

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> 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.

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

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> 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.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH 1/3] [PATCH] fix double helper assignation for NAT'ed conntracks

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> 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.

Applied, thanks.