Skip to contentSkip to conversation

deliver events for conntracks created/updated via ctnetlink

PATCH Applied Pablo Neira Ayuso 2 participants 4 messages May 20, 2008 – May 31, 2008

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 4/4 deliver events for conntracks created/updated 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-cli while running an instance of conntrackde unsynchronizes the cache of conntrackd with the kernel conntrack table. Note that deletions do not suffer from this problem.

Signed-off-by Pablo Neira Ayuso <[email protected]>
Signature · 1 line
"Los honestos son inadaptados sociales" -- Les Luthiers

[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-cli while running an instance of conntrackde unsynchronizes the cache of conntrackd with the kernel conntrack table. Note that deletions do not suffer from this problem.

Signed-off-by Pablo Neira Ayuso <[email protected]>
net/netfilter/nf_conntrack_netlink.c +80 -30
Index: net-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -529,6 +529,11 @@ nla_put_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier = {
+	.notifier_call	= ctnetlink_conntrack_event,
+};
+
 #endif /* CONFIG_NF_CONNTRACK_EVENTS */
 
 static int ctnetlink_done(struct netlink_callback *cb)

@@ -894,7 +899,7 @@ out:
 }
 
 static int
-ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	unsigned long d;
 	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));

@@ -941,12 +946,15 @@ 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);
+
+	*events |= IPCT_STATUS;
+
 	return 0;
 }
 
 
 static inline int
-ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	struct nf_conntrack_helper *helper;
 	struct nf_conn_help *help = nfct_help(ct);

@@ -990,11 +998,13 @@ ctnetlink_change_helper(struct nf_conn *
 
 	rcu_assign_pointer(help->helper, helper);
 
+	*events |= IPCT_HELPER;
+
 	return 0;
 }
 
 static inline int
