Refine the robust of helper expectfn

PATCH In review [email protected] 2 participants 8 messages Mar 31, 2017 – Apr 17, 2017

Subsystems (3)

Networking netfilter IPVS

lore

Share & tools

lore

[email protected]

author

2017-03-31 10:38 UTC #0 lore raw

v5 cover Refine the robust of helper expectfn

From: Gao Feng <[email protected]>

These patches are used to refine the codes of helper expectfn, and enhance its robust, including fixing possible panic bug.

Gao Feng (3):
netfilter: helper: Rename struct nf_ct_helper_expectfn to
nf_ct_nat_helper
netfilter: nat_helper: Make sure every proto nat module uses its
nat_helper
netfilter: nat_helper: Remove the expectations when its module is
unloaded

 include/net/netfilter/nf_conntrack_expect.h |  5 +-
 include/net/netfilter/nf_conntrack_helper.h | 13 ++--
 net/ipv4/netfilter/nf_nat_h323.c            | 55 +++++++++++------
 net/netfilter/ipvs/ip_vs_nfct.c             |  8 ++-
 net/netfilter/nf_conntrack_broadcast.c      |  2 +-
 net/netfilter/nf_conntrack_core.c           |  4 +-
 net/netfilter/nf_conntrack_expect.c         | 10 ++-
 net/netfilter/nf_conntrack_helper.c         | 94 +++++++++++++++++------------
 net/netfilter/nf_conntrack_netlink.c        | 20 +++---
 net/netfilter/nf_conntrack_pptp.c           | 15 ++++-
 net/netfilter/nf_nat_amanda.c               | 10 ++-
 net/netfilter/nf_nat_core.c                 |  7 ++-
 net/netfilter/nf_nat_ftp.c                  | 10 ++-
 net/netfilter/nf_nat_irc.c                  | 10 ++-
 net/netfilter/nf_nat_sip.c                  | 23 ++++---
 net/netfilter/nf_nat_tftp.c                 | 10 ++-
 16 files changed, 194 insertions(+), 102 deletions(-)

Signature · 1 line

1.9.1

[email protected]

author

2017-03-31 10:38 UTC #1 lore raw

PATCH v5 1/3 netfilter: helper: Rename struct nf_ct_helper_expectfn to nf_ct_nat_helper

From: Gao Feng <[email protected]>

Rename struct nf_ct_helper_expectfn to nf_ct_nat_helper, and rename other functions or variables which refer to it.
The new name is better than the old one.

Signed-off-by Gao Feng <[email protected]>

---
v5: Register one nat_helper for every nat module, per Pablo
v4: Cover the nat_module assignment in dataplane, per Pablo
v3: Rename the nf_ct_helper_expectfn, func, and member, per Pablo
v2: Use the module as the identifier when flush expect
v1: initial version

 include/net/netfilter/nf_conntrack_helper.h | 14 ++++++-------
 net/ipv4/netfilter/nf_nat_h323.c            | 12 +++++------
 net/netfilter/nf_conntrack_helper.c         | 32 ++++++++++++++---------------
 net/netfilter/nf_conntrack_netlink.c        | 16 +++++++--------
 net/netfilter/nf_nat_core.c                 |  6 +++---
 net/netfilter/nf_nat_sip.c                  |  6 +++---
 6 files changed, 43 insertions(+), 43 deletions(-)

include/net/netfilter/nf_conntrack_helper.h +7 -7

index 1eaac1f..d14fe493 100644
@@ -111,7 +111,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff,
 				enum ip_conntrack_info ctinfo,
 				unsigned int timeout);
 
-struct nf_ct_helper_expectfn {
+struct nf_ct_nat_helper {
 	struct list_head head;
 	const char *name;
 	void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp);

@@ -121,12 +121,12 @@ struct nf_ct_helper_expectfn {
 void nf_ct_helper_log(struct sk_buff *skb, const struct nf_conn *ct,
 		      const char *fmt, ...);
 
-void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n);
-void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n);
-struct nf_ct_helper_expectfn *
-nf_ct_helper_expectfn_find_by_name(const char *name);
-struct nf_ct_helper_expectfn *
-nf_ct_helper_expectfn_find_by_symbol(const void *symbol);
+void nf_ct_nat_helper_register(struct nf_ct_nat_helper *n);
+void nf_ct_nat_helper_unregister(struct nf_ct_nat_helper *n);
+struct nf_ct_nat_helper *
+nf_ct_nat_helper_find_by_name(const char *name);
+struct nf_ct_nat_helper *
+nf_ct_nat_helper_find_by_symbol(const void *symbol);
 
 extern struct hlist_head *nf_ct_helper_hash;
 extern unsigned int nf_ct_helper_hsize;

net/ipv4/netfilter/nf_nat_h323.c +6 -6

index 574f7eb..346e764 100644
@@ -567,12 +567,12 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
 	return 0;
 }
 
