[PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Pablo Neira Ayuso]

This patch moves the assignation of the master conntrack to
ctnetlink_create_conntrack(), which is where it really belongs.
This patch is a cleanup.

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

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 41226f0..a240a66 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1234,9 +1234,9 @@ static int
 ctnetlink_create_conntrack(struct nlattr *cda[],
 			   struct nf_conntrack_tuple *otuple,
 			   struct nf_conntrack_tuple *rtuple,
-			   struct nf_conn *master_ct,
 			   u32 pid,
-			   int report)
+			   int report,
+			   u8 u3)
 {
 	struct nf_conn *ct;
 	int err = -EINVAL;
@@ -1347,7 +1347,22 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 #endif
 
 	/* setup master conntrack: this is a confirmed expectation */
-	if (master_ct) {
+	if (cda[CTA_TUPLE_MASTER]) {
+		struct nf_conntrack_tuple master;
+		struct nf_conntrack_tuple_hash *master_h;
+		struct nf_conn *master_ct;
+
+		err = ctnetlink_parse_tuple(cda, &master, CTA_TUPLE_MASTER, u3);
+		if (err < 0)
+			goto err;
+
+		master_h = __nf_conntrack_find(&init_net, &master);
+		if (master_h == NULL) {
+			err = -ENOENT;
+			goto err;
+		}
+		master_ct = nf_ct_tuplehash_to_ctrack(master_h);
+		nf_conntrack_get(&master_ct->ct_general);
 		__set_bit(IPS_EXPECTED_BIT, &ct->status);
 		ct->master = master_ct;
 	}
@@ -1395,39 +1410,15 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 		h = __nf_conntrack_find(&init_net, &rtuple);
 
 	if (h == NULL) {
-		struct nf_conntrack_tuple master;
-		struct nf_conntrack_tuple_hash *master_h = NULL;
-		struct nf_conn *master_ct = NULL;
-
-		if (cda[CTA_TUPLE_MASTER]) {
-			err = ctnetlink_parse_tuple(cda,
-						    &master,
-						    CTA_TUPLE_MASTER,
-						    u3);
-			if (err < 0)
-				goto out_unlock;
-
-			master_h = __nf_conntrack_find(&init_net, &master);
-			if (master_h == NULL) {
-				err = -ENOENT;
-				goto out_unlock;
-			}
-			master_ct = nf_ct_tuplehash_to_ctrack(master_h);
-			nf_conntrack_get(&master_ct->ct_general);
-		}
-
 		err = -ENOENT;
 		if (nlh->nlmsg_flags & NLM_F_CREATE)
 			err = ctnetlink_create_conntrack(cda,
 							 &otuple,
 							 &rtuple,
-							 master_ct,
 							 NETLINK_CB(skb).pid,
-							 nlmsg_report(nlh));
+							 nlmsg_report(nlh),
+							 u3);
 		spin_unlock_bh(&nf_conntrack_lock);
-		if (err < 0 && master_ct)
-			nf_ct_put(master_ct);
-
 		return err;
 	}
 	/* implicit 'else' */

[PATCH 2/3] netfilter: ctnetlink: cleanup conntrack update preliminary checkings

[Pablo Neira Ayuso]

This patch moves the preliminary checkings that must be fulfilled
to update a conntrack, which are the following:

 * NAT manglings cannot be updated
 * Changing the master conntrack is not allowed.

This patch is a cleanup.

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

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index a240a66..d229370 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1168,6 +1168,10 @@ ctnetlink_change_conntrack(struct nf_conn *ct, struct nlattr *cda[])
 {
 	int err;
 
+	/* only allow NAT changes and master assignation for new conntracks */
+	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST] || cda[CTA_TUPLE_MASTER])
+		return -EOPNOTSUPP;
+
 	if (cda[CTA_HELP]) {
 		err = ctnetlink_change_helper(ct, cda);
 		if (err < 0)
@@ -1429,17 +1433,6 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 	if (!(nlh->nlmsg_flags & NLM_F_EXCL)) {
 		struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
 
-		/* we only allow nat config for new conntracks */
-		if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
-			err = -EOPNOTSUPP;
-			goto out_unlock;
-		}
-		/* can't link an existing conntrack to a master */
-		if (cda[CTA_TUPLE_MASTER]) {
-			err = -EOPNOTSUPP;
-			goto out_unlock;
-		}
-
 		err = ctnetlink_change_conntrack(ct, cda);
 		if (err == 0) {
 			nf_conntrack_get(&ct->ct_general);

[PATCH 3/3] netfilter: ctnetlink: move event reporting for new entries outside the lock

[Pablo Neira Ayuso]

This patch moves the event reporting outside the lock section. With
this patch, the creation and update of entries is homogeneous from
the event reporting perspective. Moreover, as the event reporting is
done outside the lock section, the netlink broadcast delivery can
benefit of the yield() call under congestion.

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

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index d229370..fa5b23f 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1234,12 +1234,10 @@ ctnetlink_event_report(struct nf_conn *ct, u32 pid, int report)
 				  report);
 }
 
