Question about reference count of expectations in 2.6.14 tree

Question about reference count of expectations in 2.6.14 tree

[Yasuyuki KOZAKAI]

Hi,

In 2.6.14 tree, ip_conntrack_expect_insert() increments the reference
count of expectations twice. I understand that once is for timer.
What is next incrementing for ? expectation list ?

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

Question about reference count of expectations in 2.6.14 tree

[Yasuyuki KOZAKAI]

# Well, the mail I sent yesterday seems not to reach list, so I resent it.
# Sorry if you have already received same mail.

Hi,

In 2.6.14 tree, ip_conntrack_expect_insert() increments the reference
count of expectations twice. I understand that once is for timer.
What is next incrementing for ? expectation list ?

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

Re: Question about reference count of expectations in 2.6.14 tree

[Harald Welte]

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

On Wed, Aug 10, 2005 at 10:11:49AM +0900, Yasuyuki KOZAKAI wrote:
> 
> # Well, the mail I sent yesterday seems not to reach list, so I resent it.
> # Sorry if you have already received same mail.

clamav has some problems at the listserver, so the list was stuck for
some time :(

> In 2.6.14 tree, ip_conntrack_expect_insert() increments the reference
> count of expectations twice. I understand that once is for timer.
> What is next incrementing for ? expectation list ?

Good question.  To me it looks like a bug introduced when merging
Rusty's conntrack-refcounting patch and the original ctnetlink code.

If nobody objects, I'll remove the second atomic_inc() line from that
function.

-- 
- 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: Question about reference count of expectations in 2.6.14 tree

[Harald Welte]

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

On Thu, Aug 11, 2005 at 03:35:45PM +0200, Harald Welte wrote:
> On Wed, Aug 10, 2005 at 10:11:49AM +0900, Yasuyuki KOZAKAI wrote:
> > 
> > # Well, the mail I sent yesterday seems not to reach list, so I resent it.
> > # Sorry if you have already received same mail.
> 
> clamav has some problems at the listserver, so the list was stuck for
> some time :(
> 
> > In 2.6.14 tree, ip_conntrack_expect_insert() increments the reference
> > count of expectations twice. I understand that once is for timer.
> > What is next incrementing for ? expectation list ?
> 
> Good question.  To me it looks like a bug introduced when merging
> Rusty's conntrack-refcounting patch and the original ctnetlink code.

Looking at it again, and doing some tests: The current code works,
/proc/slabinfo shows me there is no expectation leak.

This is because the reference count is also put two times:  in
unlink_expect, and in expectation_timed_out().  Almost all over the
place, first unlink_expect() drops the first refcount, and
ip_conntrack_espect_put() is called explicitly to drop the second.

So yes, one count seems to be for the timer, the second one for
ip_conntrack_expect_list.  Two references.

I think it would be possible to remove the second atomic_inc() and all
the ip_conntrack_put() that immediately follow ulink_expect() - but
would it be worth the effort?  Isn't it an over-optimization that is
likely to cause bugs later on?

Patrick? Pablo?

-- 
- 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: Question about reference count of expectations in 2.6.14 tree

[Patrick McHardy]

Harald Welte wrote:
> Looking at it again, and doing some tests: The current code works,
> /proc/slabinfo shows me there is no expectation leak.
> 
> This is because the reference count is also put two times:  in
> unlink_expect, and in expectation_timed_out().  Almost all over the
> place, first unlink_expect() drops the first refcount, and
> ip_conntrack_espect_put() is called explicitly to drop the second.
> 
> So yes, one count seems to be for the timer, the second one for
> ip_conntrack_expect_list.  Two references.

Correct.

> I think it would be possible to remove the second atomic_inc() and all
> the ip_conntrack_put() that immediately follow ulink_expect() - but
> would it be worth the effort?  Isn't it an over-optimization that is
> likely to cause bugs later on?

There is one spot where it isn't possible, in find_expectation
the timer reference isn't dropped but returned to the caller.
Changing it would either require a special version of unlink_expect
or an additional atomic_inc in this function. I don't think its
worth the trouble, we do take two references so this seems the
cleanest way to me.

Re: Question about reference count of expectations in 2.6.14 tree

[Harald Welte]

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

On Thu, Aug 11, 2005 at 04:40:32PM +0200, Patrick McHardy wrote:

> >I think it would be possible to remove the second atomic_inc() and all
> >the ip_conntrack_put() that immediately follow ulink_expect() - but
> >would it be worth the effort?  Isn't it an over-optimization that is
> >likely to cause bugs later on?
> 
> There is one spot where it isn't possible, in find_expectation
> the timer reference isn't dropped but returned to the caller.
> Changing it would either require a special version of unlink_expect
> or an additional atomic_inc in this function. I don't think its
> worth the trouble, we do take two references so this seems the
> cleanest way to me.

I think I'll add some comment to the code explaining why we need to get
two references.  

-- 
- 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: Question about reference count of expectations in 2.6.14 tree

[Pablo Neira]

Harald Welte wrote:
> On Thu, Aug 11, 2005 at 04:40:32PM +0200, Patrick McHardy wrote:
> 
> 
>>>I think it would be possible to remove the second atomic_inc() and all
>>>the ip_conntrack_put() that immediately follow ulink_expect() - but
>>>would it be worth the effort?  Isn't it an over-optimization that is
>>>likely to cause bugs later on?
>>
>>There is one spot where it isn't possible, in find_expectation
>>the timer reference isn't dropped but returned to the caller.
>>Changing it would either require a special version of unlink_expect
>>or an additional atomic_inc in this function. I don't think its
>>worth the trouble, we do take two references so this seems the
>>cleanest way to me.
> 
> I think I'll add some comment to the code explaining why we need to get
> two references.  

Sure, Yasuyuki and I have asked the same thing in a very short time. So 
this probably will help others.

BTW, I'm still willing to post two patches related with this stuff:

a) kill __ip_ct_expect_unlink_destroy. As Patrick pointed out, it's kind 
of confusing. Actually the only user is conntrack_netlink. So we can 
just export unlink_expect and use it calling ip_conntrack_expect_put 
(already exported).

b) don't increase master conntrack refcount for non-confirmed 
conntracks. It could deadlock. If expectations are killed before the 
master conntrack, we call ip_conntrack_put holding ip_conntrack_lock as 
Yasuyuki described. Actually such thing shouldn't ever happen because 
non-fulfilled(confirmed) expectations are always killed first. Hm, but 
it still looks to me a bit ugly. So I'm proposing to increase the master 
conntrack refcount just once the expectation is fulfilled.

