netdev
[Top] [All Lists]

Re: [PATCH] Prevent crash on ip_conntrack removal

To: Patrick McHardy <kaber@xxxxxxxxx>
Subject: Re: [PATCH] Prevent crash on ip_conntrack removal
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Sun, 29 Aug 2004 23:48:28 +0200
Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>, David Stevens <dlstevens@xxxxxxxxxx>, davem@xxxxxxxxxx, laforge@xxxxxxxxxxxxx, netdev@xxxxxxxxxxx, netdev-bounce@xxxxxxxxxxx, netfilter-devel@xxxxxxxxxxxxxxxxxxx, okir@xxxxxxx
In-reply-to: <4132303C.2060807@trash.net>
References: <412A8FB5.4080700@trash.net> <OFE08A3DF5.09BCA1C4-ON88256EFA.00806C05-88256EFA.0080E98F@us.ibm.com> <20040828231529.051a73cc.davem@davemloft.net> <4132303C.2060807@trash.net>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5
Patrick McHardy wrote:

Attached. The first patch still crashed, we need to prevent new
fragments from getting queued after the queue is flushed until the
hook in unregistered.

The patch is racy, another CPU could already have passed the check for ip_ct_no_defrag and queue the packet after __ip_evictor calculated the work to do, so the packet (or another one) will not get evicted. This patch on top calls synchronize_net() to prevent this.


@@ -1181,6 +1183,12 @@
#ifdef CONFIG_NETFILTER_DEBUG
unsigned int olddebug = skb->nf_debug;
#endif
+
+ if (unlikely(ip_ct_no_defrag)) {
+ kfree_skb(skb);
+ return NULL;
+ }
+
if (sk) {
sock_hold(sk);
skb_orphan(skb);
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-29 20:55:13 +02:00
+++ b/net/ipv4/netfilter/ip_conntrack_standalone.c 2004-08-29 20:55:13 +02:00
@@ -805,6 +805,12 @@
cleanup_defraglocalops:
nf_unregister_hook(&ip_conntrack_defrag_local_out_ops);
cleanup_defragops:
+ /* Frag queues may hold fragments with skb->dst == NULL */
+ ip_ct_no_defrag = 1;
+ smp_wmb();
+ local_bh_disable();
+ ipfrag_flush();
+ local_bh_enable();
nf_unregister_hook(&ip_conntrack_defrag_ops);
cleanup_proc_stat:
proc_net_remove("ip_conntrack_stat");



# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/08/29 23:36:17+02:00 kaber@xxxxxxxxxxxx 
#   [NETFILTER]: Fix race when flushing fragment queue
#   
#   Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>
# 
# net/ipv4/netfilter/ip_conntrack_standalone.c
#   2004/08/29 23:35:54+02:00 kaber@xxxxxxxxxxxx +1 -1
#   [NETFILTER]: Fix race when flushing fragment queue
# 
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-29 23:39:07 
+02:00
+++ b/net/ipv4/netfilter/ip_conntrack_standalone.c      2004-08-29 23:39:07 
+02:00
@@ -807,7 +807,7 @@
  cleanup_defragops:
        /* Frag queues may hold fragments with skb->dst == NULL */
        ip_ct_no_defrag = 1;
-       smp_wmb();
+       synchronize_net();
        local_bh_disable();
        ipfrag_flush();
        local_bh_enable();
<Prev in Thread] Current Thread [Next in Thread>