[PATCH] Fix hang on netfilter module unload

[PATCH] Fix hang on netfilter module unload

[Phil Oester]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

At module unload time, we use ip_conntrack_count to determine how many
conntracks need to be destroyed before we can unload.  get_next_corpse
looks in the hash tables and unconfirmed list for the conntracks until
ip_conntrack_count == 0.  

Unfortunately, conntracks occasionally get pinned by an skb which for
some reason or another won't go away.  So ct_general->use never reduces
to 1 for these conntracks, and they do not appear in either the
hashes or the unconfirmed list.  As such, we loop around forever at
i_see_dead_people trying to kill a conntrack which we will never find.

The below patch attempts to fix this by doing two things:

1) when a conntrack is removed from the hashes in clean_from_lists,
   add it to a new 'cleaned' list, similar to how the unconfirmed list
   works.  This ensures that we never lose sight of a conntrack --
   it moves from unconfirmed->hashed->cleaned in its lifetime.
   get_next_corpse now checks the cleaned list for conntracks also.

2) change get_next_corpse to set the usage count of conntracks to
   1 once they are found.  Otherwise, these pinned conntracks will never
   be able to be removed, since use will be > 1 

Without the below, I can trivially hang a box on module unload by
using NetworkManager on FC3 -- seems to be related to the DHCP process.
With this patch, unload works every time.

This fixes Netfilter bugzilla #91 and Redhat bugzilla #112630.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>




[-- Attachment #2: patch-unload --]
[-- Type: text/plain, Size: 1834 bytes --]

diff -ruN linux-orig/net/ipv4/netfilter/ip_conntrack_core.c linux-new/net/ipv4/netfilter/ip_conntrack_core.c
--- linux-orig/net/ipv4/netfilter/ip_conntrack_core.c	2005-03-02 02:37:30.000000000 -0500
+++ linux-new/net/ipv4/netfilter/ip_conntrack_core.c	2005-03-14 20:18:46.289349488 -0500
@@ -74,6 +74,7 @@
 struct ip_conntrack ip_conntrack_untracked;
 unsigned int ip_ct_log_invalid;
 static LIST_HEAD(unconfirmed);
+static LIST_HEAD(cleaned);
 static int ip_conntrack_vmalloc;
 
 DEFINE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
@@ -216,6 +217,9 @@
 	LIST_DELETE(&ip_conntrack_hash[ho], &ct->tuplehash[IP_CT_DIR_ORIGINAL]);
 	LIST_DELETE(&ip_conntrack_hash[hr], &ct->tuplehash[IP_CT_DIR_REPLY]);
 
+	/* Overload tuple linked list to put us in cleaned list. */
+	list_add(&ct->tuplehash[IP_CT_DIR_ORIGINAL].list, &cleaned);
+
 	/* Destroy all pending expectations */
 	remove_expectations(ct);
 }
@@ -247,11 +251,9 @@
 	 * too. */
 	remove_expectations(ct);
 
-	/* We overload first tuple to link into unconfirmed list. */
-	if (!is_confirmed(ct)) {
-		BUG_ON(list_empty(&ct->tuplehash[IP_CT_DIR_ORIGINAL].list));
-		list_del(&ct->tuplehash[IP_CT_DIR_ORIGINAL].list);
-	}
+	/* We overload first tuple to link into unconfirmed or cleaned list.
+	   Will always be on one or the other at this point */
+	list_del(&ct->tuplehash[IP_CT_DIR_ORIGINAL].list);
 
 	CONNTRACK_STAT_INC(delete);
 	WRITE_UNLOCK(&ip_conntrack_lock);
@@ -1019,8 +1021,11 @@
 	if (!h)
 		h = LIST_FIND_W(&unconfirmed, do_iter,
 				struct ip_conntrack_tuple_hash *, iter, data);
+	if (!h)
+		h = LIST_FIND_W(&cleaned, do_iter,
+				struct ip_conntrack_tuple_hash *, iter, data);
 	if (h)
-		atomic_inc(&tuplehash_to_ctrack(h)->ct_general.use);
+		atomic_set(&tuplehash_to_ctrack(h)->ct_general.use, 1);
 	WRITE_UNLOCK(&ip_conntrack_lock);
 
 	return h;

Re: [PATCH] Fix hang on netfilter module unload

[Pablo Neira]

Hi Phil,

Phil Oester wrote:
> Unfortunately, conntracks occasionally get pinned by an skb which for
> some reason or another won't go away.  So ct_general->use never reduces
> to 1 for these conntracks

So we are leaking conntracks for some reason that we must diagnose.


> @@ -1019,8 +1021,11 @@
>  	if (!h)
>  		h = LIST_FIND_W(&unconfirmed, do_iter,
>  				struct ip_conntrack_tuple_hash *, iter, data);
> +	if (!h)
> +		h = LIST_FIND_W(&cleaned, do_iter,
> +				struct ip_conntrack_tuple_hash *, iter, data);
>  	if (h)
> -		atomic_inc(&tuplehash_to_ctrack(h)->ct_general.use);
> +		atomic_set(&tuplehash_to_ctrack(h)->ct_general.use, 1);

But this is cheating, of course that the module will unload if we set 
all refcounts to 1 we won't spin forever in the i_see_dead_people loop 
but we are leaking somewhere anyway.

--
Pablo

Re: [PATCH] Fix hang on netfilter module unload

[Phil Oester]

> So we are leaking conntracks for some reason that we must diagnose.