-static int
+static struct nf_conn *
 ctnetlink_create_conntrack(struct nlattr *cda[],
 			   struct nf_conntrack_tuple *otuple,
 			   struct nf_conntrack_tuple *rtuple,
-			   u32 pid,
-			   int report,
 			   u8 u3)
 {
 	struct nf_conn *ct;
@@ -1248,7 +1246,7 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 
 	ct = nf_conntrack_alloc(&init_net, otuple, rtuple, GFP_ATOMIC);
 	if (IS_ERR(ct))
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	if (!cda[CTA_TIMEOUT])
 		goto err;
@@ -1371,18 +1369,14 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 		ct->master = master_ct;
 	}
 
-	nf_conntrack_get(&ct->ct_general);
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
 	rcu_read_unlock();
-	ctnetlink_event_report(ct, pid, report);
-	nf_ct_put(ct);
-
-	return 0;
 
+	return ct;
 err:
 	nf_conntrack_free(ct);
-	return err;
+	return ERR_PTR(err);
 }
 
 static int
@@ -1415,14 +1409,25 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 
 	if (h == NULL) {
 		err = -ENOENT;
-		if (nlh->nlmsg_flags & NLM_F_CREATE)
-			err = ctnetlink_create_conntrack(cda,
-							 &otuple,
-							 &rtuple,
-							 NETLINK_CB(skb).pid,
-							 nlmsg_report(nlh),
-							 u3);
-		spin_unlock_bh(&nf_conntrack_lock);
+		if (nlh->nlmsg_flags & NLM_F_CREATE) {
+			struct nf_conn *ct;
+
+			ct = ctnetlink_create_conntrack(cda, &otuple,
+							&rtuple, u3);
+			if (IS_ERR(ct)) {
+				err = PTR_ERR(ct);
+				goto out_unlock;
+			}
+			err = 0;
+			nf_conntrack_get(&ct->ct_general);
+			spin_unlock_bh(&nf_conntrack_lock);
+			ctnetlink_event_report(ct,
+					       NETLINK_CB(skb).pid,
+					       nlmsg_report(nlh));
+			nf_ct_put(ct);
+		} else
+			spin_unlock_bh(&nf_conntrack_lock);
+
 		return err;
 	}
 	/* implicit 'else' */

Re: [PATCH 3/3] netfilter: ctnetlink: move event reporting for new entries outside the lock

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch moves the event reporting outside the lock section. With
> this patch, the creation and update of entries is homogeneous from
> the event reporting perspective. Moreover, as the event reporting is
> done outside the lock section, the netlink broadcast delivery can
> benefit of the yield() call under congestion.

This one doesn't apply cleanly, probably because of the missing
netlink_set_err patch. I've pushed out my current nf-next tree,
please rediff against that tree.

Re: [PATCH 3/3] netfilter: ctnetlink: move event reporting for new entries outside the lock

