[PATCHv2] deliver events for conntracks created via ctnetlink

[PATCHv2] deliver events for conntracks created via ctnetlink

[Pablo Neira Ayuso]

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

As for now, the creation and update of conntracks via ctnetlink do not
propagate an event to userspace. This can result in inconsistent
situations if several userspace processes modify the connection tracking
table by means of ctnetlink at the same time. Specifically, using the
conntrack command line tool and conntrackd at the same time can trigger
unconsistencies.

This patch fixes this inconsistent situation. Note that the deletion
does not suffer from this problem.

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

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

[-- Attachment #2: 05.patch --]
[-- Type: text/x-diff, Size: 5910 bytes --]

[PATCH] deliver events for conntracks created via ctnetlink

As for now, the creation and update of conntracks via ctnetlink do not
propagate an event to userspace. This can result in inconsistent situations
if several userspace processes modify the connection tracking table by means
of ctnetlink at the same time. Specifically, using the conntrack command
line tool and conntrackd at the same time can trigger unconsistencies. 

This patch fixes this inconsistent situation. Note that the deletion does
not suffer from this problem.

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

Index: net-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-06-08 20:26:36.000000000 +0200
+++ net-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-06-14 12:22:31.000000000 +0200
@@ -36,6 +36,7 @@
 #include <net/netfilter/nf_conntrack_l3proto.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
 #include <net/netfilter/nf_conntrack_tuple.h>
+#include <net/netfilter/nf_conntrack_ecache.h>
 #ifdef CONFIG_NF_NAT_NEEDED
 #include <net/netfilter/nf_nat_core.h>
 #include <net/netfilter/nf_nat_protocol.h>
@@ -933,6 +934,9 @@ ctnetlink_change_status(struct nf_conn *
 	 * so don't let users modify them directly if they don't pass
 	 * nf_nat_range. */
 	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+
+	nf_conntrack_event_cache_ct(IPCT_STATUS, ct);
+
 	return 0;
 }
 
@@ -982,6 +986,8 @@ ctnetlink_change_helper(struct nf_conn *
 
 	rcu_assign_pointer(help->helper, helper);
 
+	nf_conntrack_event_cache_ct(IPCT_HELPER, ct);
+
 	return 0;
 }
 
@@ -996,6 +1002,8 @@ ctnetlink_change_timeout(struct nf_conn 
 	ct->timeout.expires = jiffies + timeout * HZ;
 	add_timer(&ct->timeout);
 
+	nf_conntrack_event_cache_ct(IPCT_REFRESH, ct);
+
 	return 0;
 }
 
@@ -1009,8 +1017,11 @@ ctnetlink_change_protoinfo(struct nf_con
 	nla_parse_nested(tb, CTA_PROTOINFO_MAX, attr, NULL);
 
 	l4proto = nf_ct_l4proto_find_get(nf_ct_l3num(ct), nf_ct_protonum(ct));
-	if (l4proto->from_nlattr)
+	if (l4proto->from_nlattr) {
 		err = l4proto->from_nlattr(tb, ct);
+		if (err == 0)
+			nf_conntrack_event_cache_ct(IPCT_PROTOINFO, ct);
+	}
 	nf_ct_l4proto_put(l4proto);
 
 	return err;
@@ -1061,6 +1072,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		nf_conntrack_event_cache_ct(IPCT_NATSEQADJ, ct);
 	}
 
 	if (cda[CTA_NAT_SEQ_ADJ_REPLY]) {
@@ -1070,6 +1083,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		nf_conntrack_event_cache_ct(IPCT_NATSEQADJ, ct);
 	}
 
 	return 0;
@@ -1106,8 +1121,10 @@ ctnetlink_change_conntrack(struct nf_con
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		nf_conntrack_event_cache_ct(IPCT_MARK, ct);
+	}
 #endif
 
 #ifdef CONFIG_NF_NAT_NEEDED
@@ -1156,8 +1173,10 @@ ctnetlink_create_conntrack(struct nlattr
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		nf_conntrack_event_cache_ct(IPCT_MARK, ct);
+	}
 #endif
 
 	rcu_read_lock();
@@ -1171,18 +1190,27 @@ ctnetlink_create_conntrack(struct nlattr
 		}
 		/* not in hash table yet so not strictly necessary */
 		rcu_assign_pointer(help->helper, helper);
+		nf_conntrack_event_cache_ct(IPCT_HELPER, ct);
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
 		__set_bit(IPS_EXPECTED_BIT, &ct->status);
 		ct->master = master_ct;
-	}
+		nf_conntrack_event_cache_ct(IPCT_RELATED, ct);
+	} else
+		nf_conntrack_event_cache_ct(IPCT_NEW, ct);
+
+	atomic_inc(&ct->ct_general.use);
 
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
 	rcu_read_unlock();
 
