Re: [PATCH 1/7] Fix expectation leak

[Pablo Neira <pablo@eurodev.net>]

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

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>>Yes, but the refcount is incremented twice ;). Please apply.
> 
> There is one reference for the list and one for the timer. Both are
> needed since the timer deletion can race with the timer. The one
> for the list is dropped in __ip_ct_expect_unlink_destroy, the one
> for the timer after it has been successfully removed.

OK, in that case the following patch should fix the hung that I'm 
experimenting while trying to remove ip_conntrack. It happens if there's 
an expectation pending to be confirmed. It seems that we forgot to drop 
the refcount when removing a conntrack the list, unlink_expect doesn't 
actually do it. So, in this case the correct call must be 
__ip_conntrack_expect_unlink_destroy instead of unlink_expect.

--
Pablo

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

Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
--- netfilter-2.6.14.orig/net/ipv4/netfilter/ip_conntrack_core.c	2005-08-02 12:47:39.000000000 +0200
+++ netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c	2005-08-02 14:28:01.000000000 +0200
@@ -294,7 +294,7 @@
 
 	list_for_each_entry_safe(i, tmp, &ip_conntrack_expect_list, list) {
 		if (i->master == ct && del_timer(&i->timeout)) {
-			unlink_expect(i);
+			__ip_ct_expect_unlink_destroy(i);
 			ip_conntrack_expect_put(i);
 		}
 	}
@@ -936,7 +936,7 @@
 	/* choose the the oldest expectation to evict */
 	list_for_each_entry_reverse(i, &ip_conntrack_expect_list, list) {
 		if (expect_matches(i, exp) && del_timer(&i->timeout)) {
-			unlink_expect(i);
+			__ip_ct_expect_unlink_destroy(i);
 			write_unlock_bh(&ip_conntrack_lock);
 			ip_conntrack_expect_put(i);
 			return;
@@ -993,7 +993,7 @@
 	list_for_each_entry_reverse(i, &ip_conntrack_expect_list, list) {
 		if (i->master == master) {
 			if (del_timer(&i->timeout)) {
-				unlink_expect(i);
+				__ip_ct_expect_unlink_destroy(i);
 				ip_conntrack_expect_put(i);
 			}
 			break;
@@ -1110,7 +1110,7 @@
 	/* Get rid of expectations */
 	list_for_each_entry_safe(exp, tmp, &ip_conntrack_expect_list, list) {
 		if (exp->master->helper == me && del_timer(&exp->timeout)) {
-			unlink_expect(exp);
+			__ip_ct_expect_unlink_destroy(exp);
 			ip_conntrack_expect_put(exp);
 		}
 	}