[PATCH 1/7] Fix expectation leak

[PATCH 1/7] Fix expectation leak

[Pablo Neira]

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

expectation refcount is set to 1 in ip_conntrack_expect_alloc, and 
incremented again in ip_conntrack_expect_related. So once the 
ip_conntrack_expect_free is called, the expectation is never released.

[-- Attachment #2: 00fix-leak-expect.patch --]
[-- Type: text/x-patch, Size: 532 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-01 16:20:26.000000000 +0200
+++ netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c	2005-08-01 16:24:47.000000000 +0200
@@ -970,7 +970,6 @@
 
 static void ip_conntrack_expect_insert(struct ip_conntrack_expect *exp)
 {
-	atomic_inc(&exp->use);
 	exp->master->expecting++;
 	list_add(&exp->list, &ip_conntrack_expect_list);
 

Re: [PATCH 1/7] Fix expectation leak

[Patrick McHardy]

Pablo Neira wrote:
> expectation refcount is set to 1 in ip_conntrack_expect_alloc, and
> incremented again in ip_conntrack_expect_related. So once the
> ip_conntrack_expect_free is called, the expectation is never released.

This looks wrong. expect_alloc sets it to 1, expect_insert increases it
too 2, the helper then sets puts it when done with it, so it is 1 while
inserted.

> ------------------------------------------------------------------------
> 
> 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-01 16:20:26.000000000 +0200
> +++ netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c	2005-08-01 16:24:47.000000000 +0200
> @@ -970,7 +970,6 @@
>  
>  static void ip_conntrack_expect_insert(struct ip_conntrack_expect *exp)
>  {
> -	atomic_inc(&exp->use);
>  	exp->master->expecting++;
>  	list_add(&exp->list, &ip_conntrack_expect_list);
>  

Re: [PATCH 1/7] Fix expectation leak

[Patrick McHardy]

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.

Re: [PATCH 1/7] Fix expectation leak

[Pablo Neira]

[-- 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);
 		}
 	}

Re: [PATCH 1/7] Fix expectation leak

[Patrick McHardy]

Pablo Neira wrote:
> 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.

Seems like you missed a few. Whenever del_timer succeeds we need one
expect_put for the timer and need to call the reference-dropping
variant of unlink_expect. In the expectation timer we need to call
this variant too. This doesn't leave a single case where we use
the other one, so please add the expect_put to unlink_expect.