Skip to contentSkip to conversation

Fix invoking expectfn unloaded

PATCH In review [email protected] 3 participants 5 messages Mar 21, 2017

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

[email protected]

author
#0 lore raw

v3 cover Fix invoking expectfn unloaded

From: Gao Feng <[email protected]>

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            | 14 +++--
 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_nat_core.c                 |  7 ++-
 net/netfilter/nf_nat_sip.c                  |  7 ++-
 9 files changed, 95 insertions(+), 68 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v3 1/2 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]>

---
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 6806b5e..66817a5 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 94b14c5..eae9bec 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
author
#2 lore raw

PATCH v3 2/2 netfilter: helper: Fix possible panic caused by invoking expectfn 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.

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

---
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            |  2 +
 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_nat_core.c                 |  1 +
 net/netfilter/nf_nat_sip.c                  |  1 +
 9 files changed, 52 insertions(+), 25 deletions(-)
include/net/netfilter/nf_conntrack_expect.h +2 -0
index 5ed33ea..f2bc64e 100644
@@ -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;
include/net/netfilter/nf_conntrack_helper.h +1 -0
index d14fe493..dd1a687 100644
@@ -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);
 };
net/ipv4/netfilter/nf_nat_h323.c +2 -0
index 346e764..4c8eef6 100644
@@ -569,11 +569,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,
 };
net/netfilter/nf_conntrack_broadcast.c +1 -0
index 4e99cca..ca54735 100644
@@ -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;
net/netfilter/nf_conntrack_expect.c +1 -0
index 4b2e1fb..b6d081a 100644
@@ -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;
net/netfilter/nf_conntrack_helper.c +39 -24

87 lines not on this page · raw message

net/netfilter/nf_conntrack_netlink.c +4 -1
index 66817a5..50628aa 100644
@@ -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;
net/netfilter/nf_nat_core.c +1 -0
index eae9bec..f337208 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_sip.c +1 -0
index d27c5a2..4095a45 100644
@@ -620,6 +620,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,
 };
Signature · 1 line
1.9.1
maintainer
#3 lore raw

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

Quoted 9 lines · On Tue, Mar 21, 2017 at 02:06:26PM +0800, [email protected] wrote:

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.

We still need something like this, right?

net/netfilter/nf_nat_sip.c +0 -0
index 791fac4fd745..38f1ed5219e4 100644
@@ -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;

So if the user invokes:

rmmod nf_nat_sip

we can get rid of this expectation from nf_ct_flush_expect().

#4 lore raw

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

Hi Pablo,

Quoted 3 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Pablo Neira

Ayuso

Quoted 4 lines

Sent: Tuesday, March 21, 2017 10:13 PM
To: [email protected]
Cc: [email protected]; [email protected]
Subject: Re: [PATCH nf v3 2/2] netfilter: helper: Fix possible panic

caused by

Quoted 11 lines

invoking expectfn unloaded

Quoted 8 lines · On Tue, Mar 21, 2017 at 02:06:26PM +0800, [email protected] wrote:

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.

Quoted 3 lines
Quoted 1 line

Actually it is necessary too when remove the nat helper.

We still need something like this, right?

Yes, we need to remove expect when remove helper and nat_helper.

Quoted 15 lines
net/netfilter/nf_nat_sip.c index renamed from net/netfilter/nf_nat_sip.c +0 -0

791fac4fd745..38f1ed5219e4 100644

net/netfilter/nf_nat_sip.c +1 -0
@@ -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;
Quoted 5 lines

So if the user invokes:

rmmod nf_nat_sip

we can get rid of this expectation from nf_ct_flush_expect().

Oh, I missed the data path, only covered the ctlink codes. I would update the patch again.

Best 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