--
Pablo

Re: Question about reference count of expectations in 2.6.14 tree

[Yasuyuki KOZAKAI]

Hi,

Thanks for clarification.

From: Harald Welte <laforge@netfilter.org>
Date: Thu, 11 Aug 2005 21:20:50 +0200

> On Thu, Aug 11, 2005 at 04:40:32PM +0200, Patrick McHardy wrote:
> 
> > >I think it would be possible to remove the second atomic_inc() and all
> > >the ip_conntrack_put() that immediately follow ulink_expect() - but
> > >would it be worth the effort?  Isn't it an over-optimization that is
> > >likely to cause bugs later on?

Their counters are for timer, and we cannot remove them. But
ip_conntrack_expect_put() 'in' unlink_expect() is for expectation list,
I think we can remove is only it.

> > There is one spot where it isn't possible, in find_expectation
> > the timer reference isn't dropped but returned to the caller.
> > Changing it would either require a special version of unlink_expect
> > or an additional atomic_inc in this function. I don't think its
> > worth the trouble, we do take two references so this seems the
> > cleanest way to me.

Well, I'm afraid of possibility of deadlock because unlink_expect() may
cause to call nf_conntrack_put() while holding ip_conntrack_lock,
in logical. Then I want to remove ip_conntrack_expect_put() in unlink_expect().

Of cause we need to take care of the order of unlink_expect() and
ip_conntrack_expect_put(), to avoid destroying expectation in list.

> I think I'll add some comment to the code explaining why we need to get
> two references.  

If we can avoid deadlock without removing ip_conntrack_put(), I think this is
the cleanet way.

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