[NETFILTER]: Fix deadlock condition in conntrack/nat-helpers

There is a possible deadlock condition with conntrack/nat-helpers:

CPU1:
conntrack-helper:help: lock(private_lock)
ip_conntrack_expect_related: write_lock(ip_conntrack_lock)

CPU2:
nat-core:do_bindings: read_lock(ip_conntrack_lock)
nat-helper:help: lock(private_lock)

The lock in the nat-helper is unneccessary because the expectation
is never changed and is protected by ip_conntrack_lock.
Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Harald Welte <laforge@netfilter.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

include/linux/netfilter_ipv4/ip_conntrack_ftp.h

@@ -4,11 +4,6 @@
 
 #ifdef __KERNEL__
 
-#include <linux/netfilter_ipv4/lockhelp.h>
-
-/* Protects ftp part of conntracks */
-DECLARE_LOCK_EXTERN(ip_ftp_lock);
-
 #define FTP_PORT	21
 
 #endif /* __KERNEL__ */

include/linux/netfilter_ipv4/ip_conntrack_irc.h

@@ -33,13 +33,8 @@ struct ip_ct_irc_master {
 
 #ifdef __KERNEL__
 
-#include <linux/netfilter_ipv4/lockhelp.h>
-
 #define IRC_PORT	6667
 
-/* Protects irc part of conntracks */
-DECLARE_LOCK_EXTERN(ip_irc_lock);
-
 #endif /* __KERNEL__ */
 
 #endif /* _IP_CONNTRACK_IRC_H */

net/ipv4/netfilter/ip_conntrack_ftp.c

@@ -28,7 +28,7 @@ MODULE_DESCRIPTION("ftp connection tracking helper");
 /* This is slow, but it's simple. --RR */
 static char ftp_buffer[65536];
 
-DECLARE_LOCK(ip_ftp_lock);
+static DECLARE_LOCK(ip_ftp_lock);
 struct module *ip_conntrack_ftp = THIS_MODULE;
 
 #define MAX_PORTS 8
@@ -460,7 +460,6 @@ static int __init init(void)
 }
 
 PROVIDES_CONNTRACK(ftp);
-EXPORT_SYMBOL(ip_ftp_lock);
 
 module_init(init);
 module_exit(fini);

net/ipv4/netfilter/ip_conntrack_irc.c

@@ -41,6 +41,7 @@ static int max_dcc_channels = 8;
 static unsigned int dcc_timeout = 300;
 /* This is slow, but it's simple. --RR */
 static char irc_buffer[65536];
+static DECLARE_LOCK(irc_buffer_lock);
 
 MODULE_AUTHOR("Harald Welte <laforge@netfilter.org>");
 MODULE_DESCRIPTION("IRC (DCC) connection tracking helper");
@@ -55,7 +56,6 @@ MODULE_PARM_DESC(dcc_timeout, "timeout on for unestablished DCC channels");
 static char *dccprotos[] = { "SEND ", "CHAT ", "MOVE ", "TSEND ", "SCHAT " };
 #define MINMATCHLEN	5
 
-DECLARE_LOCK(ip_irc_lock);
 struct module *ip_conntrack_irc = THIS_MODULE;
 
 #if 0
@@ -137,7 +137,7 @@ static int help(struct sk_buff *skb,
 	if (dataoff >= skb->len)
 		return NF_ACCEPT;
 
-	LOCK_BH(&ip_irc_lock);
+	LOCK_BH(&irc_buffer_lock);
 	ib_ptr = skb_header_pointer(skb, dataoff,
 				    skb->len - dataoff, irc_buffer);
 	BUG_ON(ib_ptr == NULL);
@@ -232,7 +232,7 @@ static int help(struct sk_buff *skb,
 	} /* while data < ... */
 
  out:
-	UNLOCK_BH(&ip_irc_lock);
+	UNLOCK_BH(&irc_buffer_lock);
 	return NF_ACCEPT;
 }
 
@@ -306,7 +306,6 @@ static void fini(void)
 }
 
 PROVIDES_CONNTRACK(irc);
-EXPORT_SYMBOL(ip_irc_lock);
 
 module_init(init);
 module_exit(fini);

net/ipv4/netfilter/ip_nat_ftp.c

@@ -36,8 +36,6 @@ static int ports_c;
 
 module_param_array(ports, int, ports_c, 0400);
 
-DECLARE_LOCK_EXTERN(ip_ftp_lock);
-
 /* FIXME: Time out? --RR */
 
 static unsigned int
