use nf_conntrack_get instead of atomic_inc

PATCH Applied Pablo Neira Ayuso 4 participants 22 messages Nov 17, 2008 – Nov 19, 2008

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2008-11-17 08:39 UTC #0 lore raw

PATCH 1/8 use nf_conntrack_get instead of atomic_inc

Use nf_conntrack_get instead of the direct call to atomic_inc.

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 a040d46..80a3f13 100644
@@ -1209,7 +1209,7 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 				goto out_unlock;
 			}
 			master_ct = nf_ct_tuplehash_to_ctrack(master_h);
-			atomic_inc(&master_ct->ct_general.use);
+			nf_conntrack_get(&master_ct->ct_general);
 		}
 
 		spin_unlock_bh(&nf_conntrack_lock);

Pablo Neira Ayuso

author maintainer

2008-11-17 08:39 UTC #1 lore raw

PATCH 2/8 use EOPNOTSUPP instead of EINVAL if the conntrack has no helper

This patch changes the return value if the conntrack has no helper assigned. Instead of EINVAL, which is reserved for malformed messages, it returns EOPNOTSUPP.

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 80a3f13..d87a939 100644
@@ -1652,7 +1652,7 @@ ctnetlink_create_expect(struct nlattr *cda[], u_int8_t u3)
 
 	if (!help || !help->helper) {
 		/* such conntrack hasn't got any helper, abort */
-		err = -EINVAL;
+		err = -EOPNOTSUPP;
 		goto out;
 	}

Patrick McHardy

Applied

2008-11-17 14:56 UTC #2 lore raw

Re: [PATCH 2/8] [PATCH] use EOPNOTSUPP instead of EINVAL if the conntrack has no helper

Quoted 3 lines · Pablo Neira Ayuso

This patch changes the return value if the conntrack has no helper assigned. Instead of EINVAL, which is reserved for malformed messages, it returns EOPNOTSUPP.

Applied, thanks.

Pablo Neira Ayuso

author maintainer

2008-11-17 08:40 UTC #3 lore raw

PATCH 3/8 get rid of module refcounting in ctnetlink

In reply to Pablo Neira Ayuso

This patch replaces the unnecessary module refcounting with the read-side locks. With this patch, all the dump and fill_info function are called under the RCU read lock.

Based on a patch from Fabian Hugelshofer.

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

---

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

net/netfilter/nf_conntrack_netlink.c +18 ~~-20~~

index d87a939..49a04fa 100644
@@ -105,16 +105,14 @@ ctnetlink_dump_tuples(struct sk_buff *skb,
 	struct nf_conntrack_l3proto *l3proto;
 	struct nf_conntrack_l4proto *l4proto;
 
-	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	l3proto = __nf_ct_l3proto_find(tuple->src.l3num);
 	ret = ctnetlink_dump_tuples_ip(skb, tuple, l3proto);
-	nf_ct_l3proto_put(l3proto);
 
 	if (unlikely(ret < 0))
 		return ret;
 
-	l4proto = nf_ct_l4proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	l4proto = __nf_ct_l4proto_find(tuple->src.l3num, tuple->dst.protonum);
 	ret = ctnetlink_dump_tuples_proto(skb, tuple, l4proto);
-	nf_ct_l4proto_put(l4proto);
 
 	return ret;
 }

@@ -151,11 +149,9 @@ ctnetlink_dump_protoinfo(struct sk_buff *skb, const struct nf_conn *ct)
 	struct nlattr *nest_proto;
 	int ret;
 
-	l4proto = nf_ct_l4proto_find_get(nf_ct_l3num(ct), nf_ct_protonum(ct));
-	if (!l4proto->to_nlattr) {
-		nf_ct_l4proto_put(l4proto);
+	l4proto = __nf_ct_l4proto_find(nf_ct_l3num(ct), nf_ct_protonum(ct));
+	if (!l4proto->to_nlattr)
 		return 0;
-	}
 
 	nest_proto = nla_nest_start(skb, CTA_PROTOINFO | NLA_F_NESTED);
 	if (!nest_proto)

@@ -163,14 +159,11 @@ ctnetlink_dump_protoinfo(struct sk_buff *skb, const struct nf_conn *ct)
 
 	ret = l4proto->to_nlattr(skb, nest_proto, ct);
 
-	nf_ct_l4proto_put(l4proto);
-
 	nla_nest_end(skb, nest_proto);
 
 	return ret;
 
 nla_put_failure:
-	nf_ct_l4proto_put(l4proto);
 	return -1;
 }

@@ -184,7 +177,6 @@ ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct)
 	if (!help)
 		return 0;
 
-	rcu_read_lock();
 	helper = rcu_dereference(help->helper);
 	if (!helper)
 		goto out;

@@ -199,11 +191,9 @@ ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct)
 
 	nla_nest_end(skb, nest_helper);
 out:
-	rcu_read_unlock();
 	return 0;
 
 nla_put_failure:
-	rcu_read_unlock();
 	return -1;
 }

@@ -461,6 +451,7 @@ static int ctnetlink_conntrack_event(struct notifier_block *this,
 	nfmsg->version	= NFNETLINK_V0;
 	nfmsg->res_id	= 0;
 
+	rcu_read_lock();
 	nest_parms = nla_nest_start(skb, CTA_TUPLE_ORIG | NLA_F_NESTED);
 	if (!nest_parms)
 		goto nla_put_failure;

@@ -517,13 +508,15 @@ static int ctnetlink_conntrack_event(struct notifier_block *this,
 	    && ctnetlink_dump_mark(skb, ct) < 0)
 		goto nla_put_failure;
 #endif
+	rcu_read_unlock();
 
 	nlh->nlmsg_len = skb->tail - b;
 	nfnetlink_send(skb, 0, group, 0);
 	return NOTIFY_DONE;
 
-nlmsg_failure:
 nla_put_failure:
+	rcu_read_unlock();
+nlmsg_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }

@@ -795,8 +788,10 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
 		return -ENOMEM;
 	}
 
+	rcu_read_lock();
 	err = ctnetlink_fill_info(skb2, NETLINK_CB(skb).pid, nlh->nlmsg_seq,
 				  IPCTNL_MSG_CT_NEW, 1, ct);
