[PATCH] Drop expectation refcount after unlinking expectation

[PATCH] Drop expectation refcount after unlinking expectation

[Pablo Neira]

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

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the 
refcount must be dropped as well.

Signed-off-by: Pablo Neira Ayuso <pablo@eurodev.net>

This fixes the problem although I think that there's something wrong 
with current refcounting. Every time an expectation is created, the 
refcount of the master conntrack is incremented. Then, if the master 
conntrack is destroyed, say the timeout has expired, 
ip_ct_remove_expectation will be called. So firstly, all expectations 
linked to such conntrack are destroyed and then the conntrack itself. 
OK, my question is: why do we need to increase the master conntrack 
refcount for expectations, if they are always killed first? Actually I 
think that the master conntrack refcount should be increased once the 
expectation is confirmed but not in ip_conntrack_expect_alloc.

[-- Attachment #2: 04fix-leak.patch --]
[-- Type: text/x-patch, Size: 543 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-03 16:32:30.000000000 +0200
+++ netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c	2005-08-03 16:39:36.000000000 +0200
@@ -215,6 +215,7 @@
 	list_del(&exp->list);
 	CONNTRACK_STAT_INC(expect_delete);
 	exp->master->expecting--;
+	ip_conntrack_expect_put(exp);
 }
 
 void __ip_ct_expect_unlink_destroy(struct ip_conntrack_expect *exp)

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Patrick McHardy]

Pablo Neira wrote:
> This patch comes from the following thread:
> 
> [PATCH 6/7] Fix expectation creation
> 
> In unlink_expect, the expectation is removed from the list so the
> refcount must be dropped as well.

The patch is fine, but it leaves the somewhat broken function
__ip_ct_expect_unlink_destroy around. I'm not sure what the
intention of this function is, it looks like a lazy way of
killing conntracks without manually dropping the timer refcnt.
Anyway it invites people to use it in the wrong place and
should die IMO.

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Harald Welte]

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

On Fri, Aug 05, 2005 at 12:21:00PM +0200, Patrick McHardy wrote:
> Pablo Neira wrote:
> > This patch comes from the following thread:
> > 
> > [PATCH 6/7] Fix expectation creation
> > 
> > In unlink_expect, the expectation is removed from the list so the
> > refcount must be dropped as well.
> 
> The patch is fine, but it leaves the somewhat broken function
> __ip_ct_expect_unlink_destroy around. I'm not sure what the
> intention of this function is, it looks like a lazy way of
> killing conntracks without manually dropping the timer refcnt.
> Anyway it invites people to use it in the wrong place and
> should die IMO.

Ok, if it dies we'd have to export unlink_expect() for ctnetlink.

However, all places calling unlink_expect() also call expect_put()
immediately after it - with one exception: find_expectation() :(

Getting back to your comment:  
> it looks like a lazy way of killing conntracks without manually
> dropping the timer refcnt.

Sorry, I can't follow you: Did you mean something like 'lazy way of
killing expects without manually expiring the timer' ?  

So your whole objection is to not have timer_del() inside
__ip_ct_expect_unlink_destroy() ?   this can be changed easily.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Patrick McHardy]

Harald Welte wrote:
> On Fri, Aug 05, 2005 at 12:21:00PM +0200, Patrick McHardy wrote:
> 
>>Pablo Neira wrote:
>>
>>>This patch comes from the following thread:
>>>
>>>[PATCH 6/7] Fix expectation creation
>>>
>>>In unlink_expect, the expectation is removed from the list so the
>>>refcount must be dropped as well.
>>
>>The patch is fine, but it leaves the somewhat broken function
>>__ip_ct_expect_unlink_destroy around. I'm not sure what the
>>intention of this function is, it looks like a lazy way of
>>killing conntracks without manually dropping the timer refcnt.
>>Anyway it invites people to use it in the wrong place and
>>should die IMO.
> 
> Ok, if it dies we'd have to export unlink_expect() for ctnetlink.
> 
> However, all places calling unlink_expect() also call expect_put()
> immediately after it - with one exception: find_expectation() :(
> 
> Getting back to your comment:  
> 
>>it looks like a lazy way of killing conntracks without manually
>>dropping the timer refcnt.
> 
> Sorry, I can't follow you: Did you mean something like 'lazy way of
> killing expects without manually expiring the timer' ?  
> 
> So your whole objection is to not have timer_del() inside
> __ip_ct_expect_unlink_destroy() ?   this can be changed easily.

My objection was the other way around, unlink_expect drops the
reference taken for the lists, as it should. 
__ip_ct_expect_unlink_destroy() additionally drops a reference
which I assume is the timer reference, but it doesn't delete
the timer. If the caller deletes the timer, he should be the
one to drop the reference. Your suggestion is of course perfectly
fine with me, if the timer is deleted within that function it
can also drop the reference.

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Patrick McHardy]

