[PATCH nf v4 0/2] Fix invoking expectfn unloaded

[PATCH nf v4 0/2] Fix invoking expectfn unloaded

[gfree.wind]

From: Gao Feng <fgao@ikuai8.com>

It is possible that invoke one expectfn whose module is already unloaded.
These two patches are used to fix it.

Gao Feng (2):
  netfilter: helper: Rename struct nf_ct_helper_expectfn to
    nf_ct_nat_helper
  netfilter: helper: Fix possible panic caused by invoking expectfn
    unloaded

 include/net/netfilter/nf_conntrack_expect.h |  2 +
 include/net/netfilter/nf_conntrack_helper.h | 15 ++---
 net/ipv4/netfilter/nf_nat_h323.c            | 20 ++++--
 net/netfilter/nf_conntrack_broadcast.c      |  1 +
 net/netfilter/nf_conntrack_expect.c         |  1 +
 net/netfilter/nf_conntrack_helper.c         | 95 +++++++++++++++++------------
 net/netfilter/nf_conntrack_netlink.c        | 21 ++++---
 net/netfilter/nf_conntrack_pptp.c           |  2 +
 net/netfilter/nf_nat_amanda.c               |  1 +
 net/netfilter/nf_nat_core.c                 |  7 ++-
 net/netfilter/nf_nat_ftp.c                  |  1 +
 net/netfilter/nf_nat_irc.c                  |  1 +
 net/netfilter/nf_nat_sip.c                  | 10 ++-
 net/netfilter/nf_nat_tftp.c                 |  1 +
 14 files changed, 110 insertions(+), 68 deletions(-)

-- 
1.9.1

[PATCH nf v4 1/2] netfilter: helper: Rename struct nf_ct_helper_expectfn to nf_ct_nat_helper

[gfree.wind]

From: Gao Feng <fgao@ikuai8.com>

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 <fgao@ikuai8.com>
---
 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(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 1eaac1f..d14fe493 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -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;
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7eb..346e764 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -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();
 }
 
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..0eaa01e 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -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,
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 6806b5e..66817a5 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -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;
 
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 94b14c5..eae9bec 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -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);
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index 791fac4..d27c5a2 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -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;
 }
 
-- 
1.9.1

[PATCH nf v4 2/2] netfilter: helper: Fix possible panic caused by invoking expectfn unloaded

[gfree.wind]

From: Gao Feng <fgao@ikuai8.com>

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.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 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_expect.h |  2 +
 include/net/netfilter/nf_conntrack_helper.h |  1 +
 net/ipv4/netfilter/nf_nat_h323.c            |  8 ++++
 net/netfilter/nf_conntrack_broadcast.c      |  1 +
 net/netfilter/nf_conntrack_expect.c         |  1 +
 net/netfilter/nf_conntrack_helper.c         | 63 ++++++++++++++++++-----------
 net/netfilter/nf_conntrack_netlink.c        |  5 ++-
 net/netfilter/nf_conntrack_pptp.c           |  2 +
 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                  |  4 ++
 net/netfilter/nf_nat_tftp.c                 |  1 +
 14 files changed, 67 insertions(+), 25 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
index 5ed33ea..f2bc64e 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -26,6 +26,8 @@ struct nf_conntrack_expect {
 	/* Function to call after setup and insertion */
 	void (*expectfn)(struct nf_conn *new,
 			 struct nf_conntrack_expect *this);
+	/* The module which expectfn belongs to */
+	struct module *nat_module;
 
 	/* Helper to assign to new connection */
 	struct nf_conntrack_helper *helper;
diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index d14fe493..dd1a687 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -113,6 +113,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb, unsigned int protoff,
 
 struct nf_ct_nat_helper {
 	struct list_head head;
+	struct module *me;
 	const char *name;
 	void (*expectfn)(struct nf_conn *ct, struct nf_conntrack_expect *exp);
 };
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 346e764..df57219 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -188,9 +188,11 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
 	/* Set expectations for NAT */
 	rtp_exp->saved_proto.udp.port = rtp_exp->tuple.dst.u.udp.port;
 	rtp_exp->expectfn = nf_nat_follow_master;
+	rtp_exp->nat_module = THIS_MODULE;
 	rtp_exp->dir = !dir;
 	rtcp_exp->saved_proto.udp.port = rtcp_exp->tuple.dst.u.udp.port;
 	rtcp_exp->expectfn = nf_nat_follow_master;
+	rtcp_exp->nat_module = THIS_MODULE;
 	rtcp_exp->dir = !dir;
 
 	/* Lookup existing expects */
@@ -290,6 +292,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
 	/* Set expectations for NAT */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = nf_nat_follow_master;
+	exp->nat_module = THIS_MODULE;
 	exp->dir = !dir;
 
 	/* Try to get same port: if not, try to change it. */
@@ -342,6 +345,7 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
 	/* Set expectations for NAT */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = nf_nat_follow_master;
+	exp->nat_module = THIS_MODULE;
 	exp->dir = !dir;
 
 	/* Check existing expects */
@@ -434,6 +438,7 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
 	/* Set expectations for NAT */
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = ip_nat_q931_expect;
+	exp->nat_module = THIS_MODULE;
 	exp->dir = !dir;
 
 	/* Check existing expects */
@@ -528,6 +533,7 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
 	exp->tuple.dst.u3.ip = ct->tuplehash[!dir].tuple.dst.u3.ip;
 	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
 	exp->expectfn = ip_nat_callforwarding_expect;
+	exp->nat_module = THIS_MODULE;
 	exp->dir = !dir;
 
 	/* Try to get same port: if not, try to change it. */
@@ -569,11 +575,13 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
 
 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,
 };
 
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index 4e99cca..ca54735 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -66,6 +66,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
 	exp->mask.src.u.udp.port  = htons(0xFFFF);
 
 	exp->expectfn             = NULL;