+	rcu_read_unlock();
 	nf_ct_put(ct);
 	if (err <= 0)
 		goto free;

@@ -1292,16 +1287,14 @@ ctnetlink_exp_dump_mask(struct sk_buff *skb,
 	if (!nest_parms)
 		goto nla_put_failure;
 
-	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	l3proto = __nf_ct_l3proto_find(tuple->src.l3num);
 	ret = ctnetlink_dump_tuples_ip(skb, &m, l3proto);
-	nf_ct_l3proto_put(l3proto);
 
 	if (unlikely(ret < 0))
 		goto nla_put_failure;
 
-	l4proto = nf_ct_l4proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	l4proto = __nf_ct_l4proto_find(tuple->src.l3num, tuple->dst.protonum);
 	ret = ctnetlink_dump_tuples_proto(skb, &m, l4proto);
-	nf_ct_l4proto_put(l4proto);
 	if (unlikely(ret < 0))
 		goto nla_put_failure;

@@ -1408,15 +1401,18 @@ static int ctnetlink_expect_event(struct notifier_block *this,
 	nfmsg->version	    = NFNETLINK_V0;
 	nfmsg->res_id	    = 0;
 
+	rcu_read_lock();
 	if (ctnetlink_exp_dump_expect(skb, exp) < 0)
 		goto nla_put_failure;
+	rcu_read_unlock();
 
 	nlh->nlmsg_len = skb->tail - b;
 	nfnetlink_send(skb, 0, NFNLGRP_CONNTRACK_EXP_NEW, 0);
 	return NOTIFY_DONE;
 
-nlmsg_failure:
 nla_put_failure:
+	rcu_read_unlock();
+nlmsg_failure:
 	kfree_skb(skb);
 	return NOTIFY_DONE;
 }

@@ -1520,9 +1516,11 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
 	if (!skb2)
 		goto out;
 
+	rcu_read_lock();
 	err = ctnetlink_exp_fill_info(skb2, NETLINK_CB(skb).pid,
 				      nlh->nlmsg_seq, IPCTNL_MSG_EXP_NEW,
 				      1, exp);
+	rcu_read_unlock();
 	if (err <= 0)
 		goto free;

Patrick McHardy

Applied

2008-11-17 15:01 UTC #4 lore raw

Re: [PATCH 3/8] [PATCH] get rid of module refcounting in ctnetlink

Quoted 5 lines · Pablo Neira Ayuso

This patch replaces the unnecessary module refcounting with the read-side locks. With this patch, all the dump and fill_info function are called under the RCU read lock.
Based on a patch from Fabian Hugelshofer.

Applied, thanks.

Pablo Neira Ayuso

author maintainer

2008-11-17 08:40 UTC #5 lore raw

PATCH 4/8 connection tracking helper name persistent aliases

In reply to Pablo Neira Ayuso

This patch adds the macro MODULE_ALIAS_NFCT_HELPER that defines a way to provide generic and persistent aliases for the connection tracking helpers.

This next patch requires this patch.

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

---

 include/net/netfilter/nf_conntrack.h    |    3 +++
 net/netfilter/nf_conntrack_amanda.c     |    1 +
 net/netfilter/nf_conntrack_ftp.c        |    1 +
 net/netfilter/nf_conntrack_h323_main.c  |    1 +
 net/netfilter/nf_conntrack_irc.c        |    1 +
 net/netfilter/nf_conntrack_netbios_ns.c |    1 +
 net/netfilter/nf_conntrack_pptp.c       |    1 +
 net/netfilter/nf_conntrack_sane.c       |    1 +
 net/netfilter/nf_conntrack_sip.c        |    1 +
 net/netfilter/nf_conntrack_tftp.c       |    1 +
 10 files changed, 12 insertions(+), 0 deletions(-)

include/net/netfilter/nf_conntrack.h +3 -0

index b76a868..f11255e 100644
@@ -298,5 +298,8 @@ do {							\
 	local_bh_enable();				\
 } while (0)
 
+#define MODULE_ALIAS_NFCT_HELPER(helper) \
+        MODULE_ALIAS("nfct-helper-" helper)
+
 #endif /* __KERNEL__ */
 #endif /* _NF_CONNTRACK_H */

net/netfilter/nf_conntrack_amanda.c +1 -0

index 38aedee..4f8fcf4 100644
@@ -30,6 +30,7 @@ MODULE_AUTHOR("Brian J. Murrell <[email protected]>");
 MODULE_DESCRIPTION("Amanda connection tracking module");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_amanda");
+MODULE_ALIAS_NFCT_HELPER("amanda");
 
 module_param(master_timeout, uint, 0600);
 MODULE_PARM_DESC(master_timeout, "timeout for the master connection");

net/netfilter/nf_conntrack_ftp.c +1 -0

index 6295009..703a437 100644
@@ -29,6 +29,7 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rusty Russell <[email protected]>");
 MODULE_DESCRIPTION("ftp connection tracking helper");
 MODULE_ALIAS("ip_conntrack_ftp");
+MODULE_ALIAS_NFCT_HELPER("ftp");
 
 /* This is slow, but it's simple. --RR */
 static char *ftp_buffer;

net/netfilter/nf_conntrack_h323_main.c +1 -0

index 99bc803..687bd63 100644
@@ -1827,3 +1827,4 @@ MODULE_AUTHOR("Jing Min Zhao <[email protected]>");
 MODULE_DESCRIPTION("H.323 connection tracking helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_h323");
+MODULE_ALIAS_NFCT_HELPER("h323");

net/netfilter/nf_conntrack_irc.c +1 -0

index 4d681a0..409c8be 100644
@@ -41,6 +41,7 @@ MODULE_AUTHOR("Harald Welte <[email protected]>");
 MODULE_DESCRIPTION("IRC (DCC) connection tracking helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_irc");
+MODULE_ALIAS_NFCT_HELPER("irc");
 
 module_param_array(ports, ushort, &ports_c, 0400);
 MODULE_PARM_DESC(ports, "port numbers of IRC servers");

net/netfilter/nf_conntrack_netbios_ns.c +1 -0

index 08404e6..5af4273 100644
@@ -37,6 +37,7 @@ MODULE_AUTHOR("Patrick McHardy <[email protected]>");
 MODULE_DESCRIPTION("NetBIOS name service broadcast connection tracking helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_netbios_ns");
+MODULE_ALIAS_NFCT_HELPER("netbios_ns");
 
 static unsigned int timeout __read_mostly = 3;
 module_param(timeout, uint, 0400);

net/netfilter/nf_conntrack_pptp.c +1 -0

index 1bc3001..9e169ef 100644
@@ -37,6 +37,7 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Harald Welte <[email protected]>");
 MODULE_DESCRIPTION("Netfilter connection tracking helper module for PPTP");
 MODULE_ALIAS("ip_conntrack_pptp");
+MODULE_ALIAS_NFCT_HELPER("pptp");
 
 static DEFINE_SPINLOCK(nf_pptp_lock);

net/netfilter/nf_conntrack_sane.c +1 -0

index a94294b..dcfecbb 100644
@@ -30,6 +30,7 @@
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Michal Schmidt <[email protected]>");
 MODULE_DESCRIPTION("SANE connection tracking helper");
+MODULE_ALIAS_NFCT_HELPER("sane");
 
 static char *sane_buffer;

net/netfilter/nf_conntrack_sip.c +1 -0

index 6813f1c..4b57216 100644
@@ -28,6 +28,7 @@ MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Christian Hentschel <[email protected]>");
 MODULE_DESCRIPTION("SIP connection tracking helper");
 MODULE_ALIAS("ip_conntrack_sip");
+MODULE_ALIAS_NFCT_HELPER("sip");
 
 #define MAX_PORTS	8
 static unsigned short ports[MAX_PORTS];

net/netfilter/nf_conntrack_tftp.c +1 -0

index f57f6e7..46e646b 100644
@@ -22,6 +22,7 @@ MODULE_AUTHOR("Magnus Boden <[email protected]>");
 MODULE_DESCRIPTION("TFTP connection tracking helper");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_conntrack_tftp");
+MODULE_ALIAS_NFCT_HELPER("tftp");
 
 #define MAX_PORTS 8
 static unsigned short ports[MAX_PORTS];

Patrick McHardy

Applied

2008-11-17 15:02 UTC #6 lore raw

Re: [PATCH 4/8] [PATCH] connection tracking helper name persistent aliases

Quoted 5 lines · Pablo Neira Ayuso

This patch adds the macro MODULE_ALIAS_NFCT_HELPER that defines a way to provide generic and persistent aliases for the connection tracking helpers.
This next patch requires this patch.

Applied, thanks.

Pablo Neira Ayuso

author maintainer

2008-11-17 08:41 UTC #7 lore raw

PATCH 5/8 Helper modules load-on-demand support for ctnetlink

In reply to Pablo Neira Ayuso

This patch adds module loading for helpers via ctnetlink.

* Creation path: We support explicit and implicit helper assignation. For the explicit case, we try to load the module. If the module is correctly loaded and the helper is present, we return EAGAIN to re-start the creation. Otherwise, we return EOPNOTSUPP.
* Update path: release the spin lock, load the module and check. If it is present, then return EAGAIN to re-start the update.

This patch provides a refactorized function to lookup-and-set the connection tracking helper. The function removes the exported symbol __nf_ct_helper_find as it has not clients anymore.

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

---

 include/net/netfilter/nf_conntrack_helper.h |    5 +-
 net/netfilter/nf_conntrack_core.c           |   28 +----------
 net/netfilter/nf_conntrack_helper.c         |   33 ++++++++++++-
 net/netfilter/nf_conntrack_netlink.c        |   70 ++++++++++++++++++++++++---
 4 files changed, 96 insertions(+), 40 deletions(-)

include/net/netfilter/nf_conntrack_helper.h +2 -3

index f8060ab..10ba2e9 100644
@@ -39,9 +39,6 @@ struct nf_conntrack_helper
 };
 
 extern struct nf_conntrack_helper *