[Patrick McHardy]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> This patch moves the event reporting outside the lock section. With
>> this patch, the creation and update of entries is homogeneous from
>> the event reporting perspective. Moreover, as the event reporting is
>> done outside the lock section, the netlink broadcast delivery can
>> benefit of the yield() call under congestion.
> 
> This one doesn't apply cleanly, probably because of the missing
> netlink_set_err patch. I've pushed out my current nf-next tree,
> please rediff against that tree.

It might also have been ordering, let me try again :)

Re: [PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch moves the assignation of the master conntrack to
> ctnetlink_create_conntrack(), which is where it really belongs.
> This patch is a cleanup.

Applied, thanks.

Re: [PATCH 2/3] netfilter: ctnetlink: cleanup conntrack update preliminary checkings

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch moves the preliminary checkings that must be fulfilled
> to update a conntrack, which are the following:
> 
>  * NAT manglings cannot be updated
>  * Changing the master conntrack is not allowed.
> 
> This patch is a cleanup.

Applied, thanks.

Re: [PATCH 3/3] netfilter: ctnetlink: move event reporting for new entries outside the lock

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch moves the event reporting outside the lock section. With
> this patch, the creation and update of entries is homogeneous from
> the event reporting perspective. Moreover, as the event reporting is
> done outside the lock section, the netlink broadcast delivery can
> benefit of the yield() call under congestion.

Applied, thanks.

Re: [PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> This patch moves the assignation of the master conntrack to
>> ctnetlink_create_conntrack(), which is where it really belongs.
>> This patch is a cleanup.
> 
> Applied, thanks.

I've added this patch on top to fix a RCU context imbalance.

This seems like a good opportunity to say this again: please (everyone)
compile your code using sparse. It catches this type and more bugs.

There is a second bug introduced by this patch:

	add_timer(&ct->timeout);
	nf_conntrack_hash_insert(ct);
	rcu_read_unlock();

	return ct;

The conntrack lock is not held, it might crash or create double entries.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3745 bytes --]

commit 0f5b3e85a3716efebb0150ebb7c6d022e2bf17d7
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Mar 18 17:36:40 2009 +0100

    netfilter: ctnetlink: fix rcu context imbalance
    
    Introduced by 7ec47496 (netfilter: ctnetlink: cleanup master conntrack assignation):
    
    net/netfilter/nf_conntrack_netlink.c:1275:2: warning: context imbalance in 'ctnetlink_create_conntrack' - different lock contexts for basic block
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 735ea9c..d1fe9d1 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1146,7 +1146,7 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 		return ERR_PTR(-ENOMEM);
 
 	if (!cda[CTA_TIMEOUT])
-		goto err;
+		goto err1;
 	ct->timeout.expires = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
@@ -1157,10 +1157,8 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
  		char *helpname;
  
  		err = ctnetlink_parse_help(cda[CTA_HELP], &helpname);
- 		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+ 		if (err < 0)
+			goto err2;
 
 		helper = __nf_conntrack_helper_find_byname(helpname);
 		if (helper == NULL) {
@@ -1168,28 +1166,26 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 #ifdef CONFIG_MODULES
 			if (request_module("nfct-helper-%s", helpname) < 0) {
 				err = -EOPNOTSUPP;
-				goto err;
+				goto err1;
 			}
 
 			rcu_read_lock();
 			helper = __nf_conntrack_helper_find_byname(helpname);
 			if (helper) {
-				rcu_read_unlock();
 				err = -EAGAIN;
-				goto err;
+				goto err2;
 			}
 			rcu_read_unlock();
 #endif
 			err = -EOPNOTSUPP;
-			goto err;
+			goto err1;
 		} else {
 			struct nf_conn_help *help;
 
 			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 			if (help == NULL) {
-				rcu_read_unlock();
 				err = -ENOMEM;
-				goto err;
+				goto err2;
 			}
 
 			/* not in hash table yet so not strictly necessary */
@@ -1198,44 +1194,34 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 	} else {
 		/* try an implicit helper assignation */
 		err = __nf_ct_try_assign_helper(ct, GFP_ATOMIC);
-		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+		if (err < 0)
+			goto err2;
 	}
 
 	if (cda[CTA_STATUS]) {
 		err = ctnetlink_change_status(ct, cda);
-		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+		if (err < 0)
+			goto err2;
 	}
 
 	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
 		err = ctnetlink_change_nat(ct, cda);
-		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+		if (err < 0)
+			goto err2;
 	}
 
 #ifdef CONFIG_NF_NAT_NEEDED
 	if (cda[CTA_NAT_SEQ_ADJ_ORIG] || cda[CTA_NAT_SEQ_ADJ_REPLY]) {
 		err = ctnetlink_change_nat_seq_adj(ct, cda);
-		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+		if (err < 0)
+			goto err2;
 	}
 #endif
 
 	if (cda[CTA_PROTOINFO]) {
 		err = ctnetlink_change_protoinfo(ct, cda);
-		if (err < 0) {
-			rcu_read_unlock();
-			goto err;
-		}
+		if (err < 0)
+			goto err2;
 	}
 
 	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