+	exp->nat_module           = NULL;
 	exp->flags                = NF_CT_EXPECT_PERMANENT;
 	exp->class		  = NF_CT_EXPECT_CLASS_DEFAULT;
 	exp->helper               = NULL;
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 4b2e1fb..b6d081a 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -296,6 +296,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_module = NULL;
 	exp->helper = NULL;
 	exp->tuple.src.l3num = family;
 	exp->tuple.dst.protonum = proto;
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 0eaa01e..c25c9be 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -130,6 +130,42 @@ static unsigned int helper_hash(const struct nf_conntrack_tuple *tuple)
 	return NULL;
 }
 
+static void
+nf_ct_flush_expect(const struct module *me)
+{
+	struct nf_conntrack_expect *exp;
+	const struct hlist_node *next;
+	u32 i;
+
+	if (!me)
+		return;
+
+	/* Make sure no one is still using the module unless
+	 * its a connection in the hash.
+	 */
+	synchronize_rcu();
+
+	/* Get rid of expectations */
+	spin_lock_bh(&nf_conntrack_expect_lock);
+	for (i = 0; i < nf_ct_expect_hsize; i++) {
+		hlist_for_each_entry_safe(exp, next,
+					  &nf_ct_expect_hash[i], hnode) {
+			struct nf_conn_help *master_help = nfct_help(exp->master);
+
+			if ((master_help->helper && master_help->helper->me == me) ||
+			    (exp->helper && exp->helper->me == me) ||
+			     exp->nat_module == me) {
+				/* This expect belongs to the dying module */
+				if (del_timer(&exp->timeout)) {
+					nf_ct_unlink_expect(exp);
+					nf_ct_expect_put(exp);
+				}
+			}
+		}
+	}
+	spin_unlock_bh(&nf_conntrack_expect_lock);
+}
+
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find(const char *name, u16 l3num, u8 protonum)
 {
@@ -308,6 +344,8 @@ 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);
+
+	nf_ct_flush_expect(n->me);
 }
 EXPORT_SYMBOL_GPL(nf_ct_nat_helper_unregister);
 
@@ -421,8 +459,6 @@ static void __nf_conntrack_helper_unregister(struct nf_conntrack_helper *me,
 void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 {
 	struct nf_conntrack_tuple_hash *h;
-	struct nf_conntrack_expect *exp;
-	const struct hlist_node *next;
 	const struct hlist_nulls_node *nn;
 	unsigned int last_hsize;
 	spinlock_t *lock;
@@ -434,28 +470,7 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	nf_ct_helper_count--;
 	mutex_unlock(&nf_ct_helper_mutex);
 
-	/* Make sure every nothing is still using the helper unless its a
-	 * connection in the hash.
-	 */
-	synchronize_rcu();
-
-	/* Get rid of expectations */
-	spin_lock_bh(&nf_conntrack_expect_lock);
-	for (i = 0; i < nf_ct_expect_hsize; i++) {
-		hlist_for_each_entry_safe(exp, next,
-					  &nf_ct_expect_hash[i], hnode) {
-			struct nf_conn_help *help = nfct_help(exp->master);
-			if ((rcu_dereference_protected(
-					help->helper,
-					lockdep_is_held(&nf_conntrack_expect_lock)
-					) == me || exp->helper == me) &&
-			    del_timer(&exp->timeout)) {
-				nf_ct_unlink_expect(exp);
-				nf_ct_expect_put(exp);
-			}
-		}
-	}
-	spin_unlock_bh(&nf_conntrack_expect_lock);
+	nf_ct_flush_expect(me->me);
 
 	rtnl_lock();
 	for_each_net(net)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 66817a5..50628aa 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -3078,8 +3078,11 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
 			goto err_out;
 		}
 		exp->expectfn = nat_helper->expectfn;
-	} else
+		exp->nat_module = nat_helper->me;
+	} else {
 		exp->expectfn = NULL;
+		exp->nat_module = NULL;
+	}
 
 	exp->class = class;
 	exp->master = ct;
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index f60a475..372dd7f 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -222,6 +222,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &peer_callid, &callid);
 	exp_orig->expectfn = pptp_expectfn;