-__nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
-
-extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);

@@ -49,6 +46,8 @@ extern void nf_conntrack_helper_unregister(struct nf_conntrack_helper *);
 
 extern struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 
+extern int __nf_ct_assign_helper(struct nf_conn *ct, gfp_t flags);
+
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_HELPER);

net/netfilter/nf_conntrack_core.c +2 ~~-26~~

index 622d7c6..d6af04e 100644
@@ -588,14 +588,7 @@ init_conntrack(struct net *net,
 		nf_conntrack_get(&ct->master->ct_general);
 		NF_CT_STAT_INC(net, expect_new);
 	} else {
-		struct nf_conntrack_helper *helper;
-
-		helper = __nf_ct_helper_find(&repl_tuple);
-		if (helper) {
-			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-			if (help)
-				rcu_assign_pointer(help->helper, helper);
-		}
+		__nf_ct_assign_helper(ct, GFP_ATOMIC);
 		NF_CT_STAT_INC(net, new);
 	}

@@ -772,7 +765,6 @@ void nf_conntrack_alter_reply(struct nf_conn *ct,
 			      const struct nf_conntrack_tuple *newreply)
 {
 	struct nf_conn_help *help = nfct_help(ct);
-	struct nf_conntrack_helper *helper;
 
 	/* Should be unconfirmed, so not in hash table yet */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));

@@ -785,23 +777,7 @@ void nf_conntrack_alter_reply(struct nf_conn *ct,
 		return;
 
 	rcu_read_lock();
-	helper = __nf_ct_helper_find(newreply);
-	if (helper == NULL) {
-		if (help)
-			rcu_assign_pointer(help->helper, NULL);
-		goto out;
-	}
-
-	if (help == NULL) {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			goto out;
-	} else {
-		memset(&help->help, 0, sizeof(help->help));
-	}
-
-	rcu_assign_pointer(help->helper, helper);
-out:
+	__nf_ct_assign_helper(ct, GFP_ATOMIC);
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);

net/netfilter/nf_conntrack_helper.c +31 -2