No...the app is leaking skbs.  Since the skb never goes away (at least
until closing the app), the conntrack can thus not go away.  Isn't
it better for us to work around stupid apps like this?  Seems preferable
to not hang the box on iptables restart/unload, eh?

> But this is cheating, of course that the module will unload if we set 
> all refcounts to 1 we won't spin forever in the i_see_dead_people loop 
> but we are leaking somewhere anyway.

Well, again, the admin has asked to unload iptables -- should we unload,
or lockup the box?

Phil

Re: [PATCH] Fix hang on netfilter module unload

[Patrick McHardy]

Phil Oester wrote:
>>So we are leaking conntracks for some reason that we must diagnose.
> 
> 
> No...the app is leaking skbs.  Since the skb never goes away (at least
> until closing the app), the conntrack can thus not go away.  Isn't
> it better for us to work around stupid apps like this?  Seems preferable
> to not hang the box on iptables restart/unload, eh?

I guess by leaking you mean doesn't pull them of the receive queue.
Anyhow, the reference to the conntrack is dropped before packets
are delivered to the protocols, so this can't be the reason. Do
you have a reproducable test-case ?

>>But this is cheating, of course that the module will unload if we set 
>>all refcounts to 1 we won't spin forever in the i_see_dead_people loop 
>>but we are leaking somewhere anyway.
> 
> 
> Well, again, the admin has asked to unload iptables -- should we unload,
> or lockup the box?

Unload - after fixing the leaks :) If we cover it up this way it will
never get fixed properly.

Regards
Patrick

Re: [PATCH] Fix hang on netfilter module unload

[Phil Oester]

On Tue, Mar 15, 2005 at 08:25:17PM +0100, Patrick McHardy wrote:
> I guess by leaking you mean doesn't pull them of the receive queue.
> Anyhow, the reference to the conntrack is dropped before packets
> are delivered to the protocols, so this can't be the reason. Do
> you have a reproducable test-case ?

On a box with modular iptables, a DHCP interface, and NetworkManager
set to run on boot:

1) boot box
2) verify ip_conntrack_count and wc -l /proc/net/ip_conntrack are both 0
3) NetworkManager stop
4) ifdown eth0
5) NetworkManager start

NM seems to never release the reference to at least one of its
conntracks (sometimes more than one).  At this point, a conntrack
has been cleaned from lists but not destroyed, and the counts
shown in #2 will not match.  

It seems to me we should still track conntracks which are cleaned
but not yet destroyed, no?  Isn't it possible for a malicious
app to screw us here by intentionally getting the counts out of
whack?

Phil

Re: [PATCH] Fix hang on netfilter module unload

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

Phil Oester wrote:
> On a box with modular iptables, a DHCP interface, and NetworkManager
> set to run on boot:

What is NetworkManager? Can I download the source somewhere?

> 1) boot box
> 2) verify ip_conntrack_count and wc -l /proc/net/ip_conntrack are both 0
> 3) NetworkManager stop
> 4) ifdown eth0
> 5) NetworkManager start
> 
> NM seems to never release the reference to at least one of its
> conntracks (sometimes more than one).  At this point, a conntrack
> has been cleaned from lists but not destroyed, and the counts
> shown in #2 will not match.  
> 
> It seems to me we should still track conntracks which are cleaned
> but not yet destroyed, no?  Isn't it possible for a malicious
> app to screw us here by intentionally getting the counts out of
> whack?

I don't think it is related to the specific application.
More likely the packet is queued while the device is down
and never dequeued. Does this patch fix the problem ?

Regards
Patrick

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 387 bytes --]

===== net/core/dev.c 1.184 vs edited =====
--- 1.184/net/core/dev.c	2005-02-09 01:20:02 +01:00
+++ edited/net/core/dev.c	2005-03-15 20:53:16 +01:00
@@ -1246,6 +1246,9 @@
 	      	if (skb_checksum_help(skb, 0))
 	      		goto out_kfree_skb;
 
+	nf_conntrack_put(skb->nfct);
+	skb->nfct = NULL;
+
 	/* Disable soft irqs for various locks below. Also 
 	 * stops preemption for RCU. 
 	 */

Re: [PATCH] Fix hang on netfilter module unload

[Patrick McHardy]

Patrick McHardy wrote:
> I don't think it is related to the specific application.
> More likely the packet is queued while the device is down
> and never dequeued. Does this patch fix the problem ?

Actually this can't happen since dev_deactivate switches the
qdisc with a noop_qdisc when the device is set down.

Regards
Patrick

Re: [PATCH] Fix hang on netfilter module unload

[Patrick McHardy]

Patrick McHardy wrote:
> Patrick McHardy wrote:
> 
>> I don't think it is related to the specific application.
>> More likely the packet is queued while the device is down
>> and never dequeued. Does this patch fix the problem ?
> 
> 
> Actually this can't happen since dev_deactivate switches the
> qdisc with a noop_qdisc when the device is set down.

Could you try it anyway ? It will help narrow down the
area where this leak happens.

Regards
Patrick

Re: [PATCH] Fix hang on netfilter module unload

[Phil Oester]

On Tue, Mar 15, 2005 at 08:53:57PM +0100, Patrick McHardy wrote:
> Phil Oester wrote:
> >On a box with modular iptables, a DHCP interface, and NetworkManager
> >set to run on boot:
> 
> What is NetworkManager? Can I download the source somewhere?

It's suposed to manage network connections, choosing the best available
interface.  The SRPM for the version I'm testing with is available here:

http://mirrors.kernel.org/fedora/core/updates/3/SRPMS/NetworkManager-0.3.3-1.cvs20050119.2.fc3.src.rpm

Phil