double call to ip_conntrack_put() ?

double call to ip_conntrack_put() ?

[Wang Jian]

Hi,

death_by_timeout() calls ip_conntrack_put() before return. And
death_by_timeout() is called combined with ip_conntrack_put() in some
places, such as

in early_drop()

        if (del_timer(&ct->timeout)) {
                death_by_timeout((unsigned long)ct);
                dropped = 1;
                CONNTRACK_STAT_INC(early_drop);
        }
        ip_conntrack_put(ct);

and in ip_ct_iterate_cleanup()

        while ((h = get_next_corpse(iter, data, &bucket)) != NULL) {
                struct ip_conntrack *ct = tuplehash_to_ctrack(h);
                /* Time to push up daises... */
                if (del_timer(&ct->timeout))
                        death_by_timeout((unsigned long)ct);
                /* ... else the timer will get him soon. */

                ip_conntrack_put(ct);
        }

Is this intended or misuse?


-- 
  lark

Re: double call to ip_conntrack_put() ?

[KOVACS Krisztian]

  Hi,

2005-04-18, h keltezéssel 17.29-kor Wang Jian ezt írta:
> death_by_timeout() calls ip_conntrack_put() before return. And
> death_by_timeout() is called combined with ip_conntrack_put() in some
> places, such as
> 
> in early_drop()
> 
>         if (del_timer(&ct->timeout)) {
>                 death_by_timeout((unsigned long)ct);
>                 dropped = 1;
>                 CONNTRACK_STAT_INC(early_drop);
>         }
>         ip_conntrack_put(ct);

  This isn't a bug. The function gets a reference to the conntrack entry
before releasing the read lock of the hash table, so it must release the
entry after deleting it from the hash.

> and in ip_ct_iterate_cleanup()
> 
>         while ((h = get_next_corpse(iter, data, &bucket)) != NULL) {
>                 struct ip_conntrack *ct = tuplehash_to_ctrack(h);
>                 /* Time to push up daises... */
>                 if (del_timer(&ct->timeout))
>                         death_by_timeout((unsigned long)ct);
>                 /* ... else the timer will get him soon. */
> 
>                 ip_conntrack_put(ct);
>         }

  Same here. get_next_corpse() increases the refcount of the returned
conntrack, so the caller has to dereference it.

-- 
KOVACS Krisztian <hidden@sch.bme.hu>

Re: double call to ip_conntrack_put() ?

[Tobias DiPasquale]

On 4/18/05, Wang Jian <lark@linux.net.cn> wrote:
> death_by_timeout() calls ip_conntrack_put() before return. And
> death_by_timeout() is called combined with ip_conntrack_put() in some
> places, such as
> 
> in early_drop()
> 
>         if (del_timer(&ct->timeout)) {
>                 death_by_timeout((unsigned long)ct);
>                 dropped = 1;
>                 CONNTRACK_STAT_INC(early_drop);
>         }
>         ip_conntrack_put(ct);
> 
> and in ip_ct_iterate_cleanup()
> 
>         while ((h = get_next_corpse(iter, data, &bucket)) != NULL) {
>                 struct ip_conntrack *ct = tuplehash_to_ctrack(h);
>                 /* Time to push up daises... */
>                 if (del_timer(&ct->timeout))
>                         death_by_timeout((unsigned long)ct);
>                 /* ... else the timer will get him soon. */
> 
>                 ip_conntrack_put(ct);
>         }
> 
> Is this intended or misuse?

This is intended. tuplehash_to_ctrack() generates a reference to the
conntrack record. In death_by_timeout() the refcount is decremented,
yielding 1 for the second call to ip_conntrack_put(). The dec_and_test
in nf_conntrack_put() (called by ip_conntrack_put()) will result in a
refcount of 0, sending the record to the destroy_conntrack() function
(or more properly, whatever's registered in nfct->destroy(), which I'm
pretty sure is always either NULL or destroy_conntrack()).

-- 
[ Tobias DiPasquale ]
0x636f6465736c696e67657240676d61696c2e636f6d

Re: double call to ip_conntrack_put() ?

[Wang Jian]

Hi Tobias DiPasquale,


On Mon, 18 Apr 2005 05:57:30 -0400, Tobias DiPasquale <codeslinger@gmail.com> wrote:

> 
> This is intended. tuplehash_to_ctrack() generates a reference to the
> conntrack record. In death_by_timeout() the refcount is decremented,
> yielding 1 for the second call to ip_conntrack_put(). The dec_and_test
> in nf_conntrack_put() (called by ip_conntrack_put()) will result in a
> refcount of 0, sending the record to the destroy_conntrack() function
> (or more properly, whatever's registered in nfct->destroy(), which I'm
> pretty sure is always either NULL or destroy_conntrack()).
> 

Thanks for your clear answer :)

-- 
  lark

Re: double call to ip_conntrack_put() ?

[Amin Azez]

Tobias DiPasquale wrote:
> This is intended. tuplehash_to_ctrack() generates a reference to the
> conntrack record. In death_by_timeout() the refcount is decremented,
> yielding 1 for the second call to ip_conntrack_put(). The dec_and_test
> in nf_conntrack_put() (called by ip_conntrack_put()) will result in a
> refcount of 0, sending the record to the destroy_conntrack() function
> (or more properly, whatever's registered in nfct->destroy(), which I'm
> pretty sure is always either NULL or destroy_conntrack()).

What I found confusing initially was that "put" in "ip_conntrack_put" 
means "release" or "refcount--" instead of "write" or "store" which 
would have been my first guess.

Sam Azez

Re: double call to ip_conntrack_put() ?

[Tobias DiPasquale]

On 4/18/05, Amin Azez <azez@ufomechanic.net> wrote:
> What I found confusing initially was that "put" in "ip_conntrack_put"
> means "release" or "refcount--" instead of "write" or "store" which
> would have been my first guess.

In the Linux kernel sources, *_get() usually increments a reference
count on an object, and *_put() decrements it. This is a common naming
convention, although I can see how it could be confusing.

-- 
[ Tobias DiPasquale ]
0x636f6465736c696e67657240676d61696c2e636f6d

Re: double call to ip_conntrack_put() ?

[Wang Jian]

Hi Amin Azez,

Yes, I am also confused by the _put for a while until I read the code.
IMHO, it's more clear to rename it to _release.

On Mon, 18 Apr 2005 12:02:31 +0100, Amin Azez <azez@ufomechanic.net> wrote:

> Tobias DiPasquale wrote:
> > This is intended. tuplehash_to_ctrack() generates a reference to the
> > conntrack record. In death_by_timeout() the refcount is decremented,
> > yielding 1 for the second call to ip_conntrack_put(). The dec_and_test
> > in nf_conntrack_put() (called by ip_conntrack_put()) will result in a
> > refcount of 0, sending the record to the destroy_conntrack() function
> > (or more properly, whatever's registered in nfct->destroy(), which I'm
> > pretty sure is always either NULL or destroy_conntrack()).
> 
> What I found confusing initially was that "put" in "ip_conntrack_put" 
> means "release" or "refcount--" instead of "write" or "store" which 
> would have been my first guess.
> 
> Sam Azez
> 



-- 
  lark