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

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 10, 2014 at 11:12:56PM +0200, Florian Westphal wrote:
> > This brings the (per-conntrack) ecache extension back to 24 bytes in size
> > (was 152 byte on x86_64 with lockdep on).
> > 
> > When event delivery fails, re-delivery is attempted via work queue.
> > 
> > Redelivery is attempted at least every 0.1 seconds, but can happen
> > more frequently if userspace is not congested.
> > 
> > The nf_ct_release_dying_list() function is removed.
> > With this patch, ownership of the to-be-redelivered conntracks
> > (on-dying-list-with-DYING-bit not yet set) is with the work queue,
> > which will release the references once event is out.
> 
> I think we need to keep the nf_ct_release_dying_list(), otherwise we
> will hit problems when destroying the kmem_cache, since the workqueue
> may race with that, right?

Are you sure?

nf_ct_release_dying_list() looks broken to me, it _puts() entries
it doesn't own (i.e. refcnt underflows when real owner _puts() entry).

In any case, nf_conntrack_cleanup_net_list() loops until
net->ct.count is zero.  That should be enough, no?

If there are lot of entries with undelivered events the wait period should
not be big; Since there are no listeners at this point the worker will be
rescheduled for immediate reexecuition until it has put() all conntracks
without DYING set.

nf_conntrack_ecache_pernet_fini() (which stops the work queue) is called
after ct.count is 0 and before kmem_cache_destroy().  Looks good to me,
did I miss something?

Thanks,
Florian