index 9c06b9f..77f4b15 100644
@@ -44,7 +44,7 @@ static unsigned int helper_hash(const struct nf_conntrack_tuple *tuple)
 		(__force __u16)tuple->src.u.all) % nf_ct_helper_hsize;
 }
 
-struct nf_conntrack_helper *
+static struct nf_conntrack_helper *
 __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;

@@ -62,7 +62,6 @@ __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 	}
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(__nf_ct_helper_find);
 
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name)

@@ -94,6 +93,36 @@ struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+int __nf_ct_assign_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_assign_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {

net/netfilter/nf_conntrack_netlink.c +61 -9

92 lines not on this page · raw message

Alexey Dobriyan

2008-11-17 08:52 UTC #8 lore raw

Re: [PATCH 5/8] [PATCH] Helper modules load-on-demand support for ctnetlink

net/netfilter/nf_conntrack_netlink.c ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[]) · line 921

Quoted 9 lines · Pablo Neira Ayuso

net/netfilter/nf_conntrack_netlink.c +2 -1

@@ -917,8 +917,22 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
 	}
 
 	helper = __nf_conntrack_helper_find_byname(helpname);
-	if (helper == NULL)
+	if (helper == NULL) {
+#ifdef CONFIG_KMOD

CONFIG_KMOD is on the way to always on, no need for ifdef.

Patrick McHardy

2008-11-17 15:24 UTC #9 lore raw

Re: [PATCH 5/8] [PATCH] Helper modules load-on-demand support for ctnetlink

In reply to Pablo Neira Ayuso

Quoted 18 lines · Pablo Neira Ayuso

This patch adds module loading for helpers via ctnetlink.
* Creation path: We support explicit and implicit helper assignation. For the explicit case, we try to load the module. If the module is correctly loaded and the helper is present, we return EAGAIN to re-start the creation. Otherwise, we return EOPNOTSUPP.
* Update path: release the spin lock, load the module and check. If it is present, then return EAGAIN to re-start the update.
This patch provides a refactorized function to lookup-and-set the connection tracking helper. The function removes the exported symbol __nf_ct_helper_find as it has not clients anymore.
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
net/netfilter/nf_conntrack_helper.c +2 -0
+int __nf_ct_assign_helper(struct nf_conn *ct, gfp_t flags)
+{

net/netfilter/nf_conntrack_helper.c struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp) · line 107

Quoted 10 lines

net/netfilter/nf_conntrack_helper.c +10 -0

+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;

Its a bit confusing to change the entry, but still return an error. ctnetlink_create_conntrack() explicitly checks for ENOMEM and ignores other errors, other callers ignore them completely. This is risky because people changing that function will probably expect the caller to handle any kind of error.

Since failure to find a helper is not really an error, I think it would be better to simply return 0 in that case and have ctnetlink_create_conntrack() check for < 0. Even better would be to reflect in the function name that it only tries to find a matching helper. I don't have a good suggestion though, maybe try_assign_helper or lookup_helper.

Quoted 26 lines

net/netfilter/nf_conntrack_helper.c +18 -0

+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_assign_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {

net/netfilter/nf_conntrack_netlink.c +0 -0

index 49a04fa..7af7a86 100644

net/netfilter/nf_conntrack_netlink.c ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[]) · line 921

Quoted 6 lines

net/netfilter/nf_conntrack_netlink.c +2 -1

@@ -917,8 +917,22 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nlattr *cda[])
 	}
 
 	helper = __nf_conntrack_helper_find_byname(helpname);
