[PATCH 2.6] 2/2: Fix NAT helper locking

[PATCH 2.6] 2/2: Fix NAT helper locking

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 9607 bytes --]

Hi Dave!

This is the second of a two part patch.

This part fixes the locking in NAT helpers.

Please apply, Thanks.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/08/08 01:40:28+02:00 kaber@coreworks.de 
#   [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>
# 
# net/ipv4/netfilter/ip_nat_irc.c
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +1 -17
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
# net/ipv4/netfilter/ip_nat_ftp.c
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +1 -19
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
# net/ipv4/netfilter/ip_conntrack_irc.c
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +3 -4
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
# net/ipv4/netfilter/ip_conntrack_ftp.c
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +1 -2
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
# include/linux/netfilter_ipv4/ip_conntrack_irc.h
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +0 -5
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
# include/linux/netfilter_ipv4/ip_conntrack_ftp.h
#   2004/08/08 01:40:10+02:00 kaber@coreworks.de +0 -5
#   [NETFILTER]: Fix deadlock condition in conntrack/nat-helpers
# 
diff -Nru a/include/linux/netfilter_ipv4/ip_conntrack_ftp.h b/include/linux/netfilter_ipv4/ip_conntrack_ftp.h
--- a/include/linux/netfilter_ipv4/ip_conntrack_ftp.h	2004-08-08 01:41:23 +02:00
+++ b/include/linux/netfilter_ipv4/ip_conntrack_ftp.h	2004-08-08 01:41:23 +02:00
@@ -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__ */
diff -Nru a/include/linux/netfilter_ipv4/ip_conntrack_irc.h b/include/linux/netfilter_ipv4/ip_conntrack_irc.h
--- a/include/linux/netfilter_ipv4/ip_conntrack_irc.h	2004-08-08 01:41:23 +02:00
+++ b/include/linux/netfilter_ipv4/ip_conntrack_irc.h	2004-08-08 01:41:23 +02:00
@@ -33,12 +33,7 @@
 
 #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__ */
 
diff -Nru a/net/ipv4/netfilter/ip_conntrack_ftp.c b/net/ipv4/netfilter/ip_conntrack_ftp.c
--- a/net/ipv4/netfilter/ip_conntrack_ftp.c	2004-08-08 01:41:23 +02:00
+++ b/net/ipv4/netfilter/ip_conntrack_ftp.c	2004-08-08 01:41:23 +02:00
@@ -27,7 +27,7 @@
 /* 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
@@ -455,7 +455,6 @@
 }
 
 PROVIDES_CONNTRACK(ftp);
-EXPORT_SYMBOL(ip_ftp_lock);
 
 module_init(init);
 module_exit(fini);
diff -Nru a/net/ipv4/netfilter/ip_conntrack_irc.c b/net/ipv4/netfilter/ip_conntrack_irc.c
--- a/net/ipv4/netfilter/ip_conntrack_irc.c	2004-08-08 01:41:23 +02:00
+++ b/net/ipv4/netfilter/ip_conntrack_irc.c	2004-08-08 01:41:23 +02:00
@@ -40,6 +40,7 @@
 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");
@@ -54,7 +55,6 @@
 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
@@ -134,7 +134,7 @@
 	if (dataoff >= skb->len)
 		return NF_ACCEPT;
 
-	LOCK_BH(&ip_irc_lock);
+	LOCK_BH(&irc_buffer_lock);
 	skb_copy_bits(skb, dataoff, irc_buffer, skb->len - dataoff);
 
 	data = irc_buffer;
@@ -227,7 +227,7 @@
 	} /* while data < ... */
 
  out:
-	UNLOCK_BH(&ip_irc_lock);
+	UNLOCK_BH(&irc_buffer_lock);
 	return NF_ACCEPT;
 }
 
@@ -302,7 +302,6 @@
 }
 
 PROVIDES_CONNTRACK(irc);
-EXPORT_SYMBOL(ip_irc_lock);
 
 module_init(init);
 module_exit(fini);
diff -Nru a/net/ipv4/netfilter/ip_nat_ftp.c b/net/ipv4/netfilter/ip_nat_ftp.c
--- a/net/ipv4/netfilter/ip_nat_ftp.c	2004-08-08 01:41:23 +02:00
+++ b/net/ipv4/netfilter/ip_nat_ftp.c	2004-08-08 01:41:23 +02:00
@@ -35,8 +35,6 @@
 
 MODULE_PARM(ports, "1-" __MODULE_STRING(MAX_PORTS) "i");
 