Pablo Neira wrote:
> This fixes the problem although I think that there's something wrong
> with current refcounting. Every time an expectation is created, the
> refcount of the master conntrack is incremented. Then, if the master
> conntrack is destroyed, say the timeout has expired,
> ip_ct_remove_expectation will be called. So firstly, all expectations
> linked to such conntrack are destroyed and then the conntrack itself.
> OK, my question is: why do we need to increase the master conntrack
> refcount for expectations, if they are always killed first? Actually I
> think that the master conntrack refcount should be increased once the
> expectation is confirmed but not in ip_conntrack_expect_alloc.

Only outstanding expectations are removed by remove_expectations().
The fullfilled ones need the reference for stuff like
ip_nat_follow_master.

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Harald Welte]

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

On Fri, Aug 05, 2005 at 12:43:04PM +0200, Patrick McHardy wrote:
> Pablo Neira wrote:
> > This fixes the problem although I think that there's something wrong
> > with current refcounting. Every time an expectation is created, the
> > refcount of the master conntrack is incremented. Then, if the master
> > conntrack is destroyed, say the timeout has expired,
> > ip_ct_remove_expectation will be called. So firstly, all expectations
> > linked to such conntrack are destroyed and then the conntrack itself.
> > OK, my question is: why do we need to increase the master conntrack
> > refcount for expectations, if they are always killed first? Actually I
> > think that the master conntrack refcount should be increased once the
> > expectation is confirmed but not in ip_conntrack_expect_alloc.
> 
> Only outstanding expectations are removed by remove_expectations().
> The fullfilled ones need the reference for stuff like
> ip_nat_follow_master.

So Pablo's argument stil upholds... in theory it would be sufficient to
increase the master refcount when expectation is fulfilled(confirmed)
instead of allocation time.  Or am I missing something?

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Patrick McHardy]

Harald Welte wrote:
> On Fri, Aug 05, 2005 at 12:43:04PM +0200, Patrick McHardy wrote:
> 
>>Pablo Neira wrote:
>>
>>>OK, my question is: why do we need to increase the master conntrack
>>>refcount for expectations, if they are always killed first? Actually I
>>>think that the master conntrack refcount should be increased once the
>>>expectation is confirmed but not in ip_conntrack_expect_alloc.
>>
>>Only outstanding expectations are removed by remove_expectations().
>>The fullfilled ones need the reference for stuff like
>>ip_nat_follow_master.
> 
> So Pablo's argument stil upholds... in theory it would be sufficient to
> increase the master refcount when expectation is fulfilled(confirmed)
> instead of allocation time.  Or am I missing something?

No, it is possible, but we do take a reference in expect_alloc, and I
don't see the point of obscuring the reference counting by increasing
the count at a later point in time.

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Date: Sat, 06 Aug 2005 16:24:32 +0200

> Harald Welte wrote:
> > On Fri, Aug 05, 2005 at 12:43:04PM +0200, Patrick McHardy wrote:
> > 
> >>Pablo Neira wrote:
> >>
> >>>OK, my question is: why do we need to increase the master conntrack
> >>>refcount for expectations, if they are always killed first? Actually I
> >>>think that the master conntrack refcount should be increased once the
> >>>expectation is confirmed but not in ip_conntrack_expect_alloc.
> >>
> >>Only outstanding expectations are removed by remove_expectations().
> >>The fullfilled ones need the reference for stuff like
> >>ip_nat_follow_master.
> > 
> > So Pablo's argument stil upholds... in theory it would be sufficient to
> > increase the master refcount when expectation is fulfilled(confirmed)
> > instead of allocation time.  Or am I missing something?
> 
> No, it is possible, but we do take a reference in expect_alloc, and I
> don't see the point of obscuring the reference counting by increasing
> the count at a later point in time.

This refcounts handling has other issue of deadlock.
ip_conntrack_expect_put() is called while holding nf_conntrack_lock
by ip_conntrack_expect_related() and ip_conntrack_helper_unregister().
In logical, this may result in deadlock in destroy_conntrack() destroying
master of expectation.

I cannot find the case that refcounts of conntrack become 0 by
ip_conntrack_expect_put() because expectations seems to be destroyed
earlier than conntrack. But Pablo and Harald idea will solve this issue if
deadlock is really possible.

-----------------------------------------------------------------
Yasuyuki Kozakai @ USAGI Project <yasuyuki.kozakai@toshiba.co.jp>

Re: [PATCH] Drop expectation refcount after unlinking expectation

[Harald Welte]

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

On Fri, Aug 05, 2005 at 02:38:31AM +0200, Pablo Neira wrote:
> This patch comes from the following thread:
> 
> [PATCH 6/7] Fix expectation creation
> 
> In unlink_expect, the expectation is removed from the list so the
> refcount must be dropped as well.

I'm applying this to netfilter-2.6.14 right now.

Any cleanups (as Patrick suggested) can follow later.
-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]