-	if (helper == NULL)
+	if (helper == NULL) {
+#ifdef CONFIG_KMOD

As Alexey pointed out, CONFIG_KMOD should not be used here, but CONFIG_MODULES.

Pablo Neira Ayuso

author maintainer

2008-11-17 08:41 UTC #10 lore raw

PATCH 6/8 deliver events for conntracks created via ctnetlink

In reply to 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 command line tool and conntrackd at the same time can trigger unconsistencies.

This patch also modifies the event cache infrastructure to pass the process PID and the ECHO flag to nfnetlink_send() to report back to userspace if the process that triggered the change needs so. Based on a suggestion from Patrick McHardy.

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

---

 include/net/netfilter/nf_conntrack.h        |    2 -
 include/net/netfilter/nf_conntrack_ecache.h |   57 ++++++++++++++++-
 include/net/netfilter/nf_conntrack_expect.h |    2 +
 net/netfilter/nf_conntrack_core.c           |   25 ++++++--
 net/netfilter/nf_conntrack_ecache.c         |   14 +++-
 net/netfilter/nf_conntrack_expect.c         |   43 +++++++++++--
 net/netfilter/nf_conntrack_netlink.c        |   88 ++++++++++++++++++++++-----
 7 files changed, 197 insertions(+), 34 deletions(-)

include/net/netfilter/nf_conntrack.h +1 -1

index f11255e..2e0c536 100644
@@ -199,7 +199,7 @@ __nf_conntrack_find(struct net *net, const struct nf_conntrack_tuple *tuple);
 
 extern void nf_conntrack_hash_insert(struct nf_conn *ct);
 
-extern void nf_conntrack_flush(struct net *net);
+extern void nf_conntrack_flush(struct net *net, u32 pid, int report);
 
 extern bool nf_ct_get_tuplepr(const struct sk_buff *skb,
 			      unsigned int nhoff, u_int16_t l3num,

include/net/netfilter/nf_conntrack_ecache.h +53 -4

90 lines not on this page · raw message

include/net/netfilter/nf_conntrack_expect.h +2 -0

index 37a7fc1..ab17a15 100644
@@ -100,6 +100,8 @@ void nf_ct_expect_init(struct nf_conntrack_expect *, unsigned int, u_int8_t,
 		       u_int8_t, const __be16 *, const __be16 *);
 void nf_ct_expect_put(struct nf_conntrack_expect *exp);
 int nf_ct_expect_related(struct nf_conntrack_expect *expect);
+int nf_ct_expect_related_report(struct nf_conntrack_expect *expect, 
+				u32 pid, int report);
 
 #endif /*_NF_CONNTRACK_EXPECT_H*/

net/netfilter/nf_conntrack_core.c +21 -4

index d6af04e..63afdf2 100644
@@ -181,7 +181,8 @@ destroy_conntrack(struct nf_conntrack *nfct)
 	NF_CT_ASSERT(atomic_read(&nfct->use) == 0);
 	NF_CT_ASSERT(!timer_pending(&ct->timeout));
 
-	nf_conntrack_event(IPCT_DESTROY, ct);
+	if (!test_bit(IPS_DYING_BIT, &ct->status))
+		nf_conntrack_event(IPCT_DESTROY, ct);
 	set_bit(IPS_DYING_BIT, &ct->status);
 
 	/* To make sure we don't get any weird locking issues here:

@@ -972,8 +973,20 @@ void nf_ct_iterate_cleanup(struct net *net,
 }
 EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup);
 
+struct __nf_ct_flush_report {
+	u32 pid;
+	int report;
+};
+
 static int kill_all(struct nf_conn *i, void *data)
 {
+	struct __nf_ct_flush_report *fr = (struct __nf_ct_flush_report *)data;
+
+	/* get_next_corpse sets the dying bit for us */
+	nf_conntrack_event_report(IPCT_DESTROY,
+				  i,
+				  fr->pid,
+				  fr->report);
 	return 1;
 }

@@ -987,9 +1000,13 @@ void nf_ct_free_hashtable(struct hlist_head *hash, int vmalloced, unsigned int s
 }
 EXPORT_SYMBOL_GPL(nf_ct_free_hashtable);
 
-void nf_conntrack_flush(struct net *net)
+void nf_conntrack_flush(struct net *net, u32 pid, int report)
 {
-	nf_ct_iterate_cleanup(net, kill_all, NULL);
+	struct __nf_ct_flush_report fr = {
+		.pid 	= pid,
+		.report = report,
+	};
+	nf_ct_iterate_cleanup(net, kill_all, &fr);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_flush);

@@ -1005,7 +1022,7 @@ static void nf_conntrack_cleanup_net(struct net *net)
 	nf_ct_event_cache_flush(net);
 	nf_conntrack_ecache_fini(net);
  i_see_dead_people:
-	nf_conntrack_flush(net);
+	nf_conntrack_flush(net, 0, 0);
 	if (atomic_read(&net->ct.count) != 0) {
 		schedule();
 		goto i_see_dead_people;

net/netfilter/nf_conntrack_ecache.c +11 -3

index a5f5e2e..dee4190 100644
@@ -35,9 +35,17 @@ static inline void
 __nf_ct_deliver_cached_events(struct nf_conntrack_ecache *ecache)
 {
 	if (nf_ct_is_confirmed(ecache->ct) && !nf_ct_is_dying(ecache->ct)
-	    && ecache->events)
-		atomic_notifier_call_chain(&nf_conntrack_chain, ecache->events,
-				    ecache->ct);
+	    && ecache->events) {
+		struct nf_ct_event item = {
+			.ct 	= ecache->ct,
+			.pid	= 0,
+			.report	= 0
+		};
+
+		atomic_notifier_call_chain(&nf_conntrack_chain,
+					   ecache->events,
+					   &item);
+	}
 
 	ecache->events = 0;
 	nf_ct_put(ecache->ct);

net/netfilter/nf_conntrack_expect.c +36 -7

72 lines not on this page · raw message

net/netfilter/nf_conntrack_netlink.c +73 ~~-15~~

191 lines not on this page · raw message

Pablo Neira Ayuso

author maintainer

2008-11-17 08:41 UTC #11 lore raw

PATCH 7/8 dynamic calculation of event message size for ctnetlink

In reply to Pablo Neira Ayuso

This patch adds dynamic message size calculation for ctnetlink. This reduces CPU consumption since the overhead in the message trimming is removed.

Based on a suggestion from Patrick McHardy.

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

---

 include/net/netfilter/nf_conntrack_l3proto.h   |    2 
 include/net/netfilter/nf_conntrack_l4proto.h   |    3 +
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    6 +
 net/ipv4/netfilter/nf_conntrack_proto_icmp.c   |    8 ++
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    6 +
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |    8 ++
 net/netfilter/nf_conntrack_core.c              |    6 +
 net/netfilter/nf_conntrack_netlink.c           |  103 ++++++++++++++++++++++++
 net/netfilter/nf_conntrack_proto_dccp.c        |   10 ++
 net/netfilter/nf_conntrack_proto_gre.c         |    1 
 net/netfilter/nf_conntrack_proto_sctp.c        |   12 +++
 net/netfilter/nf_conntrack_proto_tcp.c         |   14 +++
 net/netfilter/nf_conntrack_proto_udp.c         |    2 
 net/netfilter/nf_conntrack_proto_udplite.c     |    2 
 14 files changed, 182 insertions(+), 1 deletions(-)

include/net/netfilter/nf_conntrack_l3proto.h +2 -0

index 0378676..e0007f9 100644
@@ -55,6 +55,8 @@ struct nf_conntrack_l3proto
 
 	int (*nlattr_to_tuple)(struct nlattr *tb[],
 			       struct nf_conntrack_tuple *t);
+
+	size_t (*nlattr_size)(void);
 	const struct nla_policy *nla_policy;
 
 #ifdef CONFIG_SYSCTL

include/net/netfilter/nf_conntrack_l4proto.h +3 -0

index 7f2f43c..3c80479 100644
@@ -72,6 +72,8 @@ struct nf_conntrack_l4proto
 			       const struct nf_conntrack_tuple *t);
 	int (*nlattr_to_tuple)(struct nlattr *tb[],
 			       struct nf_conntrack_tuple *t);
+	size_t (*nlattr_size)(void);
+	size_t (*nlattr_protoinfo_size)(void);
 	const struct nla_policy *nla_policy;
 
 #ifdef CONFIG_SYSCTL

@@ -115,6 +117,7 @@ extern int nf_ct_port_tuple_to_nlattr(struct sk_buff *skb,
 				      const struct nf_conntrack_tuple *tuple);
 extern int nf_ct_port_nlattr_to_tuple(struct nlattr *tb[],
 				      struct nf_conntrack_tuple *t);
+extern size_t nf_ct_port_nlattr_size(void);
 extern const struct nla_policy nf_ct_port_nla_policy[];
 
 #ifdef CONFIG_SYSCTL

net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +6 -0

index b2141e1..a739d5a 100644
@@ -329,6 +329,11 @@ static int ipv4_nlattr_to_tuple(struct nlattr *tb[],
 
 	return 0;
 }
+
+static size_t ipv4_nlattr_size(void)
+{
+	return nla_total_size(sizeof(u_int32_t))*2;
+}
 #endif
 
 static struct nf_sockopt_ops so_getorigdst = {

@@ -349,6 +354,7 @@ struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv4 __read_mostly = {
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr = ipv4_tuple_to_nlattr,
 	.nlattr_to_tuple = ipv4_nlattr_to_tuple,
+	.nlattr_size	 = ipv4_nlattr_size,
 	.nla_policy	 = ipv4_nla_policy,
 #endif
 #if defined(CONFIG_SYSCTL) && defined(CONFIG_NF_CONNTRACK_PROC_COMPAT)

net/ipv4/netfilter/nf_conntrack_proto_icmp.c +8 -0

index 1fd3ef7..f2648aa 100644
@@ -262,6 +262,13 @@ static int icmp_nlattr_to_tuple(struct nlattr *tb[],
 
 	return 0;
 }
+
+static size_t icmp_nlattr_size(void)
+{
+	return nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int16_t));
+}
 #endif
 
 #ifdef CONFIG_SYSCTL

@@ -310,6 +317,7 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_icmp __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= icmp_tuple_to_nlattr,
 	.nlattr_to_tuple	= icmp_nlattr_to_tuple,
+	.nlattr_size		= icmp_nlattr_size,
 	.nla_policy		= icmp_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c +6 -0

index 727b953..4d3573e 100644
@@ -341,6 +341,11 @@ static int ipv6_nlattr_to_tuple(struct nlattr *tb[],
 
 	return 0;
 }
+
+static size_t ipv6_nlattr_size(void)
+{
+	return nla_total_size(sizeof(u_int32_t)*4)*2;
+}
 #endif
 
 struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6 __read_mostly = {

@@ -353,6 +358,7 @@ struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6 __read_mostly = {
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= ipv6_tuple_to_nlattr,
 	.nlattr_to_tuple	= ipv6_nlattr_to_tuple,
+	.nlattr_size		= ipv6_nlattr_size,
 	.nla_policy		= ipv6_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c +8 -0

index bd52151..acb852a 100644
@@ -243,6 +243,13 @@ static int icmpv6_nlattr_to_tuple(struct nlattr *tb[],
 
 	return 0;
 }
+
+static size_t icmpv6_nlattr_size(void)
+{
+	return nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int16_t));
+}
 #endif
 
 #ifdef CONFIG_SYSCTL

@@ -275,6 +282,7 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_icmpv6 __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= icmpv6_tuple_to_nlattr,
 	.nlattr_to_tuple	= icmpv6_nlattr_to_tuple,
+	.nlattr_size		= icmpv6_nlattr_size,
 	.nla_policy		= icmpv6_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/netfilter/nf_conntrack_core.c +6 -0

index 63afdf2..9a1c206 100644
@@ -904,6 +904,12 @@ int nf_ct_port_nlattr_to_tuple(struct nlattr *tb[],
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_to_tuple);
+
+size_t nf_ct_port_nlattr_size(void)
+{
+	return nla_total_size(sizeof(u_int16_t))*2;
+}
+EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_size);
 #endif
 
 /* Used by ipt_REJECT and ip6t_REJECT. */

