Fix hang on netfilter module unload

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

diff -ruN linux-orig/net/ipv4/netfilter/ip_conntrack_core.c linux-new/net/ipv4/netfilter/ip_conntrack_core.c
@@ -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;

Hi Phil,

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?

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

Phil

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 ?

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

Regards
Patrick

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

What is NetworkManager? Can I download the source somewhere?

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

===== net/core/dev.c 1.184 vs edited =====

@@ -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. 
 	 */

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

Regards
Patrick

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

Regards
Patrick

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