-ctnetlink_change_timeout(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_timeout(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 

@@ -1004,11 +1014,15 @@ ctnetlink_change_timeout(struct nf_conn 
 	ct->timeout.expires = jiffies + timeout * HZ;
 	add_timer(&ct->timeout);
 
+	*events |= IPCT_REFRESH;
+
 	return 0;
 }
 
 static inline int
-ctnetlink_change_protoinfo(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_protoinfo(struct nf_conn *ct,
+			   struct nlattr *cda[],
+			   int *events)
 {
 	struct nlattr *tb[CTA_PROTOINFO_MAX+1], *attr = cda[CTA_PROTOINFO];
 	struct nf_conntrack_l4proto *l4proto;

@@ -1020,8 +1034,12 @@ ctnetlink_change_protoinfo(struct nf_con
 
 	l4proto = nf_ct_l4proto_find_get(l3num, npt);
 
-	if (l4proto->from_nlattr)
+	if (l4proto->from_nlattr) {
 		err = l4proto->from_nlattr(tb, ct);
+		if (err == 0)
+			*events |= IPCT_PROTOINFO;
+			
+	}
 	nf_ct_l4proto_put(l4proto);
 
 	return err;

@@ -1057,7 +1075,9 @@ change_nat_seq_adj(struct nf_nat_seq *na
 }
 
 static int
-ctnetlink_change_nat_seq_adj(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_nat_seq_adj(struct nf_conn *ct,
+			     struct nlattr *cda[],
+			     int *events)
 {
 	int ret = 0;
 	struct nf_conn_nat *nat = nfct_nat(ct);

@@ -1072,6 +1092,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		*events |= IPCT_NATSEQADJ;
 	}
 
 	if (cda[CTA_NAT_SEQ_ADJ_REPLY]) {

@@ -1081,6 +1103,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		*events |= IPCT_NATSEQADJ;
 	}
 
 	return 0;

@@ -1088,47 +1112,53 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 #endif
 
 static int
-ctnetlink_change_conntrack(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_conntrack(struct nf_conn *ct,
+			   struct nlattr *cda[],
+			   unsigned int *events)
 {
 	int err;
 
 	if (cda[CTA_HELP]) {
-		err = ctnetlink_change_helper(ct, cda);
+		err = ctnetlink_change_helper(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_TIMEOUT]) {
-		err = ctnetlink_change_timeout(ct, cda);
+		err = ctnetlink_change_timeout(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+		err = ctnetlink_change_status(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_PROTOINFO]) {
-		err = ctnetlink_change_protoinfo(ct, cda);
+		err = ctnetlink_change_protoinfo(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		*events |= IPCT_MARK;
+	}
 #endif
 
 #if defined(CONFIG_NF_CONNTRACK_SECMARK)
-	if (cda[CTA_SECMARK])
+	if (cda[CTA_SECMARK]) {
 		ct->secmark = ntohl(nla_get_be32(cda[CTA_SECMARK]));
+		*events |= IPCT_SECMARK;
+	}
 #endif
 
 #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);
+		err = ctnetlink_change_nat_seq_adj(ct, cda, events);
 		if (err < 0)
 			return err;
 	}

@@ -1147,6 +1177,7 @@ ctnetlink_create_conntrack(struct nlattr
 	int err = -EINVAL;
 	struct nf_conn_help *help;
 	struct nf_conntrack_helper *helper;
+	unsigned long events = IPCT_REFRESH;
 
 	ct = nf_conntrack_alloc(otuple, rtuple);
 	if (ct == NULL || IS_ERR(ct))

@@ -1160,20 +1191,22 @@ ctnetlink_create_conntrack(struct nlattr
 	ct->status |= IPS_CONFIRMED;
 
 	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+		err = ctnetlink_change_status(ct, cda, &events);
 		if (err < 0)
 			goto err;
 	}
 
 	if (cda[CTA_PROTOINFO]) {
-		err = ctnetlink_change_protoinfo(ct, cda);
+		err = ctnetlink_change_protoinfo(ct, cda, &events);
 		if (err < 0)
 			goto err;
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		events |= IPCT_MARK;
+	}
 #endif
 
 	rcu_read_lock();

@@ -1187,18 +1220,28 @@ ctnetlink_create_conntrack(struct nlattr
 		}
 		/* not in hash table yet so not strictly necessary */
 		rcu_assign_pointer(help->helper, helper);
+
+		events |= IPCT_HELPER;
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
 		__set_bit(IPS_EXPECTED_BIT, &ct->status);
 		ct->master = master_ct;
-	}
+		events |= IPCT_RELATED;
+	} else
+		events |= IPCT_NEW;
+
+	atomic_inc(&ct->ct_general.use);
 
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
 	rcu_read_unlock();
 
+	ctnetlink_conntrack_event(&ctnl_notifier, events, ct);
+
+	nf_ct_put(ct);
+
 	return 0;
 
 err:

@@ -1274,6 +1317,9 @@ ctnetlink_new_conntrack(struct sock *ctn
 	 * so there's no need to increase the refcount */
 	err = -EEXIST;
 	if (!(nlh->nlmsg_flags & NLM_F_EXCL)) {
+		unsigned long events = 0;
+		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;

@@ -1284,8 +1330,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, &events);
+
+		spin_unlock_bh(&nf_conntrack_lock);
+
+		if (err == 0) {
+			atomic_inc(&ct->ct_general.use);
+			ctnetlink_conntrack_event(&ctnl_notifier, &events, ct);
+			nf_ct_put(ct);
+		}
+
+		return err;
 	}
 
 out_unlock:

@@ -1464,7 +1519,12 @@ nla_put_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier_exp = {
+	.notifier_call	= ctnetlink_expect_event,
+};
 #endif
+
 static int ctnetlink_exp_done(struct netlink_callback *cb)
 {
 	if (cb->args[1])

@@ -1759,16 +1819,6 @@ ctnetlink_new_expect(struct sock *ctnl, 
 	return err;
 }
 
-#ifdef CONFIG_NF_CONNTRACK_EVENTS
-static struct notifier_block ctnl_notifier = {
-	.notifier_call	= ctnetlink_conntrack_event,
-};
-
-static struct notifier_block ctnl_notifier_exp = {
-	.notifier_call	= ctnetlink_expect_event,
-};
-#endif
-
 static const struct nfnl_callback ctnl_cb[IPCTNL_MSG_MAX] = {
 	[IPCTNL_MSG_CT_NEW]		= { .call = ctnetlink_new_conntrack,
 					    .attr_count = CTA_MAX,
author maintainer
#1 lore raw
Quoted 9 lines · Pablo Neira Ayuso

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-cli while running an instance of conntrackde unsynchronizes the cache of conntrackd with the kernel conntrack table. Note that deletions do not suffer from this problem.

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

Same problem, the previous patch rejects one chunk if you try to apply it to current davem's tree. The one attached should be fine.

Signature · 1 line
"Los honestos son inadaptados sociales" -- Les Luthiers

[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 <[email protected]>
net/netfilter/nf_conntrack_netlink.c +79 -30
Index: net-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -529,6 +529,11 @@ nla_put_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier = {
+	.notifier_call	= ctnetlink_conntrack_event,
+};
+
 #endif /* CONFIG_NF_CONNTRACK_EVENTS */
 
 static int ctnetlink_done(struct netlink_callback *cb)

@@ -883,7 +888,7 @@ out:
 }
 
 static int
-ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	unsigned long d;
 	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));

@@ -930,12 +935,15 @@ 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);
+
+	*events |= IPCT_STATUS;
+
 	return 0;
 }
 
 
 static inline int
-ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	struct nf_conntrack_helper *helper;
 	struct nf_conn_help *help = nfct_help(ct);

@@ -979,11 +987,13 @@ ctnetlink_change_helper(struct nf_conn *
 
 	rcu_assign_pointer(help->helper, helper);
 
+	*events |= IPCT_HELPER;
+
 	return 0;
 }
 
 static inline int
-ctnetlink_change_timeout(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_timeout(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 

@@ -993,11 +1003,15 @@ ctnetlink_change_timeout(struct nf_conn 
 	ct->timeout.expires = jiffies + timeout * HZ;
 	add_timer(&ct->timeout);
 
+	*events |= IPCT_REFRESH;
+
 	return 0;
 }
 
 static inline int
-ctnetlink_change_protoinfo(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_protoinfo(struct nf_conn *ct,
+			   struct nlattr *cda[],
+			   int *events)
 {
 	struct nlattr *tb[CTA_PROTOINFO_MAX+1], *attr = cda[CTA_PROTOINFO];
 	struct nf_conntrack_l4proto *l4proto;

@@ -1006,8 +1020,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)
+			*events |= IPCT_PROTOINFO;
+	}
 	nf_ct_l4proto_put(l4proto);
 
 	return err;

@@ -1043,7 +1060,9 @@ change_nat_seq_adj(struct nf_nat_seq *na
 }
 
 static int
-ctnetlink_change_nat_seq_adj(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_nat_seq_adj(struct nf_conn *ct,
+			     struct nlattr *cda[],
+			     int *events)
 {
 	int ret = 0;
 	struct nf_conn_nat *nat = nfct_nat(ct);

@@ -1058,6 +1077,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		*events |= IPCT_NATSEQADJ;
 	}
 
 	if (cda[CTA_NAT_SEQ_ADJ_REPLY]) {

@@ -1067,6 +1088,8 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 			return ret;
 
 		ct->status |= IPS_SEQ_ADJUST;
+
+		*events |= IPCT_NATSEQADJ;
 	}
 
 	return 0;

@@ -1074,47 +1097,53 @@ ctnetlink_change_nat_seq_adj(struct nf_c
 #endif
 
 static int
-ctnetlink_change_conntrack(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_conntrack(struct nf_conn *ct,
+			   struct nlattr *cda[],
+			   unsigned int *events)
 {
 	int err;
 
 	if (cda[CTA_HELP]) {
-		err = ctnetlink_change_helper(ct, cda);
+		err = ctnetlink_change_helper(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_TIMEOUT]) {
-		err = ctnetlink_change_timeout(ct, cda);
+		err = ctnetlink_change_timeout(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+		err = ctnetlink_change_status(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 	if (cda[CTA_PROTOINFO]) {
-		err = ctnetlink_change_protoinfo(ct, cda);
+		err = ctnetlink_change_protoinfo(ct, cda, events);
 		if (err < 0)
 			return err;
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		*events |= IPCT_MARK;
+	}
 #endif
 
 #if defined(CONFIG_NF_CONNTRACK_SECMARK)
-	if (cda[CTA_SECMARK])
+	if (cda[CTA_SECMARK]) {
 		ct->secmark = ntohl(nla_get_be32(cda[CTA_SECMARK]));
+		*events |= IPCT_SECMARK;
+	}
 #endif
 
 #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);
+		err = ctnetlink_change_nat_seq_adj(ct, cda, events);
 		if (err < 0)
 			return err;
 	}

@@ -1133,6 +1162,7 @@ ctnetlink_create_conntrack(struct nlattr
 	int err = -EINVAL;
 	struct nf_conn_help *help;
 	struct nf_conntrack_helper *helper;
+	unsigned long events = IPCT_REFRESH;
 
 	ct = nf_conntrack_alloc(otuple, rtuple);
 	if (ct == NULL || IS_ERR(ct))

@@ -1146,20 +1176,22 @@ ctnetlink_create_conntrack(struct nlattr
 	ct->status |= IPS_CONFIRMED;
 
 	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+		err = ctnetlink_change_status(ct, cda, &events);
 		if (err < 0)
 			goto err;
 	}
 
 	if (cda[CTA_PROTOINFO]) {
-		err = ctnetlink_change_protoinfo(ct, cda);
+		err = ctnetlink_change_protoinfo(ct, cda, &events);
 		if (err < 0)
 			goto err;
 	}
 
 #if defined(CONFIG_NF_CONNTRACK_MARK)
-	if (cda[CTA_MARK])
+	if (cda[CTA_MARK]) {
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
+		events |= IPCT_MARK;
+	}
 #endif
 
 	rcu_read_lock();

@@ -1173,18 +1205,28 @@ ctnetlink_create_conntrack(struct nlattr
 		}
 		/* not in hash table yet so not strictly necessary */
 		rcu_assign_pointer(help->helper, helper);
+
+		events |= IPCT_HELPER;
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
 		__set_bit(IPS_EXPECTED_BIT, &ct->status);
 		ct->master = master_ct;
-	}
+		events |= IPCT_RELATED;
+	} else
+		events |= IPCT_NEW;
+
+	atomic_inc(&ct->ct_general.use);
 
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
 	rcu_read_unlock();
 
+	ctnetlink_conntrack_event(&ctnl_notifier, events, ct);
+
+	nf_ct_put(ct);
+
 	return 0;
 
 err:

@@ -1260,6 +1302,9 @@ ctnetlink_new_conntrack(struct sock *ctn
 	 * so there's no need to increase the refcount */
 	err = -EEXIST;
 	if (!(nlh->nlmsg_flags & NLM_F_EXCL)) {
+		unsigned long events = 0;
+		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;

@@ -1270,8 +1315,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, &events);
+
+		spin_unlock_bh(&nf_conntrack_lock);
+
+		if (err == 0) {
+			atomic_inc(&ct->ct_general.use);
+			ctnetlink_conntrack_event(&ctnl_notifier, &events, ct);
+			nf_ct_put(ct);
+		}
+
+		return err;
 	}
 
 out_unlock:

@@ -1450,7 +1504,12 @@ nla_put_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier_exp = {
+	.notifier_call	= ctnetlink_expect_event,
+};
 #endif
+
 static int ctnetlink_exp_done(struct netlink_callback *cb)
 {
 	if (cb->args[1])

@@ -1745,16 +1804,6 @@ ctnetlink_new_expect(struct sock *ctnl, 
 	return err;
 }
 
-#ifdef CONFIG_NF_CONNTRACK_EVENTS
-static struct notifier_block ctnl_notifier = {
-	.notifier_call	= ctnetlink_conntrack_event,
-};
-
-static struct notifier_block ctnl_notifier_exp = {
-	.notifier_call	= ctnetlink_expect_event,
-};
-#endif
-
 static const struct nfnl_callback ctnl_cb[IPCTNL_MSG_MAX] = {
 	[IPCTNL_MSG_CT_NEW]		= { .call = ctnetlink_new_conntrack,
 					    .attr_count = CTA_MAX,
#2 lore raw
Quoted 9 lines · Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso

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-cli while running an instance of conntrackde unsynchronizes the cache of conntrackd with the kernel conntrack table. Note that deletions do not suffer from this problem.

Thanks for working on this. I have a couple of comments/questions about the patch:

net/netfilter/nf_conntrack_netlink.c nla_put_failure: · line 535

Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +4 -0
@@ -529,6 +529,11 @@ nla_put_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier = {
+	.notifier_call	= ctnetlink_conntrack_event,
+};

Why are the notifier blocks moved? It doesn't look necessary for this patch.

net/netfilter/nf_conntrack_netlink.c ctnetlink_change_status(struct nf_conn * · line 939

Quoted 12 lines
net/netfilter/nf_conntrack_netlink.c +3 -1
 static int
-ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[], int *events)
 {
 	unsigned long d;
 	unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));

@@ -930,12 +935,15 @@ 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);
+
+	*events |= IPCT_STATUS;

I have some doubts whether we should really do this by keeping track of the events outside of the cache.

Some changes are not performed manually, but by calling NAT and conntrack functions, like nf_nat_setup_info(). These might cache events for the conntrack, which will cause the events to be delivered again some (unknown) time later. For now things look fine, but for example nf_nat_setup_info() should really cache the NAT event instead of nf_conntrack_confirm(), at which point the above would happen.

This makes me think that ctnetlink should simply use the normal event caching functions and trigger delivery once its done.

net/netfilter/nf_conntrack_netlink.c ctnetlink_new_conntrack(struct sock *ctn · line 1326

Quoted 7 lines
net/netfilter/nf_conntrack_netlink.c +7 -0
+		spin_unlock_bh(&nf_conntrack_lock);
+
+		if (err == 0) {
+			atomic_inc(&ct->ct_general.use);
+			ctnetlink_conntrack_event(&ctnl_notifier, &events, ct);
+			nf_ct_put(ct);
+		}

This doesn't look right. The conntrack might already be gone, no?

One final thing: event messages generated after changes requested by userspace should include the pid of the requesting socket in nlmsg_pid. If a unicast report is requested, the socket should be excluded from receiving the multicast message again.

Check out nlmsg_notify() and nlmsg_report() for reference.

author maintainer
#3 lore raw

Hi Patrick,

Sorry for the delay, I couldn't manage to reply before.

Quoted 14 lines · Patrick McHardy
Quoted 9 lines · Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso

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-cli while running an instance of conntrackde unsynchronizes the cache of conntrackd with the kernel conntrack table. Note that deletions do not suffer from this problem.

Thanks for working on this. I have a couple of comments/questions about the patch:

Quoted 11 lines

net/netfilter/nf_conntrack_netlink.c nla_put_failure: · line 535

Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +4 -0
@@ -529,6 +529,11 @@ nla_put_failure:
     kfree_skb(skb);
     return NOTIFY_DONE;
 }
+
+static struct notifier_block ctnl_notifier = {
+    .notifier_call    = ctnetlink_conntrack_event,
+};

Why are the notifier blocks moved? It doesn't look necessary for this patch.

I moved it there because of it is the first parameter of ctnetlink_conntrack_event() in ctnetlink_create_conntrack() and ctnetlink_new_conntrack(). However, we can just pass NULL as it is not used.

Quoted 17 lines

net/netfilter/nf_conntrack_netlink.c ctnetlink_change_status(struct nf_conn * · line 939

Quoted 13 lines
net/netfilter/nf_conntrack_netlink.c +3 -1
 static int
-ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[])
+ctnetlink_change_status(struct nf_conn *ct, struct nlattr *cda[], int
 *events)
 {
     unsigned long d;
     unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));

@@ -930,12 +935,15 @@ 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);
+
+    *events |= IPCT_STATUS;

I have some doubts whether we should really do this by keeping track of the events outside of the cache.

Quoted 7 lines

Some changes are not performed manually, but by calling NAT and conntrack functions, like nf_nat_setup_info(). These might cache events for the conntrack, which will cause the events to be delivered again some (unknown) time later. For now things look fine, but for example nf_nat_setup_info() should really cache the NAT event instead of nf_conntrack_confirm(), at which point the above would happen.

I forgot about the internal call of event_cache inside nf_setup_info(). Well, my intention was to avoid doubled event notification that may arise if an interruption preempts an user process walking in the ctnetlink bits. However, as for now, the double notification may also occur if a local process sends a packet that triggers a conntrack event.

Quoted 3 lines

This makes me think that ctnetlink should simply use the normal event caching functions and trigger delivery once its done.

I agree, thinking it well, the double notification that I was trying to avoid seems unlikely to happen.

Quoted 10 lines

net/netfilter/nf_conntrack_netlink.c ctnetlink_new_conntrack(struct sock *ctn · line 1326

Quoted 7 lines
net/netfilter/nf_conntrack_netlink.c +7 -0
+        spin_unlock_bh(&nf_conntrack_lock);
+
+        if (err == 0) {
+            atomic_inc(&ct->ct_general.use);
+            ctnetlink_conntrack_event(&ctnl_notifier, &events, ct);
+            nf_ct_put(ct);
+        }

This doesn't look right. The conntrack might already be gone, no?

Indeed. I'll fix it.

Quoted 6 lines

One final thing: event messages generated after changes requested by userspace should include the pid of the requesting socket in nlmsg_pid. If a unicast report is requested, the socket should be excluded from receiving the multicast message again.

Check out nlmsg_notify() and nlmsg_report() for reference.

I'll have a look at those. I'll send you another patch this week.

Signature · 1 line
"Los honestos son inadaptados sociales" -- Les Luthiers