[NETFILTER] Fix conntrack event cache deadlock/oops

This patch fixes a number of bugs.  It cannot be reasonably split up in
multiple fixes, since all bugs interact with each other and affect the same
function:

Bug #1:
The event cache code cannot be called while a lock is held.  Therefore, the
call to ip_conntrack_event_cache() within ip_ct_refresh_acct() needs to be
moved outside of the locked section.  This fixes a number of 2.6.14-rcX
oops and deadlock reports.

Bug #2:
We used to call ct_add_counters() for unconfirmed connections without
holding a lock.  Since the add operations are not atomic, we could race
with another CPU.

Bug #3:
ip_ct_refresh_acct() lost REFRESH events in some cases where refresh
(and the corresponding event) are desired, but no accounting shall be
performed.  Both, evenst and accounting implicitly depended on the skb
parameter bein non-null.   We now re-introduce a non-accounting
"ip_ct_refresh()" variant to explicitly state the desired behaviour.

Signed-off-by: Harald Welte <laforge@netfilter.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/netfilter_ipv4/ip_conntrack.h b/include/linux/netfilter_ipv4/ip_conntrack.h
index bace72a76cc45dea47944e26679d4473bff4e627..4ced3873681313ad4d9ead8edd868000a6cc7dbd 100644 (file)
--- a/include/linux/netfilter_ipv4/ip_conntrack.h
+++ b/include/linux/netfilter_ipv4/ip_conntrack.h
@@ -332,11 +332,28 @@ extern void need_ip_conntrack(void);
 extern int invert_tuplepr(struct ip_conntrack_tuple *inverse,
                          const struct ip_conntrack_tuple *orig);
 
+extern void __ip_ct_refresh_acct(struct ip_conntrack *ct,
+                                enum ip_conntrack_info ctinfo,
+                                const struct sk_buff *skb,
+                                unsigned long extra_jiffies,
+                                int do_acct);
+
+/* Refresh conntrack for this many jiffies and do accounting */
+static inline void ip_ct_refresh_acct(struct ip_conntrack *ct, 
+                                     enum ip_conntrack_info ctinfo,
+                                     const struct sk_buff *skb,
+                                     unsigned long extra_jiffies)
+{
+       __ip_ct_refresh_acct(ct, ctinfo, skb, extra_jiffies, 1);
+}
+
 /* Refresh conntrack for this many jiffies */
-extern void ip_ct_refresh_acct(struct ip_conntrack *ct,
-                              enum ip_conntrack_info ctinfo,
-                              const struct sk_buff *skb,
-                              unsigned long extra_jiffies);
+static inline void ip_ct_refresh(struct ip_conntrack *ct,
+                                const struct sk_buff *skb,
+                                unsigned long extra_jiffies)
+{
+       __ip_ct_refresh_acct(ct, 0, skb, extra_jiffies, 0);
+}
 
 /* These are for NAT.  Icky. */
 /* Update TCP window tracking data when NAT mangles the packet */

diff --git a/net/ipv4/netfilter/ip_conntrack_amanda.c b/net/ipv4/netfilter/ip_conntrack_amanda.c
index dc20881004bc57731b4ae348b8272d26622b863e..fa3f914117ec20f66e18b27f92b2cc7e58e47d9a 100644 (file)
--- a/net/ipv4/netfilter/ip_conntrack_amanda.c
+++ b/net/ipv4/netfilter/ip_conntrack_amanda.c
@@ -65,7 +65,7 @@ static int help(struct sk_buff **pskb,
 
        /* increase the UDP timeout of the master connection as replies from
         * Amanda clients to the server can be quite delayed */
-       ip_ct_refresh_acct(ct, ctinfo, NULL, master_timeout * HZ);
+       ip_ct_refresh(ct, *pskb, master_timeout * HZ);
 
        /* No data? */
        dataoff = (*pskb)->nh.iph->ihl*4 + sizeof(struct udphdr);

diff --git a/net/ipv4/netfilter/ip_conntrack_core.c b/net/ipv4/netfilter/ip_conntrack_core.c
index c1f82e0c81cf64d1c7f094621ff4f35fbd5b2d45..ea65dd3e517abc20e5cf14034f7c7511f979827b 100644 (file)
--- a/net/ipv4/netfilter/ip_conntrack_core.c
+++ b/net/ipv4/netfilter/ip_conntrack_core.c
@@ -1112,45 +1112,46 @@ void ip_conntrack_helper_unregister(struct ip_conntrack_helper *me)
        synchronize_net();
 }
 
-static inline void ct_add_counters(struct ip_conntrack *ct,
-                                  enum ip_conntrack_info ctinfo,
-                                  const struct sk_buff *skb)
-{
-#ifdef CONFIG_IP_NF_CT_ACCT
-       if (skb) {
-               ct->counters[CTINFO2DIR(ctinfo)].packets++;
-               ct->counters[CTINFO2DIR(ctinfo)].bytes += 
-                                       ntohs(skb->nh.iph->tot_len);
-       }
-#endif
-}
-
-/* Refresh conntrack for this many jiffies and do accounting (if skb != NULL) */
-void ip_ct_refresh_acct(struct ip_conntrack *ct, 
+/* Refresh conntrack for this many jiffies and do accounting if do_acct is 1 */
+void __ip_ct_refresh_acct(struct ip_conntrack *ct, 
                        enum ip_conntrack_info ctinfo,
                        const struct sk_buff *skb,
-                       unsigned long extra_jiffies)
+                       unsigned long extra_jiffies,
+                       int do_acct)
 {
+       int do_event = 0;
+
        IP_NF_ASSERT(ct->timeout.data == (unsigned long)ct);
+       IP_NF_ASSERT(skb);
+
+       write_lock_bh(&ip_conntrack_lock);
 
        /* If not in hash table, timer will not be active yet */
        if (!is_confirmed(ct)) {
                ct->timeout.expires = extra_jiffies;
-               ct_add_counters(ct, ctinfo, skb);
+               do_event = 1;
        } else {
-               write_lock_bh(&ip_conntrack_lock);
                /* Need del_timer for race avoidance (may already be dying). */
                if (del_timer(&ct->timeout)) {
                        ct->timeout.expires = jiffies + extra_jiffies;
                        add_timer(&ct->timeout);
-                       /* FIXME: We loose some REFRESH events if this function
-                        * is called without an skb.  I'll fix this later -HW */
-                       if (skb)
-                               ip_conntrack_event_cache(IPCT_REFRESH, skb);
+                       do_event = 1;
                }
-               ct_add_counters(ct, ctinfo, skb);
-               write_unlock_bh(&ip_conntrack_lock);
        }
+
+#ifdef CONFIG_IP_NF_CT_ACCT
+       if (do_acct) {
+               ct->counters[CTINFO2DIR(ctinfo)].packets++;
+               ct->counters[CTINFO2DIR(ctinfo)].bytes += 
+                                               ntohs(skb->nh.iph->tot_len);
+       }
+#endif
+
+       write_unlock_bh(&ip_conntrack_lock);
+
+       /* must be unlocked when calling event cache */
+       if (do_event)
+               ip_conntrack_event_cache(IPCT_REFRESH, skb);
 }
 
 #if defined(CONFIG_IP_NF_CONNTRACK_NETLINK) || \

diff --git a/net/ipv4/netfilter/ip_conntrack_helper_pptp.c b/net/ipv4/netfilter/ip_conntrack_helper_pptp.c
index 8236ee0fb0903cf1bb2014827e7c1b6e72a078a0..926a6684643dd0caf3d5378651fbfaf28097bb0c 100644 (file)
--- a/net/ipv4/netfilter/ip_conntrack_helper_pptp.c
+++ b/net/ipv4/netfilter/ip_conntrack_helper_pptp.c
@@ -172,7 +172,6 @@ static int destroy_sibling_or_exp(const struct ip_conntrack_tuple *t)
                DEBUGP("setting timeout of conntrack %p to 0\n", sibling);
                sibling->proto.gre.timeout = 0;
                sibling->proto.gre.stream_timeout = 0;
-               /* refresh_acct will not modify counters if skb == NULL */
                if (del_timer(&sibling->timeout))
                        sibling->timeout.function((unsigned long)sibling);
                ip_conntrack_put(sibling);

diff --git a/net/ipv4/netfilter/ip_conntrack_netbios_ns.c b/net/ipv4/netfilter/ip_conntrack_netbios_ns.c
index 71ef19d126d066eda819b325bb2f80eb1604534e..577bac22dcc6421e2afeccbce5f97889c3cdbf31 100644 (file)
--- a/net/ipv4/netfilter/ip_conntrack_netbios_ns.c
+++ b/net/ipv4/netfilter/ip_conntrack_netbios_ns.c
@@ -91,7 +91,7 @@ static int help(struct sk_buff **pskb,
        ip_conntrack_expect_related(exp);
        ip_conntrack_expect_put(exp);
 
-       ip_ct_refresh_acct(ct, ctinfo, NULL, timeout * HZ);
+       ip_ct_refresh(ct, *pskb, timeout * HZ);
 out:
        return NF_ACCEPT;
 }

diff --git a/net/ipv4/netfilter/ip_conntrack_standalone.c b/net/ipv4/netfilter/ip_conntrack_standalone.c
index d3c7808010ec0db617de4c089146206f51aad118..dd476b191f4b5c135802839a840aa6bb34f88ed6 100644 (file)
--- a/net/ipv4/netfilter/ip_conntrack_standalone.c
+++ b/net/ipv4/netfilter/ip_conntrack_standalone.c
@@ -989,7 +989,7 @@ EXPORT_SYMBOL(need_ip_conntrack);
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);
 EXPORT_SYMBOL(ip_ct_iterate_cleanup);
-EXPORT_SYMBOL(ip_ct_refresh_acct);
+EXPORT_SYMBOL(__ip_ct_refresh_acct);
 
 EXPORT_SYMBOL(ip_conntrack_expect_alloc);
 EXPORT_SYMBOL(ip_conntrack_expect_put);