netdev
[Top] [All Lists]

Re: [PATCH] Prevent crash on ip_conntrack removal

To: "David S. Miller" <davem@xxxxxxxxxx>
Subject: Re: [PATCH] Prevent crash on ip_conntrack removal
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Thu, 19 Aug 2004 16:55:58 +0200
Cc: Harald Welte <laforge@xxxxxxxxxxxxx>, okir@xxxxxxx, netdev@xxxxxxxxxxx, netfilter-devel@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20040819071846.2d0d6120.davem@xxxxxxxxxx>
References: <20040818091352.GB6507@xxxxxxx> <20040819101159.GC3921@xxxxxxxxxxxxxxxxxxxxxxx> <20040819071846.2d0d6120.davem@xxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5
David S. Miller wrote:

On Thu, 19 Aug 2004 12:11:59 +0200
Harald Welte <laforge@xxxxxxxxxxxxx> wrote:

Dave, is this fine with you?  What solution would you prefer?

I haven't been shown the patches so I can't generate an
opinion. :-)

These are Olaf's patches. I agree with Harald that the second
patch is better. I've fixed it up so it applies with the recent
ip_fragment.c changes.

Index: v2.6.8/net/ipv4/ip_input.c
===================================================================
--- v2.6.8.orig/net/ipv4/ip_input.c
+++ v2.6.8/net/ipv4/ip_input.c
@@ -177,6 +177,13 @@ int ip_call_ra_chain(struct sk_buff *skb
                                        read_unlock(&ip_ra_lock);
                                        return 1;
                                }