-static struct nf_ct_helper_expectfn q931_nat = {
+static struct nf_ct_nat_helper q931_nat = {
 	.name		= "Q.931",
 	.expectfn	= ip_nat_q931_expect,
 };
 
-static struct nf_ct_helper_expectfn callforwarding_nat = {
+static struct nf_ct_nat_helper callforwarding_nat = {
 	.name		= "callforwarding",
 	.expectfn	= ip_nat_callforwarding_expect,
 };

@@ -599,8 +599,8 @@ static int __init init(void)
 	RCU_INIT_POINTER(nat_h245_hook, nat_h245);
 	RCU_INIT_POINTER(nat_callforwarding_hook, nat_callforwarding);
 	RCU_INIT_POINTER(nat_q931_hook, nat_q931);
-	nf_ct_helper_expectfn_register(&q931_nat);
-	nf_ct_helper_expectfn_register(&callforwarding_nat);
+	nf_ct_nat_helper_register(&q931_nat);
+	nf_ct_nat_helper_register(&callforwarding_nat);
 	return 0;
 }

@@ -616,8 +616,8 @@ static void __exit fini(void)
 	RCU_INIT_POINTER(nat_h245_hook, NULL);
 	RCU_INIT_POINTER(nat_callforwarding_hook, NULL);
 	RCU_INIT_POINTER(nat_q931_hook, NULL);
-	nf_ct_helper_expectfn_unregister(&q931_nat);
-	nf_ct_helper_expectfn_unregister(&callforwarding_nat);
+	nf_ct_nat_helper_unregister(&q931_nat);
+	nf_ct_nat_helper_unregister(&callforwarding_nat);
 	synchronize_rcu();
 }

net/netfilter/nf_conntrack_helper.c +16 ~~-16~~

index 6dc44d9..0eaa01e 100644
@@ -293,32 +293,32 @@ void nf_ct_helper_destroy(struct nf_conn *ct)
 	}
 }
 
-static LIST_HEAD(nf_ct_helper_expectfn_list);
+static LIST_HEAD(nf_ct_nat_helper_list);
 
-void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n)
+void nf_ct_nat_helper_register(struct nf_ct_nat_helper *n)
 {
 	spin_lock_bh(&nf_conntrack_expect_lock);
-	list_add_rcu(&n->head, &nf_ct_helper_expectfn_list);
+	list_add_rcu(&n->head, &nf_ct_nat_helper_list);
 	spin_unlock_bh(&nf_conntrack_expect_lock);
 }
-EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_register);
+EXPORT_SYMBOL_GPL(nf_ct_nat_helper_register);
 
-void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n)
+void nf_ct_nat_helper_unregister(struct nf_ct_nat_helper *n)
 {
 	spin_lock_bh(&nf_conntrack_expect_lock);
 	list_del_rcu(&n->head);
 	spin_unlock_bh(&nf_conntrack_expect_lock);
 }
-EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_unregister);
+EXPORT_SYMBOL_GPL(nf_ct_nat_helper_unregister);
 