@@ -60,8 +58,6 @@ ftp_nat_expected(struct sk_buff **pskb,
 	DEBUGP("nat_expected: We have a connection!\n");
 	exp_ftp_info = &ct->master->help.exp_ftp_info;
 
-	LOCK_BH(&ip_ftp_lock);
-
 	if (exp_ftp_info->ftptype == IP_CT_FTP_PORT
 	    || exp_ftp_info->ftptype == IP_CT_FTP_EPRT) {
 		/* PORT command: make connection go to the client. */
@@ -76,7 +72,6 @@ ftp_nat_expected(struct sk_buff **pskb,
 		DEBUGP("nat_expected: PASV cmd. %u.%u.%u.%u->%u.%u.%u.%u\n",
 		       NIPQUAD(newsrcip), NIPQUAD(newdstip));
 	}
-	UNLOCK_BH(&ip_ftp_lock);
 
 	if (HOOK2MANIP(hooknum) == IP_NAT_MANIP_SRC)
 		newip = newsrcip;
@@ -112,8 +107,6 @@ mangle_rfc959_packet(struct sk_buff **pskb,
 {
 	char buffer[sizeof("nnn,nnn,nnn,nnn,nnn,nnn")];
 
-	MUST_BE_LOCKED(&ip_ftp_lock);
-
 	sprintf(buffer, "%u,%u,%u,%u,%u,%u",
 		NIPQUAD(newip), port>>8, port&0xFF);
 
@@ -135,8 +128,6 @@ mangle_eprt_packet(struct sk_buff **pskb,
 {
 	char buffer[sizeof("|1|255.255.255.255|65535|")];
 
-	MUST_BE_LOCKED(&ip_ftp_lock);
-
 	sprintf(buffer, "|1|%u.%u.%u.%u|%u|", NIPQUAD(newip), port);
 
 	DEBUGP("calling ip_nat_mangle_tcp_packet\n");
@@ -157,8 +148,6 @@ mangle_epsv_packet(struct sk_buff **pskb,
 {
 	char buffer[sizeof("|||65535|")];
 
-	MUST_BE_LOCKED(&ip_ftp_lock);
-
 	sprintf(buffer, "|||%u|", port);
 
 	DEBUGP("calling ip_nat_mangle_tcp_packet\n");
@@ -190,7 +179,6 @@ static int ftp_data_fixup(const struct ip_ct_ftp_expect *exp_ftp_info,
 	u_int16_t port;
 	struct ip_conntrack_tuple newtuple;
 
-	MUST_BE_LOCKED(&ip_ftp_lock);
 	DEBUGP("FTP_NAT: seq %u + %u in %u\n",
 	       expect->seq, exp_ftp_info->len,
 	       ntohl(tcph->seq));
@@ -269,15 +257,12 @@ static unsigned int help(struct ip_conntrack *ct,
 	}
 
 	datalen = (*pskb)->len - iph->ihl * 4 - tcph->doff * 4;
-	LOCK_BH(&ip_ftp_lock);
 	/* If it's in the right range... */
 	if (between(exp->seq + exp_ftp_info->len,
 		    ntohl(tcph->seq),
 		    ntohl(tcph->seq) + datalen)) {
-		if (!ftp_data_fixup(exp_ftp_info, ct, pskb, ctinfo, exp)) {
-			UNLOCK_BH(&ip_ftp_lock);
+		if (!ftp_data_fixup(exp_ftp_info, ct, pskb, ctinfo, exp))
 			return NF_DROP;
-		}
 	} else {
 		/* Half a match?  This means a partial retransmisison.
 		   It's a cracker being funky. */
@@ -287,11 +272,8 @@ static unsigned int help(struct ip_conntrack *ct,
 			       ntohl(tcph->seq),
 			       ntohl(tcph->seq) + datalen);
 		}
-		UNLOCK_BH(&ip_ftp_lock);
 		return NF_DROP;
 	}
-	UNLOCK_BH(&ip_ftp_lock);
-
 	return NF_ACCEPT;
 }
 

net/ipv4/netfilter/ip_nat_irc.c

@@ -45,9 +45,6 @@ MODULE_LICENSE("GPL");
 module_param_array(ports, int, ports_c, 0400);
 MODULE_PARM_DESC(ports, "port numbers of IRC servers");
 
-/* protects irc part of conntracks */
-DECLARE_LOCK_EXTERN(ip_irc_lock);
-
 /* FIXME: Time out? --RR */
 
 static unsigned int
@@ -103,8 +100,6 @@ static int irc_data_fixup(const struct ip_ct_irc_expect *exp_irc_info,
 	/* "4294967296 65635 " */
 	char buffer[18];
 
-	MUST_BE_LOCKED(&ip_irc_lock);
-
 	DEBUGP("IRC_NAT: info (seq %u + %u) in %u\n",
 	       expect->seq, exp_irc_info->len,
 	       ntohl(tcph->seq));
@@ -112,11 +107,6 @@ static int irc_data_fixup(const struct ip_ct_irc_expect *exp_irc_info,
 	newip = ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
 
 	/* Alter conntrack's expectations. */
-
-	/* We can read expect here without conntrack lock, since it's
-	   only set in ip_conntrack_irc, with ip_irc_lock held
-	   writable */
-
 	t = expect->tuple;
 	t.dst.ip = newip;
 	for (port = exp_irc_info->port; port != 0; port++) {
@@ -186,15 +176,12 @@ static unsigned int help(struct ip_conntrack *ct,
 	DEBUGP("got beyond not touching\n");
 
 	datalen = (*pskb)->len - iph->ihl * 4 - tcph->doff * 4;
-	LOCK_BH(&ip_irc_lock);
 	/* Check whether the whole IP/address pattern is carried in the payload */
 	if (between(exp->seq + exp_irc_info->len,
 		    ntohl(tcph->seq),
 		    ntohl(tcph->seq) + datalen)) {
-		if (!irc_data_fixup(exp_irc_info, ct, pskb, ctinfo, exp)) {
-			UNLOCK_BH(&ip_irc_lock);
+		if (!irc_data_fixup(exp_irc_info, ct, pskb, ctinfo, exp))
 			return NF_DROP;
-		}
 	} else { 
 		/* Half a match?  This means a partial retransmisison.
 		   It's a cracker being funky. */
@@ -205,11 +192,8 @@ static unsigned int help(struct ip_conntrack *ct,
 			     ntohl(tcph->seq),
 			     ntohl(tcph->seq) + datalen);
 		}
-		UNLOCK_BH(&ip_irc_lock);
 		return NF_DROP;
 	}
-	UNLOCK_BH(&ip_irc_lock);
-
 	return NF_ACCEPT;
 }