Re: [PATCH 1/1] netfilter: fix soft lockup when netlink adds new entries

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

On Fri, Feb 24, 2012 at 09:06:37AM +0100, Jozsef Kadlecsik wrote:
> Hi Pablo,
> 
> On Fri, 24 Feb 2012, Pablo Neira Ayuso wrote:
> 
> > On Thu, Feb 23, 2012 at 09:44:21PM +0100, Jozsef Kadlecsik wrote:
> > > OK, here it comes:
> > > 
> > > The previous patch with the title "netfilter: fix soft lockup
> > > when netlink adds new entries" introduced a race: conntrack and
> > > ctnetlink could insert the same entry twice into the hash table.
> > > The patch eliminates the race condition by using the same checking
> > > as conntrack confirm does.
> > > 
> > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > > ---
> > >  include/net/netfilter/nf_conntrack.h |    2 +
> > >  net/netfilter/nf_conntrack_core.c    |   41 ++++++++++++++++++++++++++++++++++
> > >  net/netfilter/nf_conntrack_netlink.c |    9 +++----
> > >  3 files changed, 47 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> > > index 8a2b0ae..48bfe75 100644
> > > --- a/include/net/netfilter/nf_conntrack.h
> > > +++ b/include/net/netfilter/nf_conntrack.h
> > > @@ -210,6 +210,8 @@ __nf_conntrack_find(struct net *net, u16 zone,
> > >  		    const struct nf_conntrack_tuple *tuple);
> > >  
> > >  extern void nf_conntrack_hash_insert(struct nf_conn *ct);
> > > +extern int nf_conntrack_hash_check_insert(struct net *net, u16 zone,
> > > +					  struct nf_conn *ct);
> > 
> > nf_conntrack_hash_insert has no clients anymore after this change.
> > 
> > We have two choices here:
> > 
> > 1) Expand nf_conntrack_hash_insert to perform the checking inside. You can
> > use the same prototype btw, net and zone can be extracted from the ct.
> 
> It's a void function and we need a result: if the insert fails the
> newly allocated entry must be destroyed and error reported to the user.
>
> > 2) For better code readability (and we save one exported symbol), add
> > the code that you propose for nf_conntrack_hash_check_insert to
> > nf_conntrack_netlink.c. I don't think we would ever have another client
> > of nf_conntrack_hash_insert.
> 
> That'd need exporting hash_conntrack and __nf_conntrack_hash_insert from
> nf_conntrack_core.c. That was the reason why I instead introduced a new
> function.

I see. Go ahead then.

Thanks Jozsef.