-DECLARE_LOCK_EXTERN(ip_ftp_lock);
-
 /* FIXME: Time out? --RR */
 
 static unsigned int
@@ -59,8 +57,6 @@
 	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. */
@@ -75,7 +71,6 @@
 		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;
@@ -111,8 +106,6 @@
 {
 	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);
 
@@ -134,8 +127,6 @@
 {
 	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");
@@ -156,8 +147,6 @@
 {
 	char buffer[sizeof("|||65535|")];
 
-	MUST_BE_LOCKED(&ip_ftp_lock);
-
 	sprintf(buffer, "|||%u|", port);
 
 	DEBUGP("calling ip_nat_mangle_tcp_packet\n");
@@ -189,7 +178,6 @@
 	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));
@@ -268,15 +256,12 @@
 	}
 
 	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. */
@@ -286,11 +271,8 @@
 			       ntohl(tcph->seq),
 			       ntohl(tcph->seq) + datalen);
 		}
-		UNLOCK_BH(&ip_ftp_lock);
 		return NF_DROP;
 	}
-	UNLOCK_BH(&ip_ftp_lock);
-
 	return NF_ACCEPT;
 }
 
diff -Nru a/net/ipv4/netfilter/ip_nat_irc.c b/net/ipv4/netfilter/ip_nat_irc.c
--- a/net/ipv4/netfilter/ip_nat_irc.c	2004-08-08 01:41:23 +02:00
+++ b/net/ipv4/netfilter/ip_nat_irc.c	2004-08-08 01:41:23 +02:00
@@ -44,9 +44,6 @@
 MODULE_PARM(ports, "1-" __MODULE_STRING(MAX_PORTS) "i");
 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
@@ -102,8 +99,6 @@
 	/* "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));
@@ -111,11 +106,6 @@
 	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++) {
@@ -185,15 +175,12 @@
 	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. */
@@ -204,11 +191,8 @@
 			     ntohl(tcph->seq),
 			     ntohl(tcph->seq) + datalen);
 		}
-		UNLOCK_BH(&ip_irc_lock);
 		return NF_DROP;
 	}
-	UNLOCK_BH(&ip_irc_lock);
-
 	return NF_ACCEPT;
 }
 
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2.6] 2/2: Fix NAT helper locking

[David S. Miller]

On Fri, 3 Sep 2004 09:02:34 +0200
Harald Welte <laforge@netfilter.org> wrote:

> This is the second of a two part patch.
> 
> This part fixes the locking in NAT helpers.
> 
> Please apply, Thanks.

Applied both patches, but the first had serious offsets
and the second had to be applied by hand due to rejects
against the current 2.6.x sources.

Ummm... wow what ancient tree did you patch against
Harald?  The tree you patched against didn't even have the
skb_header_pointer() changes in it, that's caveman
era :-)  That's what caused the rejects.

Anyways, I merged it all in cleanly.

Thanks.

Re: [PATCH 2.6] 2/2: Fix NAT helper locking

[Patrick McHardy]

David S. Miller wrote:

>Applied both patches, but the first had serious offsets
>and the second had to be applied by hand due to rejects
>against the current 2.6.x sources.
>
>Ummm... wow what ancient tree did you patch against
>Harald?  The tree you patched against didn't even have the
>skb_header_pointer() changes in it, that's caveman
>era :-)  That's what caused the rejects.
>  
>
My fault, I put the patches into patch-o-matic about two or
three weeks ago and didn't expect they wouldn't apply anymore
so quickly :)

>
>Anyways, I merged it all in cleanly.
>
Thanks.

Patrick

Re: [PATCH 2.6] 2/2: Fix NAT helper locking

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Fri, Sep 03, 2004 at 12:24:53AM -0700, David S. Miller wrote:
 
> Ummm... wow what ancient tree did you patch against
> Harald?  The tree you patched against didn't even have the
> skb_header_pointer() changes in it, that's caveman
> era :-)  That's what caused the rejects.

I tried to apply the patch against 2.6.9-rc1 before sending it to you.
It had offsets, but applied cleanly

> Thanks.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]