@@ -1253,12 +1239,12 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 
 		err = ctnetlink_parse_tuple(cda, &master, CTA_TUPLE_MASTER, u3);
 		if (err < 0)
-			goto err;
+			goto err2;
 
 		master_h = __nf_conntrack_find(&init_net, &master);
 		if (master_h == NULL) {
 			err = -ENOENT;
-			goto err;
+			goto err2;
 		}
 		master_ct = nf_ct_tuplehash_to_ctrack(master_h);
 		nf_conntrack_get(&master_ct->ct_general);
@@ -1271,7 +1257,10 @@ ctnetlink_create_conntrack(struct nlattr *cda[],
 	rcu_read_unlock();
 
 	return ct;
-err:
+
+err2:
+	rcu_read_unlock();
+err1:
 	nf_conntrack_free(ct);
 	return ERR_PTR(err);
 }

Re: [PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> This patch moves the assignation of the master conntrack to
>>> ctnetlink_create_conntrack(), which is where it really belongs.
>>> This patch is a cleanup.
>>
>> Applied, thanks.
> 
> I've added this patch on top to fix a RCU context imbalance.

Oh, stupid mistake, sorry for that.

> This seems like a good opportunity to say this again: please (everyone)
> compile your code using sparse. It catches this type and more bugs.

I'll do.

> There is a second bug introduced by this patch:
> 
>     add_timer(&ct->timeout);
>     nf_conntrack_hash_insert(ct);
>     rcu_read_unlock();
> 
>     return ct;
> 
> The conntrack lock is not held, it might crash or create double entries.

Hm, but that code is inside ctnetlink_create_conntrack() which is called
with the conntrack lock held.

       if (nlh->nlmsg_flags & NLM_F_CREATE)
                 err = ctnetlink_create_conntrack(cda,
                                                  &otuple,
                                                  &rtuple,
-                                                 master_ct,
                                                  NETLINK_CB(skb).pid,
-                                                 nlmsg_report(nlh));
+                                                 nlmsg_report(nlh),
+                                                 u3);
                spin_unlock_bh(&nf_conntrack_lock);

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

Re: [PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>   
>> The conntrack lock is not held, it might crash or create double entries.
>>     
>
> Hm, but that code is inside ctnetlink_create_conntrack() which is called
> with the conntrack lock held.
>
>        if (nlh->nlmsg_flags & NLM_F_CREATE)
>                  err = ctnetlink_create_conntrack(cda,
>                                                   &otuple,
>                                                   &rtuple,
> -                                                 master_ct,
>                                                   NETLINK_CB(skb).pid,
> -                                                 nlmsg_report(nlh));
> +                                                 nlmsg_report(nlh),
> +                                                 u3);
>                 spin_unlock_bh(&nf_conntrack_lock);
>   

Right, I missed that, sorry.