[NETFILTER]: ip_conntrack: fix NAT helper unload races

The NAT helpr hooks are protected by RCU, but all of the
conntrack helpers test and use the global pointers instead
of copying them first using rcu_dereference()

Also replace synchronize_net() by synchronize_rcu() for clarity
since sychronizing only with packet receive processing is
insufficient to prevent races.
Signed-off-by: Patrick McHardy <kaber@trash.net>

net/ipv4/netfilter/ip_conntrack_amanda.c

@@ -92,6 +92,7 @@ static int help(struct sk_buff **pskb,
 	char pbuf[sizeof("65535")], *tmp;
 	u_int16_t port, len;
 	int ret = NF_ACCEPT;
+	typeof(ip_nat_amanda_hook) ip_nat_amanda;
 
 	/* Only look at packets from the Amanda server */
 	if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
@@ -161,9 +162,11 @@ static int help(struct sk_buff **pskb,
 		exp->mask.dst.protonum = 0xFF;
 		exp->mask.dst.u.tcp.port = htons(0xFFFF);
 
-		if (ip_nat_amanda_hook)
-			ret = ip_nat_amanda_hook(pskb, ctinfo, off - dataoff,
-						 len, exp);
+		/* RCU read locked by nf_hook_slow */
+		ip_nat_amanda = rcu_dereference(ip_nat_amanda_hook);
+		if (ip_nat_amanda)
+			ret = ip_nat_amanda(pskb, ctinfo, off - dataoff,
+					    len, exp);
 		else if (ip_conntrack_expect_related(exp) != 0)
 			ret = NF_DROP;
 		ip_conntrack_expect_put(exp);

net/ipv4/netfilter/ip_conntrack_ftp.c

@@ -310,6 +310,7 @@ static int help(struct sk_buff **pskb,
 	struct ip_conntrack_expect *exp;
 	unsigned int i;
 	int found = 0, ends_in_nl;
+	typeof(ip_nat_ftp_hook) ip_nat_ftp;
 
 	/* Until there's been traffic both ways, don't look in packets. */
 	if (ctinfo != IP_CT_ESTABLISHED
@@ -433,9 +434,10 @@ static int help(struct sk_buff **pskb,
 
 	/* Now, NAT might want to mangle the packet, and register the
 	 * (possibly changed) expectation itself. */
-	if (ip_nat_ftp_hook)
-		ret = ip_nat_ftp_hook(pskb, ctinfo, search[dir][i].ftptype,
-				      matchoff, matchlen, exp, &seq);
+	ip_nat_ftp = rcu_dereference(ip_nat_ftp_hook);
+	if (ip_nat_ftp)
+		ret = ip_nat_ftp(pskb, ctinfo, search[dir][i].ftptype,
+				 matchoff, matchlen, exp, &seq);
 	else {
 		/* Can't expect this?  Best to drop packet now. */
 		if (ip_conntrack_expect_related(exp) != 0)

net/ipv4/netfilter/ip_conntrack_helper_pptp.c

@@ -124,6 +124,8 @@ EXPORT_SYMBOL(pptp_msg_name);
 static void pptp_expectfn(struct ip_conntrack *ct,
 			 struct ip_conntrack_expect *exp)
 {
+	typeof(ip_nat_pptp_hook_expectfn) ip_nat_pptp_expectfn;
+
 	DEBUGP("increasing timeouts\n");
 
 	/* increase timeout of GRE data channel conntrack entry */
@@ -133,7 +135,9 @@ static void pptp_expectfn(struct ip_conntrack *ct,
 	/* Can you see how rusty this code is, compared with the pre-2.6.11
 	 * one? That's what happened to my shiny newnat of 2002 ;( -HW */
 
-	if (!ip_nat_pptp_hook_expectfn) {
+	rcu_read_lock();
+	ip_nat_pptp_expectfn = rcu_dereference(ip_nat_pptp_hook_expectfn);
+	if (!ip_nat_pptp_expectfn) {
 		struct ip_conntrack_tuple inv_t;
 		struct ip_conntrack_expect *exp_other;
 
@@ -153,8 +157,9 @@ static void pptp_expectfn(struct ip_conntrack *ct,
 		}
 	} else {
 		/* we need more than simple inversion */
-		ip_nat_pptp_hook_expectfn(ct, exp);
+		ip_nat_pptp_expectfn(ct, exp);
 	}
+	rcu_read_unlock();
 }
 
 static int destroy_sibling_or_exp(const struct ip_conntrack_tuple *t)
@@ -226,6 +231,7 @@ exp_gre(struct ip_conntrack *ct,
 {
 	struct ip_conntrack_expect *exp_orig, *exp_reply;
 	int ret = 1;
+	typeof(ip_nat_pptp_hook_exp_gre) ip_nat_pptp_exp_gre;
 
 	exp_orig = ip_conntrack_expect_alloc(ct);
 	if (exp_orig == NULL)
@@ -262,8 +268,9 @@ exp_gre(struct ip_conntrack *ct,
 	exp_reply->tuple.dst.u.gre.key = peer_callid;
 	exp_reply->tuple.dst.protonum = IPPROTO_GRE;
 
-	if (ip_nat_pptp_hook_exp_gre)
-		ip_nat_pptp_hook_exp_gre(exp_orig, exp_reply);
+	ip_nat_pptp_exp_gre = rcu_dereference(ip_nat_pptp_hook_exp_gre);
+	if (ip_nat_pptp_exp_gre)
+		ip_nat_pptp_exp_gre(exp_orig, exp_reply);
 	if (ip_conntrack_expect_related(exp_orig) != 0)
 		goto out_put_both;
 	if (ip_conntrack_expect_related(exp_reply) != 0)
@@ -303,6 +310,7 @@ pptp_inbound_pkt(struct sk_buff **pskb,
 	struct ip_ct_pptp_master *info = &ct->help.ct_pptp_info;
 	u_int16_t msg;
 	__be16 cid = 0, pcid = 0;
+	typeof(ip_nat_pptp_hook_inbound) ip_nat_pptp_inbound;
 
 	msg = ntohs(ctlh->messageType);
 	DEBUGP("inbound control message %s\n", pptp_msg_name[msg]);
@@ -402,9 +410,9 @@ pptp_inbound_pkt(struct sk_buff **pskb,
 		goto invalid;
 	}
 
-	if (ip_nat_pptp_hook_inbound)
-		return ip_nat_pptp_hook_inbound(pskb, ct, ctinfo, ctlh,
-						pptpReq);
+	ip_nat_pptp_inbound = rcu_dereference(ip_nat_pptp_hook_inbound);
+	if (ip_nat_pptp_inbound)
+		return ip_nat_pptp_inbound(pskb, ct, ctinfo, ctlh, pptpReq);
 	return NF_ACCEPT;
 
 invalid:
@@ -427,6 +435,7 @@ pptp_outbound_pkt(struct sk_buff **pskb,
 	struct ip_ct_pptp_master *info = &ct->help.ct_pptp_info;
 	u_int16_t msg;
 	__be16 cid = 0, pcid = 0;
+	typeof(ip_nat_pptp_hook_outbound) ip_nat_pptp_outbound;
 
 	msg = ntohs(ctlh->messageType);
 	DEBUGP("outbound control message %s\n", pptp_msg_name[msg]);
@@ -492,9 +501,9 @@ pptp_outbound_pkt(struct sk_buff **pskb,
 		goto invalid;
 	}
 
-	if (ip_nat_pptp_hook_outbound)
-		return ip_nat_pptp_hook_outbound(pskb, ct, ctinfo, ctlh,
-						 pptpReq);
+	ip_nat_pptp_outbound = rcu_dereference(ip_nat_pptp_hook_outbound);
+	if (ip_nat_pptp_outbound)
+		return ip_nat_pptp_outbound(pskb, ct, ctinfo, ctlh, pptpReq);
 	return NF_ACCEPT;
 
 invalid:

net/ipv4/netfilter/ip_conntrack_irc.c

@@ -114,6 +114,7 @@ static int help(struct sk_buff **pskb,
 	u_int16_t dcc_port;
 	int i, ret = NF_ACCEPT;
 	char *addr_beg_p, *addr_end_p;
+	typeof(ip_nat_irc_hook) ip_nat_irc;
 
 	DEBUGP("entered\n");
 
@@ -222,11 +223,12 @@ static int help(struct sk_buff **pskb,
 					{ .tcp = { htons(0xFFFF) } }, 0xFF }});
 			exp->expectfn = NULL;
 			exp->flags = 0;
-			if (ip_nat_irc_hook)
-				ret = ip_nat_irc_hook(pskb, ctinfo, 
-						      addr_beg_p - ib_ptr,
-						      addr_end_p - addr_beg_p,
-						      exp);
+			ip_nat_irc = rcu_dereference(ip_nat_irc_hook);
+			if (ip_nat_irc)
+				ret = ip_nat_irc(pskb, ctinfo,
+						 addr_beg_p - ib_ptr,
+						 addr_end_p - addr_beg_p,
+						 exp);
 			else if (ip_conntrack_expect_related(exp) != 0)
 				ret = NF_DROP;
 			ip_conntrack_expect_put(exp);

net/ipv4/netfilter/ip_conntrack_sip.c

@@ -308,6 +308,7 @@ static int set_expected_rtp(struct sk_buff **pskb,
 	struct ip_conntrack_expect *exp;
 	enum ip_conntrack_dir dir = CTINFO2DIR(ctinfo);
 	int ret;
+	typeof(ip_nat_sdp_hook) ip_nat_sdp;
 
 	exp = ip_conntrack_expect_alloc(ct);
 	if (exp == NULL)
@@ -328,8 +329,9 @@ static int set_expected_rtp(struct sk_buff **pskb,
 	exp->expectfn = NULL;
 	exp->flags = 0;
 
-	if (ip_nat_sdp_hook)
-		ret = ip_nat_sdp_hook(pskb, ctinfo, exp, dptr);
+	ip_nat_sdp = rcu_dereference(ip_nat_sdp_hook);
+	if (ip_nat_sdp)
+		ret = ip_nat_sdp(pskb, ctinfo, exp, dptr);
 	else {
 		if (ip_conntrack_expect_related(exp) != 0)
 			ret = NF_DROP;
@@ -351,6 +353,7 @@ static int sip_help(struct sk_buff **pskb,
 	int matchoff, matchlen;
 	__be32 ipaddr;
 	u_int16_t port;
+	typeof(ip_nat_sip_hook) ip_nat_sip;
 
 	/* No Data ? */
 	dataoff = (*pskb)->nh.iph->ihl*4 + sizeof(struct udphdr);
@@ -368,8 +371,9 @@ static int sip_help(struct sk_buff **pskb,
 		goto out;
 	}
 
-	if (ip_nat_sip_hook) {
-		if (!ip_nat_sip_hook(pskb, ctinfo, ct, &dptr)) {
+	ip_nat_sip = rcu_dereference(ip_nat_sip_hook);
+	if (ip_nat_sip) {
+		if (!ip_nat_sip(pskb, ctinfo, ct, &dptr)) {
 			ret = NF_DROP;
 			goto out;
 		}

net/ipv4/netfilter/ip_conntrack_tftp.c

@@ -50,6 +50,7 @@ static int tftp_help(struct sk_buff **pskb,
 	struct tftphdr _tftph, *tfh;
 	struct ip_conntrack_expect *exp;
 	unsigned int ret = NF_ACCEPT;
+	typeof(ip_nat_tftp_hook) ip_nat_tftp;
 
 	tfh = skb_header_pointer(*pskb,
 				 (*pskb)->nh.iph->ihl*4+sizeof(struct udphdr),
@@ -81,8 +82,9 @@ static int tftp_help(struct sk_buff **pskb,
 		DEBUGP("expect: ");
 		DUMP_TUPLE(&exp->tuple);
 		DUMP_TUPLE(&exp->mask);
-		if (ip_nat_tftp_hook)
-			ret = ip_nat_tftp_hook(pskb, ctinfo, exp);
+		ip_nat_tftp = rcu_dereference(ip_nat_tftp_hook);
+		if (ip_nat_tftp)
+			ret = ip_nat_tftp(pskb, ctinfo, exp);
 		else if (ip_conntrack_expect_related(exp) != 0)
 			ret = NF_DROP;
 		ip_conntrack_expect_put(exp);

net/ipv4/netfilter/ip_nat_amanda.c

@@ -70,15 +70,14 @@ static unsigned int help(struct sk_buff **pskb,
 
 static void __exit ip_nat_amanda_fini(void)
 {
-	ip_nat_amanda_hook = NULL;
-	/* Make sure noone calls it, meanwhile. */
-	synchronize_net();
+	rcu_assign_pointer(ip_nat_amanda_hook, NULL);
+	synchronize_rcu();
 }
 
 static int __init ip_nat_amanda_init(void)
 {
-	BUG_ON(ip_nat_amanda_hook);
-	ip_nat_amanda_hook = help;
+	BUG_ON(rcu_dereference(ip_nat_amanda_hook));
+	rcu_assign_pointer(ip_nat_amanda_hook, help);
 	return 0;
 }
 

net/ipv4/netfilter/ip_nat_ftp.c

@@ -156,15 +156,14 @@ static unsigned int ip_nat_ftp(struct sk_buff **pskb,
 
 static void __exit ip_nat_ftp_fini(void)
 {
-	ip_nat_ftp_hook = NULL;
-	/* Make sure noone calls it, meanwhile. */
-	synchronize_net();
+	rcu_assign_pointer(ip_nat_ftp_hook, NULL);
+	synchronize_rcu();
 }
 
 static int __init ip_nat_ftp_init(void)
 {
-	BUG_ON(ip_nat_ftp_hook);
-	ip_nat_ftp_hook = ip_nat_ftp;
+	BUG_ON(rcu_dereference(ip_nat_ftp_hook));
+	rcu_assign_pointer(ip_nat_ftp_hook, ip_nat_ftp);
 	return 0;
 }
 

net/ipv4/netfilter/ip_nat_helper_h323.c

@@ -563,25 +563,25 @@ static int nat_callforwarding(struct sk_buff **pskb, struct ip_conntrack *ct,
 /****************************************************************************/
 static int __init init(void)
 {
-	BUG_ON(set_h245_addr_hook != NULL);
-	BUG_ON(set_h225_addr_hook != NULL);
-	BUG_ON(set_sig_addr_hook != NULL);
-	BUG_ON(set_ras_addr_hook != NULL);
-	BUG_ON(nat_rtp_rtcp_hook != NULL);
-	BUG_ON(nat_t120_hook != NULL);
-	BUG_ON(nat_h245_hook != NULL);
-	BUG_ON(nat_callforwarding_hook != NULL);
-	BUG_ON(nat_q931_hook != NULL);
-
-	set_h245_addr_hook = set_h245_addr;
-	set_h225_addr_hook = set_h225_addr;
-	set_sig_addr_hook = set_sig_addr;
-	set_ras_addr_hook = set_ras_addr;
-	nat_rtp_rtcp_hook = nat_rtp_rtcp;
-	nat_t120_hook = nat_t120;
-	nat_h245_hook = nat_h245;
-	nat_callforwarding_hook = nat_callforwarding;
-	nat_q931_hook = nat_q931;
+	BUG_ON(rcu_dereference(set_h245_addr_hook) != NULL);
+	BUG_ON(rcu_dereference(set_h225_addr_hook) != NULL);
+	BUG_ON(rcu_dereference(set_sig_addr_hook) != NULL);
+	BUG_ON(rcu_dereference(set_ras_addr_hook) != NULL);
+	BUG_ON(rcu_dereference(nat_rtp_rtcp_hook) != NULL);
+	BUG_ON(rcu_dereference(nat_t120_hook) != NULL);
+	BUG_ON(rcu_dereference(nat_h245_hook) != NULL);
+	BUG_ON(rcu_dereference(nat_callforwarding_hook) != NULL);
+	BUG_ON(rcu_dereference(nat_q931_hook) != NULL);
+
+	rcu_assign_pointer(set_h245_addr_hook, set_h245_addr);
+	rcu_assign_pointer(set_h225_addr_hook, set_h225_addr);
+	rcu_assign_pointer(set_sig_addr_hook, set_sig_addr);
+	rcu_assign_pointer(set_ras_addr_hook, set_ras_addr);
+	rcu_assign_pointer(nat_rtp_rtcp_hook, nat_rtp_rtcp);
+	rcu_assign_pointer(nat_t120_hook, nat_t120);
+	rcu_assign_pointer(nat_h245_hook, nat_h245);
+	rcu_assign_pointer(nat_callforwarding_hook, nat_callforwarding);
+	rcu_assign_pointer(nat_q931_hook, nat_q931);
 
 	DEBUGP("ip_nat_h323: init success\n");
 	return 0;
@@ -590,16 +590,16 @@ static int __init init(void)
 /****************************************************************************/
 static void __exit fini(void)
 {
-	set_h245_addr_hook = NULL;
-	set_h225_addr_hook = NULL;
-	set_sig_addr_hook = NULL;
-	set_ras_addr_hook = NULL;
-	nat_rtp_rtcp_hook = NULL;
-	nat_t120_hook = NULL;
-	nat_h245_hook = NULL;
-	nat_callforwarding_hook = NULL;
-	nat_q931_hook = NULL;
-	synchronize_net();
+	rcu_assign_pointer(set_h245_addr_hook, NULL);
+	rcu_assign_pointer(set_h225_addr_hook, NULL);
+	rcu_assign_pointer(set_sig_addr_hook, NULL);
+	rcu_assign_pointer(set_ras_addr_hook, NULL);
+	rcu_assign_pointer(nat_rtp_rtcp_hook, NULL);
+	rcu_assign_pointer(nat_t120_hook, NULL);
+	rcu_assign_pointer(nat_h245_hook, NULL);
+	rcu_assign_pointer(nat_callforwarding_hook, NULL);
+	rcu_assign_pointer(nat_q931_hook, NULL);
+	synchronize_rcu();
 }
 
 /****************************************************************************/

net/ipv4/netfilter/ip_nat_helper_pptp.c

@@ -315,17 +315,17 @@ static int __init ip_nat_helper_pptp_init(void)
 	if (ret < 0)
 		return ret;
 
-	BUG_ON(ip_nat_pptp_hook_outbound);
-	ip_nat_pptp_hook_outbound = &pptp_outbound_pkt;
+	BUG_ON(rcu_dereference(ip_nat_pptp_hook_outbound));
+	rcu_assign_pointer(ip_nat_pptp_hook_outbound, pptp_outbound_pkt);
 
-	BUG_ON(ip_nat_pptp_hook_inbound);
-	ip_nat_pptp_hook_inbound = &pptp_inbound_pkt;
+	BUG_ON(rcu_dereference(ip_nat_pptp_hook_inbound));
+	rcu_assign_pointer(ip_nat_pptp_hook_inbound, pptp_inbound_pkt);
 
-	BUG_ON(ip_nat_pptp_hook_exp_gre);
-	ip_nat_pptp_hook_exp_gre = &pptp_exp_gre;
+	BUG_ON(rcu_dereference(ip_nat_pptp_hook_exp_gre));
+	rcu_assign_pointer(ip_nat_pptp_hook_exp_gre, pptp_exp_gre);
 
-	BUG_ON(ip_nat_pptp_hook_expectfn);
-	ip_nat_pptp_hook_expectfn = &pptp_nat_expected;
+	BUG_ON(rcu_dereference(ip_nat_pptp_hook_expectfn));
+	rcu_assign_pointer(ip_nat_pptp_hook_expectfn, pptp_nat_expected);
 
 	printk("ip_nat_pptp version %s loaded\n", IP_NAT_PPTP_VERSION);
 	return 0;
@@ -335,14 +335,13 @@ static void __exit ip_nat_helper_pptp_fini(void)
 {
 	DEBUGP("cleanup_module\n" );
 
-	ip_nat_pptp_hook_expectfn = NULL;
-	ip_nat_pptp_hook_exp_gre = NULL;
-	ip_nat_pptp_hook_inbound = NULL;
-	ip_nat_pptp_hook_outbound = NULL;
+	rcu_assign_pointer(ip_nat_pptp_hook_expectfn, NULL);
+	rcu_assign_pointer(ip_nat_pptp_hook_exp_gre, NULL);
+	rcu_assign_pointer(ip_nat_pptp_hook_inbound, NULL);
+	rcu_assign_pointer(ip_nat_pptp_hook_outbound, NULL);
+	synchronize_rcu();
 
 	ip_nat_proto_gre_fini();
-	/* Make sure noone calls it, meanwhile */
-	synchronize_net();
 
 	printk("ip_nat_pptp version %s unloaded\n", IP_NAT_PPTP_VERSION);
 }

net/ipv4/netfilter/ip_nat_irc.c

@@ -98,15 +98,14 @@ static unsigned int help(struct sk_buff **pskb,
 
 static void __exit ip_nat_irc_fini(void)
 {
-	ip_nat_irc_hook = NULL;
-	/* Make sure noone calls it, meanwhile. */
-	synchronize_net();
+	rcu_assign_pointer(ip_nat_irc_hook, NULL);
+	synchronize_rcu();
 }
 
 static int __init ip_nat_irc_init(void)
 {
-	BUG_ON(ip_nat_irc_hook);
-	ip_nat_irc_hook = help;
+	BUG_ON(rcu_dereference(ip_nat_irc_hook));
+	rcu_assign_pointer(ip_nat_irc_hook, help);
 	return 0;
 }
 

net/ipv4/netfilter/ip_nat_sip.c

@@ -230,18 +230,17 @@ static unsigned int ip_nat_sdp(struct sk_buff **pskb,
 
 static void __exit fini(void)
 {
-	ip_nat_sip_hook = NULL;
-	ip_nat_sdp_hook = NULL;
-	/* Make sure noone calls it, meanwhile. */
-	synchronize_net();
+	rcu_assign_pointer(ip_nat_sip_hook, NULL);
+	rcu_assign_pointer(ip_nat_sdp_hook, NULL);
+	synchronize_rcu();
 }
 
 static int __init init(void)
 {
-	BUG_ON(ip_nat_sip_hook);
-	BUG_ON(ip_nat_sdp_hook);
-	ip_nat_sip_hook = ip_nat_sip;
-	ip_nat_sdp_hook = ip_nat_sdp;
+	BUG_ON(rcu_dereference(ip_nat_sip_hook));
+	BUG_ON(rcu_dereference(ip_nat_sdp_hook));
+	rcu_assign_pointer(ip_nat_sip_hook, ip_nat_sip);
+	rcu_assign_pointer(ip_nat_sdp_hook, ip_nat_sdp);
 	return 0;
 }
 

net/ipv4/netfilter/ip_nat_tftp.c

@@ -55,15 +55,14 @@ static unsigned int help(struct sk_buff **pskb,
 
 static void __exit ip_nat_tftp_fini(void)
 {
-	ip_nat_tftp_hook = NULL;
-	/* Make sure noone calls it, meanwhile. */
-	synchronize_net();
+	rcu_assign_pointer(ip_nat_tftp_hook, NULL);
+	synchronize_rcu();
 }
 
 static int __init ip_nat_tftp_init(void)
 {
-	BUG_ON(ip_nat_tftp_hook);
-	ip_nat_tftp_hook = help;
+	BUG_ON(rcu_dereference(ip_nat_tftp_hook));
+	rcu_assign_pointer(ip_nat_tftp_hook, help);
 	return 0;
 }
 

net/netfilter/nf_conntrack_ftp.c

@@ -369,9 +369,9 @@ static int help(struct sk_buff **pskb,
 	struct ip_ct_ftp_master *ct_ftp_info = &nfct_help(ct)->help.ct_ftp_info;
 	struct nf_conntrack_expect *exp;
 	struct nf_conntrack_man cmd = {};
-
 	unsigned int i;
 	int found = 0, ends_in_nl;
+	typeof(nf_nat_ftp_hook) nf_nat_ftp;
 
 	/* Until there's been traffic both ways, don't look in packets. */
 	if (ctinfo != IP_CT_ESTABLISHED
@@ -520,9 +520,10 @@ static int help(struct sk_buff **pskb,
 
 	/* Now, NAT might want to mangle the packet, and register the
 	 * (possibly changed) expectation itself. */
-	if (nf_nat_ftp_hook)
-		ret = nf_nat_ftp_hook(pskb, ctinfo, search[dir][i].ftptype,
-				      matchoff, matchlen, exp, &seq);
+	nf_nat_ftp = rcu_dereference(nf_nat_ftp_hook);
+	if (nf_nat_ftp)
+		ret = nf_nat_ftp(pskb, ctinfo, search[dir][i].ftptype,
+				 matchoff, matchlen, exp, &seq);
 	else {
 		/* Can't expect this?  Best to drop packet now. */
 		if (nf_conntrack_expect_related(exp) != 0)