+	exp_orig->nat_module = THIS_MODULE;
 
 	/* reply direction, PAC->PNS */
 	dir = IP_CT_DIR_REPLY;
@@ -231,6 +232,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, __be16 peer_callid)
 			  &ct->tuplehash[dir].tuple.dst.u3,
 			  IPPROTO_GRE, &callid, &peer_callid);
 	exp_reply->expectfn = pptp_expectfn;
+	exp_reply->nat_module = THIS_MODULE;
 
 	nf_nat_pptp_exp_gre = rcu_dereference(nf_nat_pptp_hook_exp_gre);
 	if (nf_nat_pptp_exp_gre && ct->status & IPS_NAT_MASK)
diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c
index eb77238..fad4363 100644
--- a/net/netfilter/nf_nat_amanda.c
+++ b/net/netfilter/nf_nat_amanda.c
@@ -42,6 +42,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_module = THIS_MODULE;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index eae9bec..f337208 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -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,
 };
 
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index e84a578..d004d49 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -81,6 +81,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_module = THIS_MODULE;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
index 1fb2258..34b2119 100644
--- a/net/netfilter/nf_nat_irc.c
+++ b/net/netfilter/nf_nat_irc.c
@@ -45,6 +45,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_module = THIS_MODULE;
 
 	/* Try to get same port: if not, try to change it. */
 	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
