Re: [PATCH 4/8][CTNETLINK] Fix race condition on conntrack creation

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

Hi Yasuyuki,

Yasuyuki KOZAKAI wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 25 Jul 2006 15:18:38 +0200
> 
> 
>> - rework get_features facility to avoid a softlockup
> 
> 
> This looks nice cleanup, but __nf_conntrack_alloc() cannot
> be called while holding nf_conntrack_lock, because it may call
> early_drop(), which holds nf_contrack_lock and also may call
> nf_ct_put().
> 
> 
>> static struct nf_conn *
>> __nf_conntrack_alloc(const struct nf_conntrack_tuple *orig,
>> 		     const struct nf_conntrack_tuple *repl,
>>-		     const struct nf_conntrack_l3proto *l3proto)
>>+		     const struct nf_conntrack_l3proto *l3proto,
>>+		     u_int32_t features)
>> {
> 
> 
> You've moved "features = l3proto->get_features(orig);" out of
> this function, then the argument 'l3proto' isn't necessary.

Indeed, I also detected another problem related with the NAT code in
ip_conntrack_netlink, so this patch needs to be dropped.

I'm questioning the usefulness of this patch since nfnetlink
serializes the creation of two new conntracks.

> BTW, I think this race is similar situation in init_conntrack().
> It doesn't care about race and __nf_conntrack_confirm() does it
> instead.
> 
> One more my concern is recent Patrick's proposal.
> 
> https://lists.netfilter.org/pipermail/netfilter-devel/2006-July/025107.html
> 
> 
>>- change conntrack to always put connections in the hash immediately
>>  and remove them again if the connection is dropped before beeing
>>  confirmed.
> 
> If we do this, we need to solve the same issue in init_conntrack().
> It might be time to consider to organize processing of hash and lock.

About the implementation, we can play around with refcounting, set 
refcount to 1 if the conntrack is unconfirmed and increase it to 2 once 
it gets confirmed.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris