Re: [PATCH 1/2] netfilter: nf_conntrack: improve nf_conn object traceability

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Dec 03, 2012 at 03:17:22PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > i.o.w., conntrack objects that were in hash table are now always moved
> > > > to the dying list.  Shouldn't nf_ct_release_dying_list() be adjusted,
> > > > too?  It still seems to assume that the dying list only contains
> > > > alive conntack objects whose events have not been delivered yet.
> > > 
> > > I see, you mean that nf_ct_release_dying_list should delete objects
> > > from the dying list. Yes, I'll fix that. Thanks for the spot.
> > 
> > destroy_conntrack will delete the object from the dying list, so I
> > think the code is fine.
> > 
> > Am I missing anything?
> 
> Nope, you're right.
> 
> I was wondering what happens when nf_ct_release_dying_list()
> nf_ct_kill()s entries which are currently going through destroy_conntrack()
> on another cpu (i was concerned that we're putting an entry that
> already has 0-refcnt).  However, since nf_ct_kill calls del_timer,
> it should just return without doing anything for those conntracks.
> 
> I think nf_ct_release_dying_list can be removed:
> 1 For those entries that are about to go through destroy_conntrack()
>   nothing happens (since ct->timeout is no longer pending)
> 2. For those entries that are waiting for event-redelivery,
>   nothing happens either :-) [ for the same reason -- ct->timeout
>   is not pending anymore ].
> 
> Those objects that sit on the dying list because they wait for event
> re-delivery, will expire normally via ecache->timeout:
> 
> nf_conntrack_cleanup_net() will schedule() until all ecache->timeout
> timers have fired.  This shouldn't take too long, and no re-arming
> of the ecache timer will happen since no listeners exist at that point.
> 
> Does that sound right, or am I missing anything?

Sounds right to me. The removal path of the module should call
ct->timeout.function and remove the ecache timer. I think this is a
different issue, I'll send a follow-up patch to address this.