+	nf_ct_deliver_cached_events(ct);
+
+	nf_ct_put(ct);
+
 	return 0;
 
 err:
@@ -1258,6 +1286,8 @@ ctnetlink_new_conntrack(struct sock *ctn
 	 * so there's no need to increase the refcount */
 	err = -EEXIST;
 	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;
@@ -1268,8 +1298,17 @@ ctnetlink_new_conntrack(struct sock *ctn
 			err = -EOPNOTSUPP;
 			goto out_unlock;
 		}
-		err = ctnetlink_change_conntrack(nf_ct_tuplehash_to_ctrack(h),
-						 cda);
+
+		err = ctnetlink_change_conntrack(ct, cda);
+		if (err == 0) {
+			atomic_inc(&ct->ct_general.use);
+			spin_unlock_bh(&nf_conntrack_lock);
+			nf_ct_deliver_cached_events(ct);
+			nf_ct_put(ct);
+		} else
+			spin_unlock(&nf_conntrack_lock);
+
+		return err;
 	}
 
 out_unlock:
Index: net-2.6.git/include/net/netfilter/nf_conntrack_ecache.h
===================================================================
--- net-2.6.git.orig/include/net/netfilter/nf_conntrack_ecache.h	2008-06-08 20:26:30.000000000 +0200
+++ net-2.6.git/include/net/netfilter/nf_conntrack_ecache.h	2008-06-14 12:16:14.000000000 +0200
@@ -28,10 +28,9 @@ extern void __nf_ct_event_cache_init(str
 extern void nf_ct_event_cache_flush(void);
 
 static inline void
-nf_conntrack_event_cache(enum ip_conntrack_events event,
-			 const struct sk_buff *skb)
+nf_conntrack_event_cache_ct(enum ip_conntrack_events event,
+			    struct nf_conn *ct)
 {
-	struct nf_conn *ct = (struct nf_conn *)skb->nfct;
 	struct nf_conntrack_ecache *ecache;
 
 	local_bh_disable();
@@ -42,6 +41,13 @@ nf_conntrack_event_cache(enum ip_conntra
 	local_bh_enable();
 }
 
+static inline void
+nf_conntrack_event_cache(enum ip_conntrack_events event,
+			 const struct sk_buff *skb)
+{
+	nf_conntrack_event_cache_ct(event, (struct nf_conn *)skb->nfct);
+}
+
 static inline void nf_conntrack_event(enum ip_conntrack_events event,
 				      struct nf_conn *ct)
 {

Re: [PATCHv2] deliver events for conntracks created via ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> As for now, the creation and update of conntracks via ctnetlink do not
> propagate an event to userspace. This can result in inconsistent
> situations if several userspace processes modify the connection tracking
> table by means of ctnetlink at the same time. Specifically, using the
> conntrack command line tool and conntrackd at the same time can trigger
> unconsistencies.
> 
> This patch fixes this inconsistent situation. Note that the deletion
> does not suffer from this problem.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 

Unfortunately all the change functions are deadlock prone,
they are called while holding the conntrack lock and
event delivery might trigger destruction of the conntrack
entry already in the cache, which takes the lock again.

Perhaps we can do all this much easier. Conntrack updates
over netlink are a lot more rare than events triggered
by packet processing. What do you think about just sending
the full entry on successful changes over ctnetlink?

A few minor nits:

> +			atomic_inc(&ct->ct_general.use);

Should be using nf_conntrack_get().

Also the patch adds newlines excessively, to a file already
containing about 20% empty lines. Things like

>  		ct->status |= IPS_SEQ_ADJUST;
> +
> +		nf_conntrack_event_cache_ct(IPCT_NATSEQADJ, ct);

or

>  	rcu_read_unlock();
>  
> +	nf_ct_deliver_cached_events(ct);
> +
> +	nf_ct_put(ct);
> +
>  	return 0;

don't aid readability.

Re: [PATCHv2] deliver events for conntracks created via ctnetlink

[Patrick McHardy]

Patrick McHardy wrote:
> A few minor nits:
> 
>> ...

One more thing: the patch still doesn't follow the correct
netlink semantic for notifations on changes triggered by
userspace I described in the review of the first version
of this patch.

Please always mention which changes requested during a
review were made and which weren't (any why) when sending
updated versions.

Re: [PATCHv2] deliver events for conntracks created via ctnetlink

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Patrick McHardy wrote:
>> A few minor nits:
>>
>>> ...
> 
> One more thing: the patch still doesn't follow the correct
> netlink semantic for notifations on changes triggered by
> userspace I described in the review of the first version
> of this patch.
> 
> Please always mention which changes requested during a
> review were made and which weren't (any why) when sending
> updated versions.

Sorry, there's no particular reason, I forgot to include this. With
regards to nlmsg_report/nlmsg_notify, I just noticed that the echo
cannot be unset for destroy messages since we call nf_conntrack_event()
inside destroy_conntrack().

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

Re: [PATCHv2] deliver events for conntracks created via ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Patrick McHardy wrote:
>>> A few minor nits:
>>>
>>>> ...
>> One more thing: the patch still doesn't follow the correct
>> netlink semantic for notifations on changes triggered by
>> userspace I described in the review of the first version
>> of this patch.
>>
>> Please always mention which changes requested during a
>> review were made and which weren't (any why) when sending
>> updated versions.
> 
> Sorry, there's no particular reason, I forgot to include this. With
> regards to nlmsg_report/nlmsg_notify, I just noticed that the echo
> cannot be unset for destroy messages since we call nf_conntrack_event()
> inside destroy_conntrack().

Mhh .. its not pretty, but one thing that might work is
deliver the event manually and then set the DYING bit
manually to avoid further event delivery for the entry.

Re: [PATCHv2] deliver events for conntracks created via ctnetlink

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> As for now, the creation and update of conntracks via ctnetlink do not
>> propagate an event to userspace. This can result in inconsistent
>> situations if several userspace processes modify the connection tracking
>> table by means of ctnetlink at the same time. Specifically, using the
>> conntrack command line tool and conntrackd at the same time can trigger
>> unconsistencies.
>>
>> This patch fixes this inconsistent situation. Note that the deletion
>> does not suffer from this problem.
>>
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Unfortunately all the change functions are deadlock prone,
> they are called while holding the conntrack lock and
> event delivery might trigger destruction of the conntrack
> entry already in the cache, which takes the lock again.

Indeed. I didn't notice the nf_ct_event_cache_init path.

> Perhaps we can do all this much easier. Conntrack updates
> over netlink are a lot more rare than events triggered
> by packet processing. What do you think about just sending
> the full entry on successful changes over ctnetlink?

Yes, that is simple.

> A few minor nits:
> 
>> +            atomic_inc(&ct->ct_general.use);
> 
> Should be using nf_conntrack_get().

OK

> Also the patch adds newlines excessively, to a file already
> containing about 20% empty lines.

OK, I'll fix those.

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