Skip to contentSkip to conversation

Fix expectation leak

PATCH In review Pablo Neira 2 participants 5 messages Aug 1, 2005 – Aug 2, 2005 1 message not in the archive

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 1/7 Fix expectation leak

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.

net/ipv4/netfilter/ip_conntrack_core.c +0 -1
Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
@@ -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);
#1 lore raw
Quoted 3 lines · Pablo Neira

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.

Quoted 14 lines

------------------------------------------------------------------------

Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================

net/ipv4/netfilter/ip_conntrack_core.c +0 -1
@@ -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);

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 1 line · Pablo Neira

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.

author
#4 lore raw
Quoted 8 lines · Patrick McHardy
Quoted 1 line · Pablo Neira

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

net/ipv4/netfilter/ip_conntrack_core.c +4 -4
Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
@@ -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);
 		}
 	}
#5 lore raw
Quoted 13 lines · Pablo Neira
Quoted 9 lines · Patrick McHardy
Quoted 1 line · Pablo Neira

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.

Quoted 6 lines

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.