From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: [PATCH, RFC]: 2.4 broken helper locking Date: Sat, 15 Nov 2003 16:40:42 +0100 Sender: netfilter-devel-admin@lists.netfilter.org Message-ID: <3FB648FA.2030403@trash.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------020206090900050409020608" Return-path: To: Netfilter Development Mailinglist Errors-To: netfilter-devel-admin@lists.netfilter.org List-Help: List-Post: List-Subscribe: , List-Unsubscribe: , List-Archive: List-Id: netfilter-devel.vger.kernel.org This is a multi-part message in MIME format. --------------020206090900050409020608 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit I've sent this a month or two ago but no response so here goes another try. Please excuse any mistakes, I don't remember all details. Locking of ftp/irc helpers in 2.4 looks broken (and amanda, but thats fixed in my last patch). Both use private locks to protect expectation data between the conntrack and the nat helper. The locks are unneccessary because: - ip_conntrack_expect_related write-locks ip_conntrack_lock - the nat-helpers are called under read-locked ip_conntrack lock - the expectations private data can't change once it's register They may also cause deadlock because of this possible sequence: 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) This patch removes the locks and renames some variables from ct_xxx to exp_xxx to make things clearer. A remaining problem is in ip_nat_standalone.c: WRITE_LOCK(&ip_nat_lock); /* Seen it before? This can happen for loopback, retrans, or local packets.. */ if (!(info->initialized & (1 << maniptype))) { unsigned int ret; if (ct->master && master_ct(ct)->nat.info.helper && master_ct(ct)->nat.info.helper->expect) { ret = call_expect(master_ct(ct), pskb, hooknum, ct, info); } else { ct->master is not protected by ip_nat_lock. The expect function of the ftp helper also touches ct->master so we need to hold ip_conntrack_lock. I'm not sure if and how the locks can be nested, so comments on this are appreciated. Patrick --------------020206090900050409020608 Content-Type: text/plain; name="helper-locking.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="helper-locking.diff" # This is a BitKeeper generated patch for the following project: # Project Name: Linux kernel tree # This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas: # ChangeSet 1.1168 -> 1.1169 # net/ipv4/netfilter/ip_nat_ftp.c 1.8 -> 1.9 # net/ipv4/netfilter/ip_conntrack_irc.c 1.10 -> 1.11 # net/ipv4/netfilter/ip_conntrack_ftp.c 1.11 -> 1.12 # net/ipv4/netfilter/ip_nat_irc.c 1.5 -> 1.6 # # The following is the BitKeeper ChangeSet Log # -------------------------------------------- # 03/11/15 kaber@trash.net 1.1169 # Partially fix broken helper locking # -------------------------------------------- # 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 Sat Nov 15 16:23:47 2003 +++ b/net/ipv4/netfilter/ip_conntrack_ftp.c Sat Nov 15 16:23:47 2003 @@ -11,7 +11,7 @@ #include #include -DECLARE_LOCK(ip_ftp_lock); +static DECLARE_LOCK(ip_ftp_lock); struct module *ip_conntrack_ftp = THIS_MODULE; #define MAX_PORTS 8 @@ -338,7 +338,6 @@ memset(&expect, 0, sizeof(expect)); /* Update the ftp info */ - LOCK_BH(&ip_ftp_lock); if (htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3]) == ct->tuplehash[dir].tuple.src.ip) { exp->seq = ntohl(tcph->seq) + matchoff; @@ -358,7 +357,8 @@ for reporting this potential problem (DMZ machines opening holes to internal networks, or the packet filter itself). */ - if (!loose) goto out; + if (!loose) + return NF_ACCEPT; } exp->tuple = ((struct ip_conntrack_tuple) @@ -376,8 +376,6 @@ /* Ignore failure; should only happen with NAT */ ip_conntrack_expect_related(ct, &expect); - out: - UNLOCK_BH(&ip_ftp_lock); return NF_ACCEPT; } @@ -434,8 +432,6 @@ } return 0; } - -EXPORT_SYMBOL(ip_ftp_lock); MODULE_LICENSE("GPL"); module_init(init); 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 Sat Nov 15 16:23:47 2003 +++ b/net/ipv4/netfilter/ip_conntrack_irc.c Sat Nov 15 16:23:47 2003 @@ -29,7 +29,6 @@ #include #include -#include #include #include @@ -61,7 +60,6 @@ }; #define MINMATCHLEN 5 -DECLARE_LOCK(ip_irc_lock); struct module *ip_conntrack_irc = THIS_MODULE; #if 0 @@ -208,8 +206,6 @@ memset(&expect, 0, sizeof(expect)); - LOCK_BH(&ip_irc_lock); - /* save position of address in dcc string, * neccessary for NAT */ DEBUGP("tcph->seq = %u\n", tcph->seq); @@ -236,8 +232,6 @@ ntohs(exp->tuple.dst.u.tcp.port)); ip_conntrack_expect_related(ct, &expect); - UNLOCK_BH(&ip_irc_lock); - return NF_ACCEPT; } /* for .. NUM_DCCPROTO */ } /* while data < ... */ @@ -314,8 +308,6 @@ ip_conntrack_helper_unregister(&irc_helpers[i]); } } - -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 Sat Nov 15 16:23:47 2003 +++ b/net/ipv4/netfilter/ip_nat_ftp.c Sat Nov 15 16:23:47 2003 @@ -24,8 +24,6 @@ MODULE_PARM(ports, "1-" __MODULE_STRING(MAX_PORTS) "i"); #endif -DECLARE_LOCK_EXTERN(ip_ftp_lock); - /* FIXME: Time out? --RR */ static unsigned int @@ -48,8 +46,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. */ @@ -64,7 +60,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; @@ -100,8 +95,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); @@ -123,8 +116,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"); @@ -145,8 +136,6 @@ { char buffer[sizeof("|||65535|")]; - MUST_BE_LOCKED(&ip_ftp_lock); - sprintf(buffer, "|||%u|", port); DEBUGP("calling ip_nat_mangle_tcp_packet\n"); @@ -166,7 +155,7 @@ [IP_CT_FTP_EPSV] mangle_epsv_packet }; -static int ftp_data_fixup(const struct ip_ct_ftp_expect *ct_ftp_info, +static int ftp_data_fixup(const struct ip_ct_ftp_expect *exp_ftp_info, struct ip_conntrack *ct, struct sk_buff **pskb, enum ip_conntrack_info ctinfo, @@ -178,15 +167,14 @@ 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, ct_ftp_info->len, + expect->seq, exp_ftp_info->len, ntohl(tcph->seq)); /* Change address inside packet to match way we're mapping this connection. */ - if (ct_ftp_info->ftptype == IP_CT_FTP_PASV - || ct_ftp_info->ftptype == IP_CT_FTP_EPSV) { + if (exp_ftp_info->ftptype == IP_CT_FTP_PASV + || exp_ftp_info->ftptype == IP_CT_FTP_EPSV) { /* PASV/EPSV response: must be where client thinks server is */ newip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip; @@ -208,7 +196,7 @@ newtuple.src.u.tcp.port = expect->tuple.src.u.tcp.port; /* Try to get same port: if not, try to change it. */ - for (port = ct_ftp_info->port; port != 0; port++) { + for (port = exp_ftp_info->port; port != 0; port++) { newtuple.dst.u.tcp.port = htons(port); if (ip_conntrack_change_expect(expect, &newtuple) == 0) @@ -217,9 +205,9 @@ if (port == 0) return 0; - if (!mangle[ct_ftp_info->ftptype](pskb, newip, port, + if (!mangle[exp_ftp_info->ftptype](pskb, newip, port, expect->seq - ntohl(tcph->seq), - ct_ftp_info->len, ct, ctinfo)) + exp_ftp_info->len, ct, ctinfo)) return 0; return 1; @@ -236,12 +224,12 @@ struct tcphdr *tcph = (void *)iph + iph->ihl*4; unsigned int datalen; int dir; - struct ip_ct_ftp_expect *ct_ftp_info; + struct ip_ct_ftp_expect *exp_ftp_info; if (!exp) DEBUGP("ip_nat_ftp: no exp!!"); - ct_ftp_info = &exp->help.exp_ftp_info; + exp_ftp_info = &exp->help.exp_ftp_info; /* Only mangle things once: original direction in POST_ROUTING and reply direction on PRE_ROUTING. */ @@ -257,28 +245,23 @@ } 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 + ct_ftp_info->len, + if (between(exp->seq + exp_ftp_info->len, ntohl(tcph->seq), ntohl(tcph->seq) + datalen)) { - if (!ftp_data_fixup(ct_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. */ if (net_ratelimit()) { printk("FTP_NAT: partial packet %u/%u in %u/%u\n", - exp->seq, ct_ftp_info->len, + exp->seq, exp_ftp_info->len, 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 Sat Nov 15 16:23:47 2003 +++ b/net/ipv4/netfilter/ip_nat_irc.c Sat Nov 15 16:23:47 2003 @@ -46,9 +46,6 @@ MODULE_PARM_DESC(ports, "port numbers of IRC servers"); #endif -/* protects irc part of conntracks */ -DECLARE_LOCK_EXTERN(ip_irc_lock); - /* FIXME: Time out? --RR */ static unsigned int @@ -89,7 +86,7 @@ return ip_nat_setup_info(ct, &mr, hooknum); } -static int irc_data_fixup(const struct ip_ct_irc_expect *ct_irc_info, +static int irc_data_fixup(const struct ip_ct_irc_expect *exp_irc_info, struct ip_conntrack *ct, struct sk_buff **pskb, enum ip_conntrack_info ctinfo, @@ -104,23 +101,17 @@ /* "4294967296 65635 " */ char buffer[18]; - MUST_BE_LOCKED(&ip_irc_lock); - DEBUGP("IRC_NAT: info (seq %u + %u) in %u\n", - expect->seq, ct_irc_info->len, + expect->seq, exp_irc_info->len, ntohl(tcph->seq)); 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 = ct_irc_info->port; port != 0; port++) { + for (port = exp_irc_info->port; port != 0; port++) { t.dst.u.tcp.port = htons(port); if (ip_conntrack_change_expect(expect, &t) == 0) { DEBUGP("using port %d", port); @@ -150,7 +141,7 @@ return ip_nat_mangle_tcp_packet(pskb, ct, ctinfo, expect->seq - ntohl(tcph->seq), - ct_irc_info->len, buffer, + exp_irc_info->len, buffer, strlen(buffer)); } @@ -165,12 +156,12 @@ struct tcphdr *tcph = (void *) iph + iph->ihl * 4; unsigned int datalen; int dir; - struct ip_ct_irc_expect *ct_irc_info; + struct ip_ct_irc_expect *exp_irc_info; if (!exp) DEBUGP("ip_nat_irc: no exp!!"); - ct_irc_info = &exp->help.exp_irc_info; + exp_irc_info = &exp->help.exp_irc_info; /* Only mangle things once: original direction in POST_ROUTING and reply direction on PRE_ROUTING. */ @@ -187,29 +178,24 @@ DEBUGP("got beyond not touching\n"); datalen = (*pskb)->len - iph->ihl * 4 - tcph->doff * 4; - LOCK_BH(&ip_irc_lock); /* Check wether the whole IP/address pattern is carried in the payload */ - if (between(exp->seq + ct_irc_info->len, + if (between(exp->seq + exp_irc_info->len, ntohl(tcph->seq), ntohl(tcph->seq) + datalen)) { - if (!irc_data_fixup(ct_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. */ if (net_ratelimit()) { printk ("IRC_NAT: partial packet %u/%u in %u/%u\n", - exp->seq, ct_irc_info->len, + exp->seq, exp_irc_info->len, ntohl(tcph->seq), ntohl(tcph->seq) + datalen); } - UNLOCK_BH(&ip_irc_lock); return NF_DROP; } - UNLOCK_BH(&ip_irc_lock); return NF_ACCEPT; } --------------020206090900050409020608--