net/netfilter/nf_conntrack_netlink.c +102 -1

115 lines not on this page · raw message

net/netfilter/nf_conntrack_proto_dccp.c +10 -0

index 8fcf176..6694173 100644
@@ -657,6 +657,12 @@ static int nlattr_to_dccp(struct nlattr *cda[], struct nf_conn *ct)
 	write_unlock_bh(&dccp_lock);
 	return 0;
 }
+
+static size_t dccp_nlattr_protoinfo_size(void)
+{
+	return nla_total_size(0)*2 +	/* CTA_PROTOINFO */
+	       nla_total_size(sizeof(u_int8_t));
+}
 #endif
 
 #ifdef CONFIG_SYSCTL

@@ -749,6 +755,8 @@ static struct nf_conntrack_l4proto dccp_proto4 __read_mostly = {
 	.from_nlattr		= nlattr_to_dccp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= dccp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

@@ -774,6 +782,8 @@ static struct nf_conntrack_l4proto dccp_proto6 __read_mostly = {
 	.from_nlattr		= nlattr_to_dccp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= dccp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/netfilter/nf_conntrack_proto_gre.c +1 -0

index 4ab62ad..8237cff 100644
@@ -294,6 +294,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_gre4 __read_mostly = {
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr = nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple = nf_ct_port_nlattr_to_tuple,
+	.nlattr_size	 = nf_ct_port_nlattr_size,
 	.nla_policy	 = nf_ct_port_nla_policy,
 #endif
 };

net/netfilter/nf_conntrack_proto_sctp.c +12 -0

index c2bd457..78283c1 100644
@@ -537,6 +537,14 @@ static int nlattr_to_sctp(struct nlattr *cda[], struct nf_conn *ct)
 
 	return 0;
 }
+
+static size_t sctp_nlattr_protoinfo_size(void)
+{
+	return nla_total_size(0)*2 +           /* CTA_PROTOINFO */
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int32_t)) +
+	       nla_total_size(sizeof(u_int32_t));
+}
 #endif
 
 #ifdef CONFIG_SYSCTL

@@ -671,6 +679,8 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp4 __read_mostly = {
 	.from_nlattr		= nlattr_to_sctp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= sctp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

@@ -699,6 +709,8 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_sctp6 __read_mostly = {
 	.from_nlattr		= nlattr_to_sctp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= sctp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/netfilter/nf_conntrack_proto_tcp.c +14 -0

index a1edb9c..36e2876 100644
@@ -1181,6 +1181,16 @@ static int nlattr_to_tcp(struct nlattr *cda[], struct nf_conn *ct)
 
 	return 0;
 }
+
+static size_t tcp_nlattr_protoinfo_size(void)
+{
+	return nla_total_size(0)*2 +	/* CTA_PROTOINFO */
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(u_int8_t)) +
+	       nla_total_size(sizeof(struct nf_ct_tcp_flags)) +
+	       nla_total_size(sizeof(struct nf_ct_tcp_flags));
+}
 #endif
 
 #ifdef CONFIG_SYSCTL

