Re: [PATCH 2/2] netfilter: conntrack: remove timer from ecache extension

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I already told you, your patchset works in my testbed here.
> 
> My only doubt still here is the need for the extra bit. I don't find
> the scenario that will trigger the problem yet. Some comments:

Its not obvious but you should be able to produce crashes in your
tests when you only look for dying-bit-not-set, however...

> > OR it can mean
> > 
> > b) 'This conntrack is being allocated/setup as new connection, the
> > flag field was already cleared'.
> 
> In this case, the conntrack is placed in the unconfirmed list or the
> hashes.

Not necessarily.
conntracks are placed on the dying list, when refcnt reaches zero
they're deleted from the dying list.  But we could have picked it up
right before.

I *think* you're right in that we could avoid the extra bit if we hold
the dying list spinlock.  Then, if we pick it up before removal, dying
bit is always set since no recycling can happen underneath.

> either a) release the entry whose event was already delivered or b)
> decrement the use counter.
> 
> > I've found no way to tell these two conditions apart except via new bit.
> 
> I believe the rule: "all dead conntracks have the dying bit set"
> always fulfills.

Looking at it again I think I need to rework the patch to use the appropriate
lock during traversal instead of rcu, as the dying list manipulation
routines don't use the _rcu versions for hlist manipulation anyway...

I'll send a v3 soon, after doing short sanity test.

Since 3.15 has been released already we should simply aim this patch for
3.17. No harm done.

Thanks for reviewing and testing!