[PATCH 1/2] netfilter: xt_CT: refactorize xt_ct_tg_check

[PATCH 1/2] netfilter: xt_CT: refactorize xt_ct_tg_check

[pablo]

From: Pablo Neira Ayuso <pablo@netfilter.org>

This patch adds xt_ct_set_helper and xt_ct_set_timeout to reduce
the size of xt_ct_tg_check.

This aims to improve code mantainability by splitting xt_ct_tg_check
in smaller chunks.

Suggested by Eric Dumazet.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |  262 ++++++++++++++++++++++++-------------------------
 1 file changed, 126 insertions(+), 136 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 1160185..16c7125 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -72,14 +72,44 @@ static u8 xt_ct_find_proto(const struct xt_tgchk_param *par)
 		return 0;
 }
 
+static int
+xt_ct_set_helper(struct nf_conn *ct, const char *helper_name,
+		 const struct xt_tgchk_param *par)
+{
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help;
+	u8 proto;
+
+	proto = xt_ct_find_proto(par);
+	if (!proto) {
+		pr_info("You must specify a L4 protocol, and not use "
+			"inversions on it.\n");
+		return -ENOENT;
+	}
+
+	helper = nf_conntrack_helper_try_module_get(helper_name, par->family,
+						    proto);
+	if (helper == NULL) {
+		pr_info("No such helper \"%s\"\n", helper_name);
+		return -ENOENT;
+	}
+
+	help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL);
+	if (help == NULL) {
+		module_put(helper->me);
+		return -ENOMEM;
+	}
+
+	help->helper = helper;
+	return 0;
+}
+
 static int xt_ct_tg_check_v0(const struct xt_tgchk_param *par)
 {
 	struct xt_ct_target_info *info = par->targinfo;
 	struct nf_conntrack_tuple t;
-	struct nf_conn_help *help;
 	struct nf_conn *ct;
-	int ret = 0;
-	u8 proto;
+	int ret;
 
 	if (info->flags & ~XT_CT_NOTRACK)
 		return -EINVAL;
@@ -112,31 +142,9 @@ static int xt_ct_tg_check_v0(const struct xt_tgchk_param *par)
 		goto err3;
 
 	if (info->helper[0]) {
-		struct nf_conntrack_helper *helper;
-
-		ret = -ENOENT;
-		proto = xt_ct_find_proto(par);
-		if (!proto) {
-			pr_info("You must specify a L4 protocol, "
-				"and not use inversions on it.\n");
-			goto err3;
-		}
-
-		ret = -ENOENT;
-		helper = nf_conntrack_helper_try_module_get(info->helper,
-							    par->family,
-							    proto);
-		if (helper == NULL) {
-			pr_info("No such helper \"%s\"\n", info->helper);
-			goto err3;
-		}
-
-		ret = -ENOMEM;
-		help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL);
-		if (help == NULL)
+		ret = xt_ct_set_helper(ct, info->helper, par);
+		if (ret < 0)
 			goto err3;
-
-		help->helper = helper;
 	}
 
 	__set_bit(IPS_TEMPLATE_BIT, &ct->status);
@@ -164,17 +172,77 @@ static void __xt_ct_tg_timeout_put(struct ctnl_timeout *timeout)
 }
 #endif
 
+static int
+xt_ct_set_timeout(struct nf_conn *ct, const struct xt_tgchk_param *par,
+		  const char *timeout_name)
+{
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+	typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
+	struct ctnl_timeout *timeout;
+	struct nf_conn_timeout *timeout_ext;
+	const struct ipt_entry *e = par->entryinfo;
+	struct nf_conntrack_l4proto *l4proto;
+	int ret = 0;
+
+	rcu_read_lock();
+	timeout_find_get = rcu_dereference(nf_ct_timeout_find_get_hook);
+	if (timeout_find_get == NULL) {
+		ret = -ENOENT;
+		pr_info("Timeout policy base is empty\n");
+		goto out;
+	}
+
+	if (e->ip.invflags & IPT_INV_PROTO) {
+		ret = -EINVAL;
+		pr_info("You cannot use inversion on L4 protocol\n");
+		goto out;
+	}
+
+	timeout = timeout_find_get(timeout_name);
+	if (timeout == NULL) {
+		ret = -ENOENT;
+		pr_info("No such timeout policy \"%s\"\n", timeout_name);
+		goto out;
+	}
+
+	if (timeout->l3num != par->family) {
+		ret = -EINVAL;
+		pr_info("Timeout policy `%s' can only be used by L3 protocol "
+			"number %d\n", timeout_name, timeout->l3num);
+		goto err_put_timeout;
+	}
+	/* Make sure the timeout policy matches any existing protocol tracker,
+	 * otherwise default to generic.
+	 */
+	l4proto = __nf_ct_l4proto_find(par->family, e->ip.proto);
+	if (timeout->l4proto->l4proto != l4proto->l4proto) {
+		ret = -EINVAL;
+		pr_info("Timeout policy `%s' can only be used by L4 protocol "
+			"number %d\n",
+			timeout_name, timeout->l4proto->l4proto);
+		goto err_put_timeout;
+	}
+	timeout_ext = nf_ct_timeout_ext_add(ct, timeout, GFP_ATOMIC);
+	if (timeout_ext == NULL)
+		ret = -ENOMEM;
+
+err_put_timeout:
+	__xt_ct_tg_timeout_put(timeout);
+out:
+	rcu_read_unlock();
+	return ret;
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
 static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 {
 	struct xt_ct_target_info_v1 *info = par->targinfo;
 	struct nf_conntrack_tuple t;
-	struct nf_conn_help *help;
 	struct nf_conn *ct;
-	int ret = 0;
-	u8 proto;
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
-	struct ctnl_timeout *timeout;
-#endif
+	int ret;
+
 	if (info->flags & ~XT_CT_NOTRACK)
 		return -EINVAL;
 
@@ -206,93 +274,16 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 		goto err3;
 
 	if (info->helper[0]) {
-		struct nf_conntrack_helper *helper;
-
-		ret = -ENOENT;
-		proto = xt_ct_find_proto(par);
-		if (!proto) {
-			pr_info("You must specify a L4 protocol, "
-				"and not use inversions on it.\n");
-			goto err3;
-		}
-
-		ret = -ENOENT;
-		helper = nf_conntrack_helper_try_module_get(info->helper,
-							    par->family,
-							    proto);
-		if (helper == NULL) {
-			pr_info("No such helper \"%s\"\n", info->helper);
-			goto err3;
-		}
-
-		ret = -ENOMEM;
-		help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL);
-		if (help == NULL)
+		ret = xt_ct_set_helper(ct, info->helper, par);
+		if (ret < 0)
 			goto err3;
-
-		help->helper = helper;
 	}
 
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 	if (info->timeout[0]) {
-		typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
-		struct nf_conn_timeout *timeout_ext;
-
-		rcu_read_lock();
-		timeout_find_get =
-			rcu_dereference(nf_ct_timeout_find_get_hook);
-
-		if (timeout_find_get) {
-			const struct ipt_entry *e = par->entryinfo;
-			struct nf_conntrack_l4proto *l4proto;
-
-			if (e->ip.invflags & IPT_INV_PROTO) {
-				ret = -EINVAL;
-				pr_info("You cannot use inversion on "
-					 "L4 protocol\n");
-				goto err4;
-			}
-			timeout = timeout_find_get(info->timeout);
-			if (timeout == NULL) {
-				ret = -ENOENT;
-				pr_info("No such timeout policy \"%s\"\n",
-					info->timeout);
-				goto err4;
-			}
-			if (timeout->l3num != par->family) {
-				ret = -EINVAL;
-				pr_info("Timeout policy `%s' can only be "
-					"used by L3 protocol number %d\n",
-					info->timeout, timeout->l3num);
-				goto err5;
-			}
-			/* Make sure the timeout policy matches any existing
-			 * protocol tracker, otherwise default to generic.
-			 */
-			l4proto = __nf_ct_l4proto_find(par->family,
-						       e->ip.proto);
-			if (timeout->l4proto->l4proto != l4proto->l4proto) {
-				ret = -EINVAL;
-				pr_info("Timeout policy `%s' can only be "
-					"used by L4 protocol number %d\n",
-					info->timeout,
-					timeout->l4proto->l4proto);
-				goto err5;
-			}
-			timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
-							    GFP_ATOMIC);
-			if (timeout_ext == NULL) {
-				ret = -ENOMEM;
-				goto err5;
-			}
-		} else {
-			ret = -ENOENT;
-			pr_info("Timeout policy base is empty\n");
-			goto err4;
-		}
-		rcu_read_unlock();
+		ret = xt_ct_set_timeout(ct, par, info->timeout);
+		if (ret < 0)
+			goto err3;
 	}
-#endif
 
 	__set_bit(IPS_TEMPLATE_BIT, &ct->status);
 	__set_bit(IPS_CONFIRMED_BIT, &ct->status);
@@ -300,12 +291,6 @@ out:
 	info->ct = ct;
 	return 0;
 
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
-err5:
-	__xt_ct_tg_timeout_put(timeout);
-err4:
-	rcu_read_unlock();
-#endif
 err3:
 	nf_conntrack_free(ct);
 err2:
@@ -330,15 +315,30 @@ static void xt_ct_tg_destroy_v0(const struct xt_tgdtor_param *par)
 	nf_ct_put(info->ct);
 }
 
-static void xt_ct_tg_destroy_v1(const struct xt_tgdtor_param *par)
+static void xt_ct_destroy_timeout(struct nf_conn *ct)
 {
-	struct xt_ct_target_info_v1 *info = par->targinfo;
-	struct nf_conn *ct = info->ct;
-	struct nf_conn_help *help;
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 	struct nf_conn_timeout *timeout_ext;
 	typeof(nf_ct_timeout_put_hook) timeout_put;
+
+	rcu_read_lock();
+	timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
+
+	if (timeout_put) {
+		timeout_ext = nf_ct_timeout_find(ct);
+		if (timeout_ext)
+			timeout_put(timeout_ext->timeout);
+	}
+	rcu_read_unlock();
 #endif
+}
+
+static void xt_ct_tg_destroy_v1(const struct xt_tgdtor_param *par)
+{
+	struct xt_ct_target_info_v1 *info = par->targinfo;
+	struct nf_conn *ct = info->ct;
+	struct nf_conn_help *help;
+
 	if (!nf_ct_is_untracked(ct)) {
 		help = nfct_help(ct);
 		if (help)
@@ -346,17 +346,7 @@ static void xt_ct_tg_destroy_v1(const struct xt_tgdtor_param *par)
 
 		nf_ct_l3proto_module_put(par->family);
 
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
-		rcu_read_lock();
-		timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
-
-		if (timeout_put) {
-			timeout_ext = nf_ct_timeout_find(ct);
-			if (timeout_ext)
-				timeout_put(timeout_ext->timeout);
-		}
-		rcu_read_unlock();
-#endif
+		xt_ct_destroy_timeout(ct);
 	}
 	nf_ct_put(info->ct);
 }
-- 
1.7.10.4

[PATCH 2/2] netfilter: nf_conntrack: add nf_ct_timeout_lookup

[pablo]

From: Pablo Neira Ayuso <pablo@netfilter.org>

This patch adds the new nf_ct_timeout_lookup function to encapsulate
the timeout policy attachment that is called in the nf_conntrack_in
path.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_timeout.h |   20 ++++++++++++++++++++
 net/netfilter/nf_conntrack_core.c            |    7 +------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
index 34ec89f..e41e472 100644
--- a/include/net/netfilter/nf_conntrack_timeout.h
+++ b/include/net/netfilter/nf_conntrack_timeout.h
@@ -55,6 +55,26 @@ struct nf_conn_timeout *nf_ct_timeout_ext_add(struct nf_conn *ct,
 #endif
 };
 
+static inline unsigned int *
+nf_ct_timeout_lookup(struct net *net, struct nf_conn *ct,
+		     struct nf_conntrack_l4proto *l4proto)
+{
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+	struct nf_conn_timeout *timeout_ext;
+	unsigned int *timeouts;
+
+	timeout_ext = nf_ct_timeout_find(ct);
+	if (timeout_ext)
+		timeouts = NF_CT_TIMEOUT_EXT_DATA(timeout_ext);
+	else
+		timeouts = l4proto->get_timeouts(net);
+
+	return timeouts;
+#else
+	return l4proto->get_timeouts(net);
+#endif
+}
+
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 extern int nf_conntrack_timeout_init(struct net *net);
 extern void nf_conntrack_timeout_fini(struct net *net);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index cf48755..fc61f40 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -924,7 +924,6 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned int hooknum,
 	enum ip_conntrack_info ctinfo;
 	struct nf_conntrack_l3proto *l3proto;
 	struct nf_conntrack_l4proto *l4proto;
-	struct nf_conn_timeout *timeout_ext;
 	unsigned int *timeouts;
 	unsigned int dataoff;
 	u_int8_t protonum;
@@ -991,11 +990,7 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned int hooknum,
 	NF_CT_ASSERT(skb->nfct);
 
 	/* Decide what timeout policy we want to apply to this flow. */
-	timeout_ext = nf_ct_timeout_find(ct);
-	if (timeout_ext)
-		timeouts = NF_CT_TIMEOUT_EXT_DATA(timeout_ext);
-	else
-		timeouts = l4proto->get_timeouts(net);
+	timeouts = nf_ct_timeout_lookup(net, ct, l4proto);
 
 	ret = l4proto->packet(ct, skb, dataoff, ctinfo, pf, hooknum, timeouts);
 	if (ret <= 0) {
-- 
1.7.10.4