@@ -1399,6 +1409,8 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_tcp4 __read_mostly =
 	.from_nlattr		= nlattr_to_tcp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= tcp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

@@ -1429,6 +1441,8 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_tcp6 __read_mostly =
 	.from_nlattr		= nlattr_to_tcp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
+	.nlattr_protoinfo_size	= tcp_nlattr_protoinfo_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/netfilter/nf_conntrack_proto_udp.c +2 -0

index 2b8b1f5..e9abc8d 100644
@@ -193,6 +193,7 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_udp4 __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

@@ -220,6 +221,7 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_udp6 __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

net/netfilter/nf_conntrack_proto_udplite.c +2 -0

index 4579d8d..90f14e7 100644
@@ -181,6 +181,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_udplite4 __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

@@ -204,6 +205,7 @@ static struct nf_conntrack_l4proto nf_conntrack_l4proto_udplite6 __read_mostly =
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_size		= nf_ct_port_nlattr_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL

David Miller

maintainer

2008-11-17 08:44 UTC #12 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

From: Pablo Neira Ayuso <[email protected]>
Date: Mon, 17 Nov 2008 09:41:42 +0100

Quoted 7 lines

This patch adds dynamic message size calculation for ctnetlink. This reduces CPU consumption since the overhead in the message trimming is removed.
Based on a suggestion from Patrick McHardy.
Signed-off-by Pablo Neira Ayuso <[email protected]>

Nice patch.

Patrick McHardy

2008-11-17 15:32 UTC #13 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

In reply to Pablo Neira Ayuso

Quoted 8 lines · Pablo Neira Ayuso

This patch adds dynamic message size calculation for ctnetlink. This reduces CPU consumption since the overhead in the message trimming is removed.
static int ctnetlink_conntrack_event(struct notifier_block *this,
net/netfilter/nf_conntrack_netlink.c +0 -0
 				     unsigned long events, void *ptr)
 {

net/netfilter/nf_conntrack_netlink.c static int ctnetlink_conntrack_event(struct notifier_block *this, · line 543

Quoted 7 lines

net/netfilter/nf_conntrack_netlink.c +1 -1

@@ -437,7 +538,7 @@ static int ctnetlink_conntrack_event(struct notifier_block *this,
 	if (!nfnetlink_has_listeners(group))
 		return NOTIFY_DONE;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_ATOMIC);
+	skb = alloc_skb(ctnetlink_calculate_room_size(ct, events), GFP_ATOMIC);
 	if (!skb)
 		return NOTIFY_DONE;

These calculations look somewhat expensive to perform for every message. Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?

Since many of these are static in size an alternative would be to update those sizes during protocol/helper/whatever (un)registration. But if this patch already improves things, we can put it in and work on perfecting it later :)

Pablo Neira Ayuso

author maintainer

2008-11-18 03:33 UTC #14 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

Quoted 13 lines · Patrick McHardy

Quoted 12 lines · Pablo Neira Ayuso
This patch adds dynamic message size calculation for ctnetlink. This reduces CPU consumption since the overhead in the message trimming is removed.
static int ctnetlink_conntrack_event(struct notifier_block *this,
unsigned long events, void *ptr)
{
net/netfilter/nf_conntrack_netlink.c +0 -0
@@ -437,7 +538,7 @@ static int ctnetlink_conntrack_event(struct
 notifier_block *this,
     if (!nfnetlink_has_listeners(group))
         return NOTIFY_DONE;
 

Quoted 9 lines

Quoted 5 lines
net/netfilter/nf_conntrack_netlink.c +1 -1
-    skb = alloc_skb(NLMSG_GOODSIZE, GFP_ATOMIC);
+    skb = alloc_skb(ctnetlink_calculate_room_size(ct, events),
 GFP_ATOMIC);
     if (!skb)
         return NOTIFY_DONE;
These calculations look somewhat expensive to perform for every message. Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?

Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.

Quoted 4 lines

Since many of these are static in size an alternative would be to update those sizes during protocol/helper/whatever (un)registration. But if this patch already improves things, we can put it in and work on perfecting it later :)

We can same save some branches doing so, but still ctnetlink has a lot of attributes that are dependent of the message type.

What if we reduce the size of the skb allocated for netlink to more reasonable size instead of the accurate calculation? I'll check this possibility.

BTW, probably you already know, but in case that you try to use oprofile with your current tree, since it is based on 2.6.28-rc2, oprofile was broken here, I had to pull the fixes from somewhere else.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

Pablo Neira Ayuso

author maintainer

2008-11-18 10:22 UTC #15 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

Quoted 8 lines · Pablo Neira Ayuso

Quoted 3 lines · Patrick McHardy
These calculations look somewhat expensive to perform for every message. Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?
Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.

Some oprofile results:

wo/patch
2189 0.0305 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event

w/patch
2302 0.0440 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event

While __alloc_skb and netlink_broadcast report similar values for w/ and wo/ the patch.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

Patrick McHardy

2008-11-18 11:03 UTC #16 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

Quoted 15 lines · Pablo Neira Ayuso

Quoted 7 lines · Pablo Neira Ayuso
Quoted 3 lines · Patrick McHardy
These calculations look somewhat expensive to perform for every message. Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?
Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.
Some oprofile results:
wo/patch
2189 0.0305 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event

Quoted 6 lines

w/patch
2302 0.0440 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event
While __alloc_skb and netlink_broadcast report similar values for w/ and wo/ the patch.

So its actually getting worse? :) Any other differences, like less cycles for memcpy in netlink_trim()?

Pablo Neira Ayuso

author maintainer

2008-11-19 00:03 UTC #17 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

Quoted 21 lines · Patrick McHardy

Quoted 20 lines · Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso
Quoted 4 lines · Patrick McHardy
These calculations look somewhat expensive to perform for every message.
Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?
Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.
Some oprofile results:
wo/patch
2189 0.0305 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event
w/patch
2302 0.0440 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event

Quoted 5 lines

Quoted 2 lines
While __alloc_skb and netlink_broadcast report similar values for w/ and wo/ the patch.
So its actually getting worse? :) Any other differences, like less cycles for memcpy in netlink_trim()?

netlink_trim is inlined, so it is included in netlink_broadcast, and there's no improve in memcpy nor netlink_broadcast. I'm going to repeat all the test to check if I'm doing something wrong, until that, let's keep it back.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

Patrick McHardy

2008-11-19 12:05 UTC #18 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

Quoted 23 lines · Pablo Neira Ayuso

Quoted 22 lines · Patrick McHardy
Quoted 21 lines · Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso
Quoted 4 lines · Patrick McHardy
These calculations look somewhat expensive to perform for every message.
Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?
Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.
Some oprofile results:
wo/patch
2189 0.0305 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event
w/patch
2302 0.0440 nf_conntrack_netlink.ko nf_conntrack_netlink
ctnetlink_conntrack_event
While __alloc_skb and netlink_broadcast report similar values for w/ and wo/ the patch.

Quoted 7 lines

Quoted 2 lines
So its actually getting worse? :) Any other differences, like less cycles for memcpy in netlink_trim()?
netlink_trim is inlined, so it is included in netlink_broadcast, and there's no improve in memcpy nor netlink_broadcast. I'm going to repeat all the test to check if I'm doing something wrong, until that, let's keep it back.

Thats really strange, there has to be at least some reduction of work because we should be avoiding the packet copy in netlink_trim. Unless there's a bug somewhere in the calculation and we're still overallocating by more than 50%.

Patrick McHardy

2008-11-18 11:01 UTC #19 lore raw

Re: [PATCH 7/8] [PATCH] dynamic calculation of event message size for ctnetlink

In reply to Pablo Neira Ayuso

Quoted 23 lines · Pablo Neira Ayuso

Quoted 13 lines · Patrick McHardy
Quoted 12 lines · Pablo Neira Ayuso
This patch adds dynamic message size calculation for ctnetlink. This reduces CPU consumption since the overhead in the message trimming is removed.
static int ctnetlink_conntrack_event(struct notifier_block *this,
unsigned long events, void *ptr)
{
net/netfilter/nf_conntrack_netlink.c +0 -0
@@ -437,7 +538,7 @@ static int ctnetlink_conntrack_event(struct
 notifier_block *this,
     if (!nfnetlink_has_listeners(group))
         return NOTIFY_DONE;
 
Quoted 8 lines
Quoted 5 lines
net/netfilter/nf_conntrack_netlink.c +1 -1
-    skb = alloc_skb(NLMSG_GOODSIZE, GFP_ATOMIC);
+    skb = alloc_skb(ctnetlink_calculate_room_size(ct, events),
 GFP_ATOMIC);
     if (!skb)
         return NOTIFY_DONE;
These calculations look somewhat expensive to perform for every message. Do you have any numbers for this new patch that shows the difference in CPU usage compared to the resizing done by af_netlink.c?

Quoted 3 lines

Fabian Hugelshofer reported some reduction (~5%) on an embedded environment but he was using top to measure the difference. I'll collect some more trustable data and get back to you.

Thanks.

Quoted 7 lines

Quoted 4 lines
Since many of these are static in size an alternative would be to update those sizes during protocol/helper/whatever (un)registration. But if this patch already improves things, we can put it in and work on perfecting it later :)
We can same save some branches doing so, but still ctnetlink has a lot of attributes that are dependent of the message type.

Indeed. But f.i. all the ->size(void) functions could obviously be constants.

Quoted 3 lines

What if we reduce the size of the skb allocated for netlink to more reasonable size instead of the accurate calculation? I'll check this possibility.

I don't think we need to be 100% accurate, just find a good middle ground between grossly overallocating and the resulting unnecessary copy, potentially expensive size calculations and socket memory waste.

Quoted 3 lines

BTW, probably you already know, but in case that you try to use oprofile with your current tree, since it is based on 2.6.28-rc2, oprofile was broken here, I had to pull the fixes from somewhere else.

I'll probably merge what I have to Dave soon so I can resync with his tree.

Pablo Neira Ayuso

author maintainer

2008-11-17 08:42 UTC #20 lore raw

PATCH 8/8 bump the expectation helper name

In reply to Pablo Neira Ayuso

Include the expectation helper name at expectation dumping.

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

---

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

net/netfilter/nf_conntrack_netlink.c +9 -0

index 0205ae6..7756d29 100644
@@ -1517,6 +1517,7 @@ ctnetlink_exp_dump_expect(struct sk_buff *skb,
 			  const struct nf_conntrack_expect *exp)
 {
 	struct nf_conn *master = exp->master;
+	struct nf_conn_help *help = nfct_help(master);
 	long timeout = (exp->timeout.expires - jiffies) / HZ;
 
 	if (timeout < 0)

@@ -1534,6 +1535,14 @@ ctnetlink_exp_dump_expect(struct sk_buff *skb,
 	NLA_PUT_BE32(skb, CTA_EXPECT_TIMEOUT, htonl(timeout));
 	NLA_PUT_BE32(skb, CTA_EXPECT_ID, htonl((unsigned long)exp));
 
+	if (help) {
+		struct nf_conntrack_helper *helper;
+
+		helper = rcu_dereference(help->helper);
+		if (helper)
+			NLA_PUT_STRING(skb, CTA_EXPECT_HELP_NAME, helper->name);
+	}
+
 	return 0;
 
 nla_put_failure:

Patrick McHardy

Applied

2008-11-17 14:55 UTC #21 lore raw

In reply to Pablo Neira Ayuso

Quoted 1 line · Pablo Neira Ayuso

Use nf_conntrack_get instead of the direct call to atomic_inc.

Applied, thanks.

BTW, it would save me some work if you could already use the same headline-format that I use. In this case this would have been:

netfilter: ctnetlink: use nf_contrack_get ...