diff --git a/net/netfilter/nf_nat_sip.c b/net/netfilter/nf_nat_sip.c
index d27c5a2..208d4d3 100644
--- a/net/netfilter/nf_nat_sip.c
+++ b/net/netfilter/nf_nat_sip.c
@@ -377,6 +377,7 @@ static unsigned int nf_nat_sip_expect(struct sk_buff *skb, unsigned int protoff,
 	exp->saved_proto.udp.port = exp->tuple.dst.u.udp.port;
 	exp->dir = !dir;
 	exp->expectfn = nf_nat_sip_expected;
+	exp->nat_module = THIS_MODULE;
 
 	for (; port != 0; port++) {
 		int ret;
@@ -562,12 +563,14 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 	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_module = THIS_MODULE;
 
 	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_module = THIS_MODULE;
 
 	/* Try to get same pair of ports: if not, try to change them. */
 	for (port = ntohs(rtp_exp->tuple.dst.u.udp.port);
@@ -620,6 +623,7 @@ static unsigned int nf_nat_sdp_media(struct sk_buff *skb, unsigned int protoff,
 
 static struct nf_ct_nat_helper sip_nat = {
 	.name		= "sip",
+	.me		= THIS_MODULE,
 	.expectfn	= nf_nat_sip_expected,
 };
 
diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c
index 7f67e1d..865ea04 100644
--- a/net/netfilter/nf_nat_tftp.c
+++ b/net/netfilter/nf_nat_tftp.c
@@ -28,6 +28,7 @@ static unsigned int help(struct sk_buff *skb,
 		= 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_module = THIS_MODULE;
 	if (nf_ct_expect_related(exp) != 0) {
 		nf_ct_helper_log(skb, exp->master, "cannot add expectation");
 		return NF_DROP;
-- 
1.9.1

Re: [PATCH nf v4 2/2] netfilter: helper: Fix possible panic caused by invoking expectfn unloaded

[Pablo Neira Ayuso]

Hi Feng,

Still two concerns with this.

On Wed, Mar 22, 2017 at 09:03:24AM +0800, gfree.wind@foxmail.com wrote:
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 0eaa01e..c25c9be 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -130,6 +130,42 @@ static unsigned int helper_hash(const struct nf_conntrack_tuple *tuple)
>  	return NULL;
>  }
>  
> +static void
> +nf_ct_flush_expect(const struct module *me)
> +{
> +	struct nf_conntrack_expect *exp;
> +	const struct hlist_node *next;
> +	u32 i;
> +
> +	if (!me)
> +		return;
> +
> +	/* Make sure no one is still using the module unless
> +	 * its a connection in the hash.
> +	 */
> +	synchronize_rcu();

I think it's more readable if you keep this synchronize_rcu() call in
nf_conntrack_helper_unregister() and nf_ct_nat_helper_unregister()
respectively, before calling nf_ct_flush_expect().

See below more reasons for this change I'm requesting at the end of
this email.

> +	/* Get rid of expectations */
> +	spin_lock_bh(&nf_conntrack_expect_lock);
> +	for (i = 0; i < nf_ct_expect_hsize; i++) {
> +		hlist_for_each_entry_safe(exp, next,
> +					  &nf_ct_expect_hash[i], hnode) {
> +			struct nf_conn_help *master_help = nfct_help(exp->master);
> +
> +			if ((master_help->helper && master_help->helper->me == me) ||

There used to be a rcu_dereference_protected() here to fetch
help->helper that now is gone.

> +			    (exp->helper && exp->helper->me == me) ||

Can we really have exp->helper set to NULL or you're just being
defensive here? I think all expectations are guaranteed to have a
exp->helper.

> +			     exp->nat_module == me) {
> +				/* This expect belongs to the dying module */
> +				if (del_timer(&exp->timeout)) {
> +					nf_ct_unlink_expect(exp);
> +					nf_ct_expect_put(exp);
> +				}
> +			}
> +		}
> +	}
> +	spin_unlock_bh(&nf_conntrack_expect_lock);
> +}
> +
>  struct nf_conntrack_helper *
>  __nf_conntrack_helper_find(const char *name, u16 l3num, u8 protonum)
>  {
[...]
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index eae9bec..f337208 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -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,

Look, this follow_master_nat structure belongs to nf_nat_core. I think
expectations using this will not suffer from the problem you describe
in this patch, given expectfn() will still be there.

However, with your patch I think two different helpers using
nat-follow-master will get both of their expectations removed if one
their nat modules is remove given that:

        exp->nat_module == me

will stand true since THIS_MODULE points to nf_nat core module for
nat-follow-master.

You mentioned another problem here that is: We currently allow to set
*any* expectfn to expectation and that is wrong. I think we need to
extend this nf_ct_nat_helper structure to make it look like:

static struct nf_ct_nat_helper irc_nat = {
        .name           = "irc",
        .expectfn	= "nat-follow-master", /* this used to be .name before */
        .me		= THIS_MODULE,
};

So we register one of this nf_ct_nat_helper structures per module.
Thus, we have a 1:1 mapping between nf_ct_nat_helper structure and
modules that we need to:

1) Fix this problem you describe in this patch.
2) Don't allow setting expectfn of h323 to a irc expectation using
   ctnetlink.

I suggest you send revamp this batch with patches to:

1) Rename nf_ct_helper_expectfn to nf_ct_nat_helper, no changes there
   just like your v4 1/2.
2) Register one nf_ct_nat_helper structure per NAT helper module.
   Validate from ctnetlink that we don't attach the wrong expectfn to
   the expectation we create there. This would be a new patch that
   introduces the 1:1 mapping between NAT modules and struct
   nf_ct_nat_helper.
3) Fix possible panic caused by removing NAT module (I'm refering to this
   patch 2/2). Now that we have the 1:1 mapping, we don't accidentally
   remove expectations that use nat-follow-master.

Let me know,
Thanks!

RE: [PATCH nf v4 2/2] netfilter: helper: Fix possible panic caused by invoking expectfn unloaded

[Gao Feng]

Hi Pablo,

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org
> [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Pablo Neira
Ayuso
> Sent: Wednesday, March 29, 2017 5:54 PM
> To: gfree.wind@foxmail.com
> Cc: netfilter-devel@vger.kernel.org; Gao Feng <fgao@ikuai8.com>
> Subject: Re: [PATCH nf v4 2/2] netfilter: helper: Fix possible panic
caused by
> invoking expectfn unloaded
> 
> Hi Feng,
> 
> Still two concerns with this.
> 
> On Wed, Mar 22, 2017 at 09:03:24AM +0800, gfree.wind@foxmail.com wrote:
> > diff --git a/net/netfilter/nf_conntrack_helper.c
> > b/net/netfilter/nf_conntrack_helper.c
> > index 0eaa01e..c25c9be 100644
> > --- a/net/netfilter/nf_conntrack_helper.c
> > +++ b/net/netfilter/nf_conntrack_helper.c
> > @@ -130,6 +130,42 @@ static unsigned int helper_hash(const struct
> nf_conntrack_tuple *tuple)
> >  	return NULL;
> >  }
> >
> > +static void
> > +nf_ct_flush_expect(const struct module *me) {
> > +	struct nf_conntrack_expect *exp;
> > +	const struct hlist_node *next;
> > +	u32 i;
> > +
> > +	if (!me)
> > +		return;
> > +
> > +	/* Make sure no one is still using the module unless
> > +	 * its a connection in the hash.
> > +	 */
> > +	synchronize_rcu();
> 
> I think it's more readable if you keep this synchronize_rcu() call in
> nf_conntrack_helper_unregister() and nf_ct_nat_helper_unregister()
> respectively, before calling nf_ct_flush_expect().
> 
> See below more reasons for this change I'm requesting at the end of this
email.
> 
> > +	/* Get rid of expectations */
> > +	spin_lock_bh(&nf_conntrack_expect_lock);
> > +	for (i = 0; i < nf_ct_expect_hsize; i++) {
> > +		hlist_for_each_entry_safe(exp, next,
> > +					  &nf_ct_expect_hash[i], hnode) {
> > +			struct nf_conn_help *master_help =
nfct_help(exp->master);
> > +
> > +			if ((master_help->helper && master_help->helper->me
== me) ||
> 
> There used to be a rcu_dereference_protected() here to fetch
> help->helper that now is gone.

I have one question about it.
We have hold the nf_conntrack_expect_lock here, so still need the
rcu_dereference_protected?

> 
> > +			    (exp->helper && exp->helper->me == me) ||
> 
> Can we really have exp->helper set to NULL or you're just being defensive
here?
> I think all expectations are guaranteed to have a
> exp->helper.
When the expect is created by ctlink, the expectfn is valid with helper is
NULL according to the ctnetlink_alloc_expect.
So I add this check.

> 
> > +			     exp->nat_module == me) {
> > +				/* This expect belongs to the dying module
*/
> > +				if (del_timer(&exp->timeout)) {
> > +					nf_ct_unlink_expect(exp);
> > +					nf_ct_expect_put(exp);
> > +				}
> > +			}
> > +		}
> > +	}
> > +	spin_unlock_bh(&nf_conntrack_expect_lock);
> > +}
> > +
> >  struct nf_conntrack_helper *
> >  __nf_conntrack_helper_find(const char *name, u16 l3num, u8 protonum)
> > {
> [...]
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index eae9bec..f337208 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -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,
> 
> Look, this follow_master_nat structure belongs to nf_nat_core. I think
> expectations using this will not suffer from the problem you describe in
this
> patch, given expectfn() will still be there.
> 
> However, with your patch I think two different helpers using
nat-follow-master
> will get both of their expectations removed if one their nat modules is
remove
> given that:
> 
>         exp->nat_module == me
> 
> will stand true since THIS_MODULE points to nf_nat core module for
> nat-follow-master.
> 
> You mentioned another problem here that is: We currently allow to set
> *any* expectfn to expectation and that is wrong. I think we need to extend
this
> nf_ct_nat_helper structure to make it look like:
> 
> static struct nf_ct_nat_helper irc_nat = {
>         .name           = "irc",
>         .expectfn	= "nat-follow-master", /* this used to be .name
before */
>         .me		= THIS_MODULE,
> };
> 
> So we register one of this nf_ct_nat_helper structures per module.
> Thus, we have a 1:1 mapping between nf_ct_nat_helper structure and
> modules that we need to:
> 
> 1) Fix this problem you describe in this patch.
> 2) Don't allow setting expectfn of h323 to a irc expectation using
>    ctnetlink.
> 
> I suggest you send revamp this batch with patches to:
> 
> 1) Rename nf_ct_helper_expectfn to nf_ct_nat_helper, no changes there
>    just like your v4 1/2.
> 2) Register one nf_ct_nat_helper structure per NAT helper module.
>    Validate from ctnetlink that we don't attach the wrong expectfn to
>    the expectation we create there. This would be a new patch that
>    introduces the 1:1 mapping between NAT modules and struct
>    nf_ct_nat_helper.
> 3) Fix possible panic caused by removing NAT module (I'm refering to this
>    patch 2/2). Now that we have the 1:1 mapping, we don't accidentally
>    remove expectations that use nat-follow-master.

Ok, I would implement this solution.

Best Regards
Feng

> 
> Let me know,
> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel"
in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html