From 7cd101a51b34e2cfe1f93543937a54f0dadc2cc2 Mon Sep 17 00:00:00 2001 From: Patrick McHardy Date: Thu, 2 Sep 2004 10:08:40 -0700 Subject: [PATCH] [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 Signed-off-by: Harald Welte Signed-off-by: David S. Miller --- .../linux/netfilter_ipv4/ip_conntrack_ftp.h | 5 ----- .../linux/netfilter_ipv4/ip_conntrack_irc.h | 5 ----- net/ipv4/netfilter/ip_conntrack_ftp.c | 3 +-- net/ipv4/netfilter/ip_conntrack_irc.c | 7 +++---- net/ipv4/netfilter/ip_nat_ftp.c | 20 +------------------ net/ipv4/netfilter/ip_nat_irc.c | 18 +---------------- 6 files changed, 6 insertions(+), 52 deletions(-) diff --git a/include/linux/netfilter_ipv4/ip_conntrack_ftp.h b/include/linux/netfilter_ipv4/ip_conntrack_ftp.h index 677db49e26fb7..2f85006c75f93 100644 --- a/include/linux/netfilter_ipv4/ip_conntrack_ftp.h +++ b/include/linux/netfilter_ipv4/ip_conntrack_ftp.h @@ -4,11 +4,6 @@ #ifdef __KERNEL__ -#include - -/* Protects ftp part of conntracks */ -DECLARE_LOCK_EXTERN(ip_ftp_lock); - #define FTP_PORT 21 #endif /* __KERNEL__ */ diff --git a/include/linux/netfilter_ipv4/ip_conntrack_irc.h b/include/linux/netfilter_ipv4/ip_conntrack_irc.h index 170248fa87bd1..0cd24a02d3605 100644 --- a/include/linux/netfilter_ipv4/ip_conntrack_irc.h +++ b/include/linux/netfilter_ipv4/ip_conntrack_irc.h @@ -33,13 +33,8 @@ struct ip_ct_irc_master { #ifdef __KERNEL__ -#include - #define IRC_PORT 6667 -/* Protects irc part of conntracks */ -DECLARE_LOCK_EXTERN(ip_irc_lock); - #endif /* __KERNEL__ */ #endif /* _IP_CONNTRACK_IRC_H */ diff --git a/net/ipv4/netfilter/ip_conntrack_ftp.c b/net/ipv4/netfilter/ip_conntrack_ftp.c index c1403a0cc7d43..df7008989c381 100644 --- a/net/ipv4/netfilter/ip_conntrack_ftp.c +++ b/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); diff --git a/net/ipv4/netfilter/ip_conntrack_irc.c b/net/ipv4/netfilter/ip_conntrack_irc.c index 0d0afe131e4ef..d007bc9950cf0 100644 --- a/net/ipv4/netfilter/ip_conntrack_irc.c +++ b/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 "); 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); diff --git a/net/ipv4/netfilter/ip_nat_ftp.c b/net/ipv4/netfilter/ip_nat_ftp.c index db2f0860ce9ac..0ea2f90eb87bf 100644 --- a/net/ipv4/netfilter/ip_nat_ftp.c +++ b/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; } diff --git a/net/ipv4/netfilter/ip_nat_irc.c b/net/ipv4/netfilter/ip_nat_irc.c index 6edc72c0c0be4..5eb60a25ac8e0 100644 --- a/net/ipv4/netfilter/ip_nat_irc.c +++ b/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; } -- 2.53.0