Re: [PATCH] netfilter: nf_conntrack: don't release a conntrack with non-zero refcnt

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Feb 04, 2014 at 12:29:25AM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > > index 4d1fb5d..356bef5 100644
> > > --- a/net/netfilter/nf_conntrack_core.c
> > > +++ b/net/netfilter/nf_conntrack_core.c
> > > @@ -448,7 +448,9 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
> > >  			goto out;
> > >  
> > >  	add_timer(&ct->timeout);
> > > -	nf_conntrack_get(&ct->ct_general);
> > > +	smp_wmb();
> > > +	/* The caller holds a reference to this object */
> > > +	atomic_set(&ct->ct_general.use, 2);
> > 
> > What happens when the skb is dropped before confirmation?
> 
> If you refer to the spot above, conntracks added via ctnetlink are
> always confirmed and the refcount will become 1 after insertion into
> the hashes. In the packet path, the refcount is 1 for unconfirmed
> conntracks,

Right, sorry.  I missed this.  You only want to delay the init-to-1
until the conntrack is inserted into the unconfirmed list.

However, I think there is still a problem with the atomic_set to 2
above.

What about e.g.
Prerouting -> set refcnt to 1
(clone)
(clone)
(refcnt is 3)
forward/postrouting -> confirmation (refcnt reset to two)

I think this could happen at least with netfilter on-top-of bridge
when dealing with e.g. udp mcast. (i am looking at br_forward and
deliver_clone in particular).

I think you could just leave hash_check_insert() alone?