-struct nf_ct_helper_expectfn *
-nf_ct_helper_expectfn_find_by_name(const char *name)
+struct nf_ct_nat_helper *
+nf_ct_nat_helper_find_by_name(const char *name)
 {
-	struct nf_ct_helper_expectfn *cur;
+	struct nf_ct_nat_helper *cur;
 	bool found = false;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
+	list_for_each_entry_rcu(cur, &nf_ct_nat_helper_list, head) {
 		if (!strcmp(cur->name, name)) {
 			found = true;
 			break;

@@ -327,16 +327,16 @@ struct nf_ct_helper_expectfn *
 	rcu_read_unlock();
 	return found ? cur : NULL;
 }
-EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_name);
+EXPORT_SYMBOL_GPL(nf_ct_nat_helper_find_by_name);
 
-struct nf_ct_helper_expectfn *
-nf_ct_helper_expectfn_find_by_symbol(const void *symbol)
+struct nf_ct_nat_helper *
+nf_ct_nat_helper_find_by_symbol(const void *symbol)
 {
-	struct nf_ct_helper_expectfn *cur;
+	struct nf_ct_nat_helper *cur;
 	bool found = false;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(cur, &nf_ct_helper_expectfn_list, head) {
+	list_for_each_entry_rcu(cur, &nf_ct_nat_helper_list, head) {
 		if (cur->expectfn == symbol) {
 			found = true;
 			break;

@@ -345,7 +345,7 @@ struct nf_ct_helper_expectfn *
 	rcu_read_unlock();
 	return found ? cur : NULL;
 }
-EXPORT_SYMBOL_GPL(nf_ct_helper_expectfn_find_by_symbol);
+EXPORT_SYMBOL_GPL(nf_ct_nat_helper_find_by_symbol);
 
 __printf(3, 4)
 void nf_ct_helper_log(struct sk_buff *skb, const struct nf_conn *ct,

net/netfilter/nf_conntrack_netlink.c +8 -8

index 908d858..e0de100 100644
@@ -2508,7 +2508,7 @@ static int ctnetlink_exp_dump_mask(struct sk_buff *skb,
 	struct nlattr *nest_parms;
 	struct nf_conntrack_tuple nat_tuple = {};
 #endif
-	struct nf_ct_helper_expectfn *expfn;
+	struct nf_ct_nat_helper *nat_helper;
 
 	if (timeout < 0)
 		timeout = 0;

@@ -2557,9 +2557,9 @@ static int ctnetlink_exp_dump_mask(struct sk_buff *skb,
 		    nla_put_string(skb, CTA_EXPECT_HELP_NAME, helper->name))
 			goto nla_put_failure;
 	}
-	expfn = nf_ct_helper_expectfn_find_by_symbol(exp->expectfn);
-	if (expfn != NULL &&
-	    nla_put_string(skb, CTA_EXPECT_FN, expfn->name))
+	nat_helper = nf_ct_nat_helper_find_by_symbol(exp->expectfn);
+	if (!nat_helper &&
+	    nla_put_string(skb, CTA_EXPECT_FN, nat_helper->name))
 		goto nla_put_failure;
 
 	return 0;

@@ -3070,14 +3070,14 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
 	}
 	if (cda[CTA_EXPECT_FN]) {
 		const char *name = nla_data(cda[CTA_EXPECT_FN]);
-		struct nf_ct_helper_expectfn *expfn;
+		struct nf_ct_nat_helper *nat_helper;
 
-		expfn = nf_ct_helper_expectfn_find_by_name(name);
-		if (expfn == NULL) {
+		nat_helper = nf_ct_nat_helper_find_by_name(name);
+		if (!nat_helper) {
 			err = -EINVAL;
 			goto err_out;
 		}
-		exp->expectfn = expfn->expectfn;
+		exp->expectfn = nat_helper->expectfn;
 	} else
 		exp->expectfn = NULL;

net/netfilter/nf_nat_core.c +3 -3

index 82802e4..2897bd4 100644
@@ -848,7 +848,7 @@ static void __net_exit nf_nat_net_exit(struct net *net)
 	.exit = nf_nat_net_exit,
 };
 
-static struct nf_ct_helper_expectfn follow_master_nat = {
+static struct nf_ct_nat_helper follow_master_nat = {
 	.name		= "nat-follow-master",
 	.expectfn	= nf_nat_follow_master,
 };

@@ -872,7 +872,7 @@ static int __init nf_nat_init(void)
 	if (ret < 0)
 		goto cleanup_extend;
 
-	nf_ct_helper_expectfn_register(&follow_master_nat);
+	nf_ct_nat_helper_register(&follow_master_nat);
 
 	/* Initialize fake conntrack so that NAT will skip it */
 	nf_ct_untracked_status_or(IPS_NAT_DONE_MASK);

@@ -898,7 +898,7 @@ static void __exit nf_nat_cleanup(void)
 
 	unregister_pernet_subsys(&nf_nat_net_ops);
 	nf_ct_extend_unregister(&nat_extend);
-	nf_ct_helper_expectfn_unregister(&follow_master_nat);
+	nf_ct_nat_helper_unregister(&follow_master_nat);
 	RCU_INIT_POINTER(nfnetlink_parse_nat_setup_hook, NULL);
 #ifdef CONFIG_XFRM
 	RCU_INIT_POINTER(nf_nat_decode_session_hook, NULL);

net/netfilter/nf_nat_sip.c +3 -3

index 791fac4..d27c5a2 100644
@@ -618,7 +618,7 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 	return NF_DROP;
 }
 
-static struct nf_ct_helper_expectfn sip_nat = {
+static struct nf_ct_nat_helper sip_nat = {
 	.name		= "sip",
 	.expectfn	= nf_nat_sip_expected,
 };

@@ -627,7 +627,7 @@ static void __exit nf_nat_sip_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_sip_hooks, NULL);
 
-	nf_ct_helper_expectfn_unregister(&sip_nat);
+	nf_ct_nat_helper_unregister(&sip_nat);
 	synchronize_rcu();
 }

@@ -645,7 +645,7 @@ static int __init nf_nat_sip_init(void)
 {
 	BUG_ON(nf_nat_sip_hooks != NULL);
 	RCU_INIT_POINTER(nf_nat_sip_hooks, &sip_hooks);
-	nf_ct_helper_expectfn_register(&sip_nat);
+	nf_ct_nat_helper_register(&sip_nat);
 	return 0;
 }

Signature · 1 line

1.9.1

[email protected]

author

2017-03-31 10:38 UTC #2 lore raw

PATCH v5 2/3 netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

In reply to [email protected]

From: Gao Feng <[email protected]>

Make sure every proto nat module owns one struct nat_helper at least, and it only uses its nat_helper.

1. Every proto nat module registers one nat_helper at least;
2. Replace the expectfn with nat_helper in the nf_conntrack_expect; It is helpful to maintain the nat_helper codes
3. Make sure the nat module only uses its nat_helper;
4. Remove nf_ct_nat_helper_find_by_symbol, it is useless now.

Signed-off-by Gao Feng <[email protected]>

 include/net/netfilter/nf_conntrack_expect.h |  5 ++--
 include/net/netfilter/nf_conntrack_helper.h |  2 --
 net/ipv4/netfilter/nf_nat_h323.c            | 44 ++++++++++++++++++-----------
 net/netfilter/ipvs/ip_vs_nfct.c             |  7 ++++-
 net/netfilter/nf_conntrack_broadcast.c      |  2 +-
 net/netfilter/nf_conntrack_core.c           |  4 +--
 net/netfilter/nf_conntrack_expect.c         |  2 +-
 net/netfilter/nf_conntrack_netlink.c        | 14 ++++-----
 net/netfilter/nf_conntrack_pptp.c           | 14 +++++++--
 net/netfilter/nf_nat_amanda.c               |  9 +++++-
 net/netfilter/nf_nat_ftp.c                  |  9 +++++-
 net/netfilter/nf_nat_irc.c                  |  9 +++++-
 net/netfilter/nf_nat_sip.c                  | 18 ++++++------
 net/netfilter/nf_nat_tftp.c                 |  9 +++++-
 14 files changed, 101 insertions(+), 47 deletions(-)

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

index 5ed33ea..f665a6b 100644
@@ -23,9 +23,8 @@ struct nf_conntrack_expect {
 	struct nf_conntrack_tuple tuple;
 	struct nf_conntrack_tuple_mask mask;
 
-	/* Function to call after setup and insertion */
-	void (*expectfn)(struct nf_conn *new,
-			 struct nf_conntrack_expect *this);
+	/* Expectation function owner */
+	struct nf_ct_nat_helper *nat_helper;
 
 	/* Helper to assign to new connection */
 	struct nf_conntrack_helper *helper;

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

index d14fe493..e8d31ca 100644
@@ -125,8 +125,6 @@ void nf_ct_helper_log(struct sk_buff *skb, const struct nf_conn *ct,
 void nf_ct_nat_helper_unregister(struct nf_ct_nat_helper *n);
 struct nf_ct_nat_helper *
 nf_ct_nat_helper_find_by_name(const char *name);
-struct nf_ct_nat_helper *
-nf_ct_nat_helper_find_by_symbol(const void *symbol);
 
 extern struct hlist_head *nf_ct_helper_hash;
 extern unsigned int nf_ct_helper_hsize;

net/ipv4/netfilter/nf_nat_h323.c +28 ~~-16~~

100 lines not on this page · raw message

net/netfilter/ipvs/ip_vs_nfct.c +6 -1

index fc230d9..83ad79f 100644
@@ -220,6 +220,11 @@ static void ip_vs_nfct_expect_callback(struct nf_conn *ct,
 	return;
 }
 
+static struct nf_ct_nat_helper ip_vs_nat = {
+	.name = "ip_vs_nat",
+	.expectfn = ip_vs_nfct_expect_callback,
+};
+
 /*
  * Create NF conntrack expectation with wildcard (optional) source port.
  * Then the default callback function will alter the reply and will confirm

@@ -245,7 +250,7 @@ void ip_vs_nfct_expect_related(struct sk_buff *skb, struct nf_conn *ct,
 			proto, port ? &port : NULL,
 			from_rs ? &cp->cport : &cp->vport);
 
-	exp->expectfn = ip_vs_nfct_expect_callback;
+	exp->nat_helper = &ip_vs_nat;
 
 	IP_VS_DBG(7, "%s: ct=%p, expect tuple=" FMT_TUPLE "\n",
 		__func__, ct, ARG_TUPLE(&exp->tuple));

net/netfilter/nf_conntrack_broadcast.c +1 -1

index 4e99cca..114e042 100644
@@ -65,7 +65,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
 	exp->mask.src.u3.ip       = mask;
 	exp->mask.src.u.udp.port  = htons(0xFFFF);
 
-	exp->expectfn             = NULL;
+	exp->nat_helper           = NULL;
 	exp->flags                = NF_CT_EXPECT_PERMANENT;
 	exp->class		  = NF_CT_EXPECT_CLASS_DEFAULT;
 	exp->helper               = NULL;

net/netfilter/nf_conntrack_core.c +2 -2

index ffb78e5..ae61513 100644
@@ -1233,8 +1233,8 @@ void nf_conntrack_free(struct nf_conn *ct)
 	local_bh_enable();
 
 	if (exp) {
-		if (exp->expectfn)
-			exp->expectfn(ct, exp);
+		if (exp->nat_helper)
+			exp->nat_helper->expectfn(ct, exp);
 		nf_ct_expect_put(exp);
 	}

net/netfilter/nf_conntrack_expect.c +1 -1

index 4b2e1fb..ba5a55e 100644
@@ -295,7 +295,7 @@ void nf_ct_expect_init(struct nf_conntrack_expect *exp, unsigned int class,
 
 	exp->flags = 0;
 	exp->class = class;
-	exp->expectfn = NULL;
+	exp->nat_helper = NULL;
 	exp->helper = NULL;
 	exp->tuple.src.l3num = family;
 	exp->tuple.dst.protonum = proto;

net/netfilter/nf_conntrack_netlink.c +7 -7

index e0de100..dc94066 100644
@@ -2508,7 +2508,6 @@ static int ctnetlink_exp_dump_mask(struct sk_buff *skb,
 	struct nlattr *nest_parms;
 	struct nf_conntrack_tuple nat_tuple = {};
 #endif
-	struct nf_ct_nat_helper *nat_helper;
 
 	if (timeout < 0)
 		timeout = 0;

@@ -2557,9 +2556,9 @@ static int ctnetlink_exp_dump_mask(struct sk_buff *skb,
 		    nla_put_string(skb, CTA_EXPECT_HELP_NAME, helper->name))
 			goto nla_put_failure;
 	}
-	nat_helper = nf_ct_nat_helper_find_by_symbol(exp->expectfn);
-	if (!nat_helper &&
-	    nla_put_string(skb, CTA_EXPECT_FN, nat_helper->name))
+
+	if (!exp->nat_helper &&
+	    nla_put_string(skb, CTA_EXPECT_FN, exp->nat_helper->name))
 		goto nla_put_failure;
 
 	return 0;

@@ -3077,9 +3076,10 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
 			err = -EINVAL;
 			goto err_out;
 		}
-		exp->expectfn = nat_helper->expectfn;
-	} else
-		exp->expectfn = NULL;
+		exp->nat_helper = nat_helper;
+	} else {
+		exp->nat_helper = NULL;
+	}
 
 	exp->class = class;
 	exp->master = ct;

net/netfilter/nf_conntrack_pptp.c +12 -2

index f60a475..5b5a45e 100644
@@ -69,6 +69,9 @@
 			     struct nf_conntrack_expect *exp) __read_mostly;
 EXPORT_SYMBOL_GPL(nf_nat_pptp_hook_expectfn);
 
+static void pptp_expectfn(struct nf_conn *ct,
+			  struct nf_conntrack_expect *exp);
+
 #if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
 /* PptpControlMessageType names */
 const char *const pptp_msg_name[] = {

@@ -99,6 +102,11 @@
 #define PPTP_GRE_TIMEOUT 		(10 MINS)
 #define PPTP_GRE_STREAM_TIMEOUT 	(5 HOURS)
 
+static struct nf_ct_nat_helper pptp_nat = {
+	.name			= "pptp_nat",
+	.expectfn		= pptp_expectfn,
+};
+
 static void pptp_expectfn(struct nf_conn *ct,
 			 struct nf_conntrack_expect *exp)
 {

@@ -221,7 +229,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &peer_callid, &callid);
-	exp_orig->expectfn = pptp_expectfn;
+	exp_orig->nat_helper = &pptp_nat;
 
 	/* reply direction, PAC->PNS */
 	dir = IP_CT_DIR_REPLY;

@@ -230,7 +238,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &callid, &peer_callid);
-	exp_reply->expectfn = pptp_expectfn;
+	exp_reply->nat_helper = &pptp_nat;
 
 	nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
 	if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)

@@ -607,11 +615,13 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 
 static int __init nf_conntrack_pptp_init(void)
 {
+	nf_ct_nat_helper_register(&pptp_nat);
 	return nf_conntrack_helper_register(&pptp);
 }
 
 static void __exit nf_conntrack_pptp_fini(void)
 {
+	nf_ct_nat_helper_unregister(&pptp_nat);
 	nf_conntrack_helper_unregister(&pptp);
 }

net/netfilter/nf_nat_amanda.c +8 -1

index eb77238..05cd3fb 100644
@@ -24,6 +24,11 @@
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_nat_amanda");
 
+static struct nf_ct_nat_helper amanda_nat = {
+	.name = "amanda_nat",
+	.expectfn = nf_nat_follow_master,
+};
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 unsigned int protoff,

@@ -41,7 +46,7 @@ static unsigned int help(struct sk_buff *skb,
 
 	/* When you see the packet, we need to NAT it the same as the
 	 * this one (ie. same IP: it will be TCP and master is UDP). */
-	exp->expectfn = nf_nat_follow_master;
+	exp->nat_helper = &amanda_nat;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {

@@ -76,12 +81,14 @@ static unsigned int help(struct sk_buff *skb,
 static void __exit nf_nat_amanda_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_amanda_hook, NULL);
+	nf_ct_nat_helper_unregister(&amanda_nat);
 	synchronize_rcu();
 }
 
 static int __init nf_nat_amanda_init(void)
 {
 	BUG_ON(nf_nat_amanda_hook != NULL);
+	nf_ct_nat_helper_register(&amanda_nat);
 	RCU_INIT_POINTER(nf_nat_amanda_hook, help);
 	return 0;
 }

net/netfilter/nf_nat_ftp.c +8 -1

index e84a578..c027d44 100644
@@ -24,6 +24,11 @@
 MODULE_DESCRIPTION("ftp NAT helper");
 MODULE_ALIAS("ip_nat_ftp");
 
+static struct nf_ct_nat_helper ftp_nat = {
+	.name = "ftp_nat",
+	.expectfn = nf_nat_follow_master,
+};
+
 /* FIXME: Time out? --RR */
 
 static int nf_nat_ftp_fmt_cmd(struct nf_conn *ct, enum nf_ct_ftp_type type,

@@ -80,7 +85,7 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 
 	/* When you see the packet, we need to NAT it the same as the
 	 * this one. */
-	exp->expectfn = nf_nat_follow_master;
+	exp->nat_helper = &ftp_nat;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {

@@ -123,12 +128,14 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 static void __exit nf_nat_ftp_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_ftp_hook, NULL);
+	nf_ct_nat_helper_unregister(&ftp_nat);
 	synchronize_rcu();
 }
 
 static int __init nf_nat_ftp_init(void)
 {
 	BUG_ON(nf_nat_ftp_hook != NULL);
+	nf_ct_nat_helper_register(&ftp_nat);
 	RCU_INIT_POINTER(nf_nat_ftp_hook, nf_nat_ftp);
 	return 0;
 }

net/netfilter/nf_nat_irc.c +8 -1

index 1fb2258..9eec7f3 100644
@@ -26,6 +26,11 @@
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_nat_irc");
 
+static struct nf_ct_nat_helper irc_nat = {
+	.name = "irc_nat",
+	.expectfn = nf_nat_follow_master,
+};
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 unsigned int protoff,

@@ -44,7 +49,7 @@ static unsigned int help(struct sk_buff *skb,
 
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->dir = IP_CT_DIR_REPLY;
-	exp->expectfn = nf_nat_follow_master;
+	exp->nat_helper = &irc_nat;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {

@@ -96,12 +101,14 @@ static unsigned int help(struct sk_buff *skb,
 static void __exit nf_nat_irc_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_irc_hook, NULL);
+	nf_ct_nat_helper_unregister(&irc_nat);
 	synchronize_rcu();
 }
 
 static int __init nf_nat_irc_init(void)
 {
 	BUG_ON(nf_nat_irc_hook != NULL);
+	nf_ct_nat_helper_register(&irc_nat);
 	RCU_INIT_POINTER(nf_nat_irc_hook, help);
 	return 0;
 }

net/netfilter/nf_nat_sip.c +10 -8

index d27c5a2..cd9333c 100644
@@ -28,6 +28,13 @@
 MODULE_DESCRIPTION("SIP NAT helper");
 MODULE_ALIAS("ip_nat_sip");
 
+static void nf_nat_sip_expected(struct nf_conn *ct,
+				struct nf_conntrack_expect *exp);
+
+static struct nf_ct_nat_helper sip_nat = {
+	.name		= "sip",
+	.expectfn	= nf_nat_sip_expected,
+};
 
 static unsigned int mangle_packet(struct sk_buff *skb, unsigned int protoff,
 				  unsigned int dataoff,

@@ -376,7 +383,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
 	exp->tuple.dst.u3 = newaddr;
 	exp->saved_proto.udp.port = exp->tuple.dst.u.udp.port;
 	exp->dir = !dir;
-	exp->expectfn = nf_nat_sip_expected;
+	exp->nat_helper = &sip_nat;
 
 	for (; port != 0; port++) {
 		int ret;

@@ -561,13 +568,13 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 	rtp_exp->tuple.dst.u3 = *rtp_addr;
 	rtp_exp->saved_proto.udp.port = rtp_exp->tuple.dst.u.udp.port;
 	rtp_exp->dir = !dir;
-	rtp_exp->expectfn = nf_nat_sip_expected;
+	rtp_exp->nat_helper = &sip_nat;
 
 	rtcp_exp->saved_addr = rtcp_exp->tuple.dst.u3;
 	rtcp_exp->tuple.dst.u3 = *rtp_addr;
 	rtcp_exp->saved_proto.udp.port = rtcp_exp->tuple.dst.u.udp.port;
 	rtcp_exp->dir = !dir;
-	rtcp_exp->expectfn = nf_nat_sip_expected;
+	rtcp_exp->nat_helper = &sip_nat;
 
 	/* Try to get same pair of ports: if not, try to change them. */
 	for (port = ntohs(rtp_exp->tuple.dst.u.udp.port);

@@ -618,11 +625,6 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 	return NF_DROP;
 }
 
-static struct nf_ct_nat_helper sip_nat = {
-	.name		= "sip",
-	.expectfn	= nf_nat_sip_expected,
-};
-
 static void __exit nf_nat_sip_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_sip_hooks, NULL);

net/netfilter/nf_nat_tftp.c +8 -1

index 7f67e1d..f84b937 100644
@@ -18,6 +18,11 @@
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ip_nat_tftp");
 
+static struct nf_ct_nat_helper tftp_nat = {
+	.name = "tftp_nat",
+	.expectfn = nf_nat_follow_master,
+};
+
 static unsigned int help(struct sk_buff *skb,
 			 enum ip_conntrack_info ctinfo,
 			 struct nf_conntrack_expect *exp)

@@ -27,7 +32,7 @@ static unsigned int help(struct sk_buff *skb,
 	exp->saved_proto.udp.port
 		= ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.udp.port;
 	exp->dir = IP_CT_DIR_REPLY;
-	exp->expectfn = nf_nat_follow_master;
+	exp->nat_helper = &tftp_nat;
 	if (nf_ct_expect_related(exp) != 0) {
 		nf_ct_helper_log(skb, exp->master, "cannot add expectation");
 		return NF_DROP;

@@ -38,12 +43,14 @@ static unsigned int help(struct sk_buff *skb,
 static void __exit nf_nat_tftp_fini(void)
 {
 	RCU_INIT_POINTER(nf_nat_tftp_hook, NULL);
+	nf_ct_nat_helper_unregister(&tftp_nat);
 	synchronize_rcu();
 }
 
 static int __init nf_nat_tftp_init(void)
 {
 	BUG_ON(nf_nat_tftp_hook != NULL);
+	nf_ct_nat_helper_register(&tftp_nat);
 	RCU_INIT_POINTER(nf_nat_tftp_hook, help);
 	return 0;
 }

Signature · 1 line

1.9.1

Pablo Neira Ayuso

maintainer

2017-04-14 00:11 UTC #3 lore raw

Re: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

net/netfilter/nf_conntrack_pptp.c line 106

Quoted 2 lines · On Fri, Mar 31, 2017 at 06:38:20PM +0800, [email protected] wrote:

net/netfilter/nf_conntrack_pptp.c +2 -0
+static struct nf_ct_nat_helper pptp_nat = {
+	.name			= "pptp_nat",

Why all these with "xyz_nat" names?

This is going to break ctnetlink, as this is the name that identifies the NAT helper to be used.

Quoted 16 lines

net/netfilter/nf_conntrack_pptp.c +4 -1

+	.expectfn		= pptp_expectfn,
+};
+
 static void pptp_expectfn(struct nf_conn *ct,
 			 struct nf_conntrack_expect *exp)
 {

@@ -221,7 +229,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &peer_callid, &callid);
-	exp_orig->expectfn = pptp_expectfn;
+	exp_orig->nat_helper = &pptp_nat;
 
 	/* reply direction, PAC->PNS */
 	dir = IP_CT_DIR_REPLY;

net/netfilter/nf_conntrack_pptp.c static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid) · line 241

Quoted 5 lines

net/netfilter/nf_conntrack_pptp.c +1 -1

@@ -230,7 +238,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &callid, &peer_callid);
-	exp_reply->expectfn = pptp_expectfn;
+	exp_reply->nat_helper = &pptp_nat;

Why do we need to attach nat_helper instead of expectfn? I would prefer we do not.

Gao Feng

author

2017-04-14 00:46 UTC #4 lore raw

RE: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

Quoted 7 lines

-----Original Message-----
From: Pablo Neira Ayuso [mailto:[email protected]]
net/netfilter/nf_conntrack_pptp.c line 106
Quoted 2 lines · On Fri, Mar 31, 2017 at 06:38:20PM +0800, [email protected] wrote:
net/netfilter/nf_conntrack_pptp.c +2 -0
+static struct nf_ct_nat_helper pptp_nat = {
+	.name			= "pptp_nat",
Why all these with "xyz_nat" names?

I just used the variable name before.
How about rename it to "xyz_nat_helper"?

Quoted 2 lines

This is going to break ctnetlink, as this is the name that identifies the

NAT

Quoted 9 lines

helper to be used.
net/netfilter/nf_conntrack_pptp.c static int exp_gre(struct nf_conn *ct, __be16
Quoted 7 lines
net/netfilter/nf_conntrack_pptp.c +3 -0
+	.expectfn		= pptp_expectfn,
+};
+
 static void pptp_expectfn(struct nf_conn *ct,
 			 struct nf_conntrack_expect *exp)
 {
@@ -221,7 +229,7 @@ static int exp_gre(struct nf_conn *ct, __be16

callid,

Quoted 10 lines

__be16 peer_callid)

net/netfilter/nf_conntrack_pptp.c static int exp_gre(struct nf_conn *ct, __be16

Quoted 9 lines

net/netfilter/nf_conntrack_pptp.c +1 -1

 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &peer_callid, &callid);
-	exp_orig->expectfn = pptp_expectfn;
+	exp_orig->nat_helper = &pptp_nat;
 
 	/* reply direction, PAC->PNS */
 	dir = IP_CT_DIR_REPLY;

@@ -230,7 +238,7 @@ static int exp_gre(struct nf_conn *ct, __be16

callid,

Quoted 9 lines

__be16 peer_callid)
net/netfilter/nf_conntrack_pptp.c static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid) · line 241
Quoted 5 lines
net/netfilter/nf_conntrack_pptp.c +1 -1
 			  &ct->tuplehash[dir].tuple.src.u3,
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &callid, &peer_callid);
-	exp_reply->expectfn = pptp_expectfn;
+	exp_reply->nat_helper = &pptp_nat;
Why do we need to attach nat_helper instead of expectfn? I would prefer we do not.

Because we need to introduce another member "expectfn_module" in the third patch.
If insisted on the current style, the codes would look like the following:

	exp_reply->expectfn = pptp_expectfn;
	exp_reply->expectfn_module = THIS_MODULE;

There are two assignments when using expect_fn.
It isn't only a little duplicated, but also it is easy to lost the module assignment and bring one bug.

If attached the nat_helper instead of expectfn, there would be only one assignment.

Regards
Feng

Pablo Neira Ayuso

maintainer

2017-04-14 01:02 UTC #5 lore raw

Re: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

Quoted 8 lines · Gao Feng

Quoted 7 lines
-----Original Message-----
From: Pablo Neira Ayuso [mailto:[email protected]]
net/netfilter/nf_conntrack_pptp.c line 106
Quoted 2 lines · On Fri, Mar 31, 2017 at 06:38:20PM +0800, [email protected] wrote:
net/netfilter/nf_conntrack_pptp.c +2 -0
+static struct nf_ct_nat_helper pptp_nat = {
+	.name			= "pptp_nat",
Why all these with "xyz_nat" names?

Quoted 6 lines

I just used the variable name before.
How about rename it to "xyz_nat_helper"?
Quoted 2 lines
This is going to break ctnetlink, as this is the name that identifies the
NAT
Quoted 1 line
helper to be used.

This name is exposed to userspace, right?

So FTP uses to rely on the "nat-follow-master" expectfn. But now the name will be "ftp_nat".

If that is the case, this would break backward.

Gao Feng

author

2017-04-17 00:50 UTC #6 lore raw

RE: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

Hi Pablo,

Quoted 15 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Pablo Neira Ayuso
Quoted 11 lines · Gao Feng
Quoted 7 lines
-----Original Message-----
From: Pablo Neira Ayuso [mailto:[email protected]] On Fri, Mar 31,
net/netfilter/nf_conntrack_pptp.c line 106
Quoted 2 lines · 2017 at 06:38:20PM +0800, [email protected] wrote:
net/netfilter/nf_conntrack_pptp.c +2 -0
+static struct nf_ct_nat_helper pptp_nat = {
+	.name			= "pptp_nat",
Why all these with "xyz_nat" names?
I just used the variable name before.
How about rename it to "xyz_nat_helper"?
Quoted 1 line

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
This is going to break ctnetlink, as this is the name that identifies the
NAT
Quoted 1 line
helper to be used.
This name is exposed to userspace, right?
So FTP uses to rely on the "nat-follow-master" expectfn. But now the name

will

Quoted 3 lines

be "ftp_nat".
If that is the case, this would break backward.

I am a little confusing.
The " struct nf_ct_nat_helper ftp_nat ftp_nat" is one new variable, while the original variable
"struct nf_ct_nat_helper follow_master_nat" name still is " "nat-follow-master".

I couldn't figure about how could this break backward.
Do you think the name " ftp_nat " should use " ftp_nat_follow_master"?

BTW, how about use nat_helper pointer instead of expect fn? As I mentioned before, it could avoid two assignment. Is it ok?

Regards
Feng

Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel"

in the

Quoted 2 lines

body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

[email protected]

author

2017-03-31 10:38 UTC #7 lore raw

PATCH v5 3/3 netfilter: nat_helper: Remove the expectations when its module is unloaded

In reply to [email protected]

From: Gao Feng <[email protected]>

Because the conntrack NAT module could be rmmod anytime, so we should really leave things in clean state if such thing happens and make sure we don't leave any packet running over code that will be gone after the removal.

We only removed the expectations when unregister conntrack helper before. Actually it is necessary too when remove the nat helper.

Now add one new struct module member "me" in the nf_ct_nat_helper to represent which module it belongs to.

Signed-off-by Gao Feng <[email protected]>

 include/net/netfilter/nf_conntrack_helper.h |  1 +
 net/ipv4/netfilter/nf_nat_h323.c            |  3 ++
 net/netfilter/ipvs/ip_vs_nfct.c             |  1 +
 net/netfilter/nf_conntrack_expect.c         |  8 ++++
 net/netfilter/nf_conntrack_helper.c         | 62 +++++++++++++++++++----------
 net/netfilter/nf_conntrack_pptp.c           |  1 +
 net/netfilter/nf_nat_amanda.c               |  1 +
 net/netfilter/nf_nat_core.c                 |  1 +
 net/netfilter/nf_nat_ftp.c                  |  1 +
 net/netfilter/nf_nat_irc.c                  |  1 +
 net/netfilter/nf_nat_sip.c                  |  1 +
 net/netfilter/nf_nat_tftp.c                 |  1 +
 12 files changed, 60 insertions(+), 22 deletions(-)

include/net/netfilter/nf_conntrack_helper.h +1 -0

index e8d31ca..d70c5be 100644
@@ -114,6 +114,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff,
 struct nf_ct_nat_helper {
 	struct list_head head;
 	const char *name;
+	struct module *me;
 	void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp);
 };

net/ipv4/netfilter/nf_nat_h323.c +3 -0

index 9101c48..c75583f 100644
@@ -28,16 +28,19 @@ static void ip_nat_callforwarding_expect(struct nf_conn *new,
 
 static struct nf_ct_nat_helper q931_nat = {
 	.name		= "Q.931",
+	.me		= THIS_MODULE,
 	.expectfn	= ip_nat_q931_expect,
 };
 
 static struct nf_ct_nat_helper callforwarding_nat = {
 	.name		= "callforwarding",
+	.me		= THIS_MODULE,
 	.expectfn	= ip_nat_callforwarding_expect,
 };
 
 static struct nf_ct_nat_helper follow_master_nat = {
 	.name		= "h323_follow_master",
+	.me		= THIS_MODULE,
 	.expectfn	= nf_nat_follow_master,
 };

net/netfilter/ipvs/ip_vs_nfct.c +1 -0

index 83ad79f..44cd98e 100644
@@ -222,6 +222,7 @@ static void ip_vs_nfct_expect_callback(struct nf_conn *ct,
 
 static struct nf_ct_nat_helper ip_vs_nat = {
 	.name = "ip_vs_nat",
+	.me = THIS_MODULE,
 	.expectfn = ip_vs_nfct_expect_callback,
 };

net/netfilter/nf_conntrack_expect.c +8 -0

index ba5a55e..9653847 100644
@@ -416,6 +416,14 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect)
 		ret = -ESHUTDOWN;
 		goto out;
 	}
+
+	/* Make sure the helper and nat_helper belong to the same module */
+	if (expect->helper && expect->nat_helper &&
+	    expect->helper->me != expect->nat_helper->me) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	h = nf_ct_expect_dst_hash(net, &expect->tuple);
 	hlist_for_each_entry_safe(i, next, &nf_ct_expect_hash[h], hnode) {
 		if (expect_matches(i, expect)) {

net/netfilter/nf_conntrack_helper.c +40 ~~-22~~

88 lines not on this page · raw message

net/netfilter/nf_conntrack_pptp.c +1 -0

index 5b5a45e..9d4a3e0 100644
@@ -104,6 +104,7 @@ static void pptp_expectfn(struct nf_conn *ct,
 
 static struct nf_ct_nat_helper pptp_nat = {
 	.name			= "pptp_nat",
+	.me			= THIS_MODULE,
 	.expectfn		= pptp_expectfn,
 };

net/netfilter/nf_nat_amanda.c +1 -0

index 05cd3fb..7263a16 100644
@@ -26,6 +26,7 @@
 
 static struct nf_ct_nat_helper amanda_nat = {
 	.name = "amanda_nat",
+	.me = THIS_MODULE,
 	.expectfn = nf_nat_follow_master,
 };

net/netfilter/nf_nat_core.c +1 -0

index 2897bd4..ae1789b 100644
@@ -850,6 +850,7 @@ static void __net_exit nf_nat_net_exit(struct net *net)
 
 static struct nf_ct_nat_helper follow_master_nat = {
 	.name		= "nat-follow-master",
+	.me		= THIS_MODULE,
 	.expectfn	= nf_nat_follow_master,
 };

net/netfilter/nf_nat_ftp.c +1 -0

index c027d44..90beafa 100644
@@ -26,6 +26,7 @@
 
 static struct nf_ct_nat_helper ftp_nat = {
 	.name = "ftp_nat",
+	.me = THIS_MODULE,
 	.expectfn = nf_nat_follow_master,
 };

net/netfilter/nf_nat_irc.c +1 -0

index 9eec7f3..ddc3f66 100644
@@ -28,6 +28,7 @@
 
 static struct nf_ct_nat_helper irc_nat = {
 	.name = "irc_nat",
+	.me = THIS_MODULE,
 	.expectfn = nf_nat_follow_master,
 };

net/netfilter/nf_nat_sip.c +1 -0

index cd9333c..d13aae6 100644
@@ -33,6 +33,7 @@ static void nf_nat_sip_expected(struct nf_conn *ct,
 
 static struct nf_ct_nat_helper sip_nat = {
 	.name		= "sip",
+	.me		= THIS_MODULE,
 	.expectfn	= nf_nat_sip_expected,
 };

net/netfilter/nf_nat_tftp.c +1 -0

index f84b937..eb183dc 100644
@@ -20,6 +20,7 @@
 
 static struct nf_ct_nat_helper tftp_nat = {
 	.name = "tftp_nat",
+	.me = THIS_MODULE,
 	.expectfn = nf_nat_follow_master,
 };

Signature · 1 line

1.9.1