+                               /* When ip_conntrack gets unloaded, we may be
+                                * left with fragment chains where the first
+                                * fragment has skb->dst = NULL. */
+                               if (skb->dst == NULL) {
+                                        kfree_skb(skb);
+                                        return 1;
+                                }
                        }
                        if (last) {
                                struct sk_buff *skb2 = skb_clone(skb, 
GFP_ATOMIC);
@@ -277,6 +284,13 @@ int ip_local_deliver(struct sk_buff *skb
                skb = ip_defrag(skb);
                if (!skb)
                        return 0;
+               /* When ip_conntrack gets unloaded, we may be
+                * left with fragment chains where the first
+                * fragment has skb->dst = NULL. */
+               if (skb->dst == NULL) {
+                        kfree_skb(skb);
+                        return 0;
+                }
        }
 
        return NF_HOOK(PF_INET, NF_IP_LOCAL_IN, skb, skb->dev, NULL,
Alternative fix for the crash on conntrack unload. Simply flush all
fragment queues when unloading conntrack_standalone so that there are
no partially assembled fragments left with skb->dst == NULL.

Index: v2.6.8/include/net/ip.h
===================================================================
--- v2.6.8.orig/include/net/ip.h
+++ v2.6.8/include/net/ip.h
@@ -255,6 +255,7 @@ extern int  ip_call_ra_chain(struct sk_bu
  */
  
 struct sk_buff *ip_defrag(struct sk_buff *skb);
+extern void ipfrag_flush(void);
 extern int ip_frag_nqueues;
 extern atomic_t ip_frag_mem;
 
Index: v2.6.8/net/ipv4/ip_fragment.c
===================================================================
--- v2.6.8.orig/net/ipv4/ip_fragment.c
+++ v2.6.8/net/ipv4/ip_fragment.c
@@ -239,13 +239,13 @@ static void ipq_kill(struct ipq *ipq)
 /* Memory limiting on fragments.  Evictor trashes the oldest 
  * fragment queue until we are back under the low threshold.
  */
-static void ip_evictor(void)
+static void __ip_evictor(int threshold)
 {
        struct ipq *qp;
        struct list_head *tmp;
 
        for(;;) {
-               if (atomic_read(&ip_frag_mem) <= sysctl_ipfrag_low_thresh)
+               if (atomic_read(&ip_frag_mem) <= threshold)
                        return;
                read_lock(&ipfrag_lock);
                if (list_empty(&ipq_lru_list)) {
@@ -267,6 +267,11 @@ static void ip_evictor(void)
        }
 }
 
+static inline void ip_evictor(void)
+{
+       __ip_evictor(sysctl_ipfrag_low_thresh);
+}
+
 /*
  * Oops, a fragment queue timed out.  Kill it and send an ICMP reply.
  */
@@ -677,4 +682,10 @@ void ipfrag_init(void)
        add_timer(&ipfrag_secret_timer);
 }
 
+void ipfrag_flush(void)
+{
+       __ip_evictor(0);
+}
+
 EXPORT_SYMBOL(ip_defrag);
+EXPORT_SYMBOL(ipfrag_flush);
Index: v2.6.8/net/ipv4/netfilter/ip_conntrack_standalone.c
===================================================================
--- v2.6.8.orig/net/ipv4/netfilter/ip_conntrack_standalone.c
+++ v2.6.8/net/ipv4/netfilter/ip_conntrack_standalone.c
@@ -562,6 +562,8 @@ static int init_or_cleanup(int init)
        nf_unregister_hook(&ip_conntrack_defrag_local_out_ops);
  cleanup_defragops:
        nf_unregister_hook(&ip_conntrack_defrag_ops);
+       /* Frag queues may hold fragments with skb->dst == NULL */
+       ipfrag_flush();
  cleanup_proc:
        proc_net_remove("ip_conntrack");
  cleanup_init:
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/08/19 16:51:10+02:00 kaber@xxxxxxxxxxxx 
#   [NETFILTER]: Flush ip fragment queue on conntrack module unload
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
# net/ipv4/netfilter/ip_conntrack_standalone.c
#   2004/08/19 16:50:47+02:00 kaber@xxxxxxxxxxxx +2 -0
#   [NETFILTER]: Flush ip fragment queue on conntrack module unload
# 
# net/ipv4/ip_fragment.c
#   2004/08/19 16:50:47+02:00 kaber@xxxxxxxxxxxx +14 -3
#   [NETFILTER]: Flush ip fragment queue on conntrack module unload
# 
# include/net/ip.h
#   2004/08/19 16:50:47+02:00 kaber@xxxxxxxxxxxx +1 -0
#   [NETFILTER]: Flush ip fragment queue on conntrack module unload
# 
diff -Nru a/include/net/ip.h b/include/net/ip.h
--- a/include/net/ip.h  2004-08-19 16:53:39 +02:00
+++ b/include/net/ip.h  2004-08-19 16:53:39 +02:00
@@ -255,6 +255,7 @@
  */
  
 struct sk_buff *ip_defrag(struct sk_buff *skb);
+extern void ipfrag_flush(void);
 extern int ip_frag_nqueues;
 extern atomic_t ip_frag_mem;
 
diff -Nru a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
--- a/net/ipv4/ip_fragment.c    2004-08-19 16:53:38 +02:00
+++ b/net/ipv4/ip_fragment.c    2004-08-19 16:53:38 +02:00
@@ -241,15 +241,15 @@
 }
 
 /* Memory limiting on fragments.  Evictor trashes the oldest 
- * fragment queue until we are back under the low threshold.
+ * fragment queue until we are back under the threshold.
  */
-static void ip_evictor(void)
+static void __ip_evictor(int threshold)
 {
        struct ipq *qp;
        struct list_head *tmp;
        int work;
 
-       work = atomic_read(&ip_frag_mem) - sysctl_ipfrag_low_thresh;
+       work = atomic_read(&ip_frag_mem) - threshold;
        if (work <= 0)
                return;
 
@@ -274,6 +274,11 @@
        }
 }
 
+static inline void ip_evictor(void)
+{
+       __ip_evictor(sysctl_ipfrag_low_thresh);
+}
+
 /*
  * Oops, a fragment queue timed out.  Kill it and send an ICMP reply.
  */
@@ -684,4 +689,10 @@
        add_timer(&ipfrag_secret_timer);
 }
 
+void ipfrag_flush(void)
+{
+       __ip_evictor(0);
+}
+
 EXPORT_SYMBOL(ip_defrag);
+EXPORT_SYMBOL(ipfrag_flush);
diff -Nru a/net/ipv4/netfilter/ip_conntrack_standalone.c 
b/net/ipv4/netfilter/ip_conntrack_standalone.c
--- a/net/ipv4/netfilter/ip_conntrack_standalone.c      2004-08-19 16:53:38 
+02:00
+++ b/net/ipv4/netfilter/ip_conntrack_standalone.c      2004-08-19 16:53:39 
+02:00
@@ -806,6 +806,8 @@
        nf_unregister_hook(&ip_conntrack_defrag_local_out_ops);
  cleanup_defragops:
        nf_unregister_hook(&ip_conntrack_defrag_ops);
+       /* Frag queues may hold fragments with skb->dst == NULL */
+       ipfrag_flush();
  cleanup_proc_stat:
        proc_net_remove("ip_conntrack_stat");
 cleanup_proc_exp:
<Prev in Thread] Current Thread [Next in Thread>