Re: [nf-next PATCH 3/5] netfilter: avoid race with exp->master ct

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Thu, 27 Feb 2014 22:34:52 +0100
Florian Westphal <fw@strlen.de> wrote:

> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> > Preparation for disconnecting the nf_conntrack_lock from the
> > expectations code.  Once the nf_conntrack_lock is lifted, a race
> > condition is exposed.
> > 
> > The expectations master conntrack exp->master, can race with
> > delete operations, as the refcnt increment happens too late in
> > init_conntrack().  Race is against other CPUs invoking
> > ->destroy() (destroy_conntrack()), or nf_ct_delete() (via timeout
> > or early_drop()).
> > 
> > Avoid this race in nf_ct_find_expectation() by using atomic_inc_not_zero(),
> > and checking if nf_ct_is_dying() (path via nf_ct_delete()).
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> > 
> >  net/netfilter/nf_conntrack_core.c   |    2 +-
> >  net/netfilter/nf_conntrack_expect.c |   16 +++++++++++++++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index ac85fd1..a822720 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -898,6 +898,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
> >  			 ct, exp);
> >  		/* Welcome, Mr. Bond.  We've been expecting you... */
> >  		__set_bit(IPS_EXPECTED_BIT, &ct->status);
> > +		/* exp->master safe, refcnt bumped in nf_ct_find_expectation */
> >  		ct->master = exp->master;
> >  		if (exp->helper) {
> >  			help = nf_ct_helper_ext_add(ct, exp->helper,
> > @@ -912,7 +913,6 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
> >  #ifdef CONFIG_NF_CONNTRACK_SECMARK
> >  		ct->secmark = exp->master->secmark;
> >  #endif
> > -		nf_conntrack_get(&ct->master->ct_general);
> >  		NF_CT_STAT_INC(net, expect_new);
> >  	} else {
> >  		__nf_ct_try_assign_helper(ct, tmpl, GFP_ATOMIC);
> > diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
> > index 4fd1ca9..2c4ffdb 100644
> > --- a/net/netfilter/nf_conntrack_expect.c
> > +++ b/net/netfilter/nf_conntrack_expect.c
> > @@ -147,13 +147,27 @@ nf_ct_find_expectation(struct net *net, u16 zone,
> >  	if (!exp)
> >  		return NULL;
> >  
> > +	/* Avoid race with other CPUs, that for exp->master ct, is
> > +	 * about to invoke ->destroy(), or nf_ct_delete() via timeout
> > +	 * or early_drop().
> > +	 *
> > +	 * The atomic_inc_not_zero() check tells:  If that fails, we
> > +	 * know that the ct is being destroyed.  If it succeeds, we
> > +	 * can be sure the ct cannot disappear underneath.
> > +	 */
> > +	if (unlikely(nf_ct_is_dying(exp->master) ||
> > +		     !atomic_inc_not_zero(&exp->master->ct_general.use)))
> > +		return NULL;
> > +
> >  	/* If master is not in hash table yet (ie. packet hasn't left
> >  	   this machine yet), how can other end know about expected?
> >  	   Hence these are not the droids you are looking for (if
> >  	   master ct never got confirmed, we'd hold a reference to it
> >  	   and weird things would happen to future packets). */
> > -	if (!nf_ct_is_confirmed(exp->master))
> > +	if (!nf_ct_is_confirmed(exp->master)) {
> > +		atomic_dec(&exp->master->ct_general.use);
> >  		return NULL;
> > +	}
> 
> Not sure if this is safe.
> 
> What about:
> CPU0: atomic_inc_not_zero()
> CPU1: calls nf_conntrack_put()
> CPU0: atomic_dec() -> zero refcnt without invocation of ->destroy

Okay, so, you are saying CPU0 should use nf_ct_put() or nf_conntrack_put().

> [ Cannot happen now because of nf_conntrack_lock ]
> 
> I'd suggest to test nf_ct_is_confirmed() first, it avoids the need to
> undo the atomic_inc_not_zero.

Okay, guess that should be okay.

> You also need to deal with the "timer-deletion-fails" a bit later in the same
> function:
> 
>         if (exp->flags & NF_CT_EXPECT_PERMANENT) {
>                 atomic_inc(&exp->use);
>                 return exp;
>         } else if (del_timer(&exp->timeout)) {
>                 nf_ct_unlink_expect(exp);
>                 return exp;
>         }
> 	// Problem: exp->master ref was bumped
> 	nf_ct_put(exp->master); // missing
>         return NULL;

True, and yes, use of use nf_ct_put() or nf_conntrack_put() would be
necessary here instead of manual refcnt dec.

Thanks for your review, I will fix it up...

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer