Re: Fw: [Bug 13339] New: rtable leak in ipv4/route.c

[Jarek Poplawski <jarkao2@gmail.com>]

On Tue, May 19, 2009 at 01:45:04PM -0400, Neil Horman wrote:
> On Tue, May 19, 2009 at 07:17:03PM +0200, Jarek Poplawski wrote:
...
> > > diff --git a/include/net/dst.h b/include/net/dst.h
> > > index 6be3b08..a39db6d 100644
> > > --- a/include/net/dst.h
> > > +++ b/include/net/dst.h
> > > @@ -47,6 +47,7 @@ struct dst_entry
> > >  #define DST_NOXFRM		2
> > >  #define DST_NOPOLICY		4
> > >  #define DST_NOHASH		8
> > > +#define DST_GRPLDR		16
> > >  	unsigned long		expires;
> > >  
> > >  	unsigned short		header_len;	/* more space at head required */
> > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > > index c4c60e9..0120f0e 100644
> > > --- a/net/ipv4/route.c
> > > +++ b/net/ipv4/route.c
> > > @@ -610,6 +610,8 @@ static inline int ip_rt_proc_init(void)
> > >  
> > >  static inline void rt_free(struct rtable *rt)
> > >  {
> > > +	if (rt->u.dst.flags & DST_GRPLDR)
> > > +		rt->u.dst.rt_next->u.dst.flag |= DST_GRPLDR;
> > >  	call_rcu_bh(&rt->u.dst.rcu_head, dst_rcu_free);
> > >  }
> > >  
> > > @@ -1143,8 +1145,11 @@ restart:
> > >  		 * relvant to the hash function together, which we use to adjust
> > >  		 * our chain length
> > >  		 */
> > > -		if (*rthp && compare_hash_inputs(&(*rthp)->fl, &rt->fl))
> > > +		if (!*rthi && *rthp && 
> > > +		    compare_hash_inputs(&(*rthp)->fl, &rt->fl) &&
> > > +		    (cand != rth))
> > >  			rthi = rth;
> > 
> > Does it really prevent cand == rthi in the next loop?
> > 
> Yes, because cand and rthi are inspected during the same loop iteration, and
> both assigned from rth.  since I added a check which requires !rthi (which is
> actually a bug above that I need to fix), once rthi is set, it won't be moved,
> and on the next iteration, if cand is assigned, it is assigned to rth, which
> (being the next iteration), is a different rt cache entry
> 
> 
> > I didn't check Eric's patch yet, but I really don't know what's wrong
> > with something as simple as below for -stable, until "proper" fix is
> > analyzed and tested.
> > 
> Because the above fixes it without continuing to break the ordering.  You're
> change below prevents the leak, but still allows for disordered lists to form,
> which IMHO doesn't really make it a candidate for -stable.  I'd much rather fix
> both the leak and the ordering before pushing anything out
> 
> speaking of which, I'm going to ask again, I've been looking all morning, and
> I'm unable to find the move to front heuristic that you mentioned creates furthe
> list disordering.  If you can point it out to me, I can complete my patch and
> present something for you to test more throughly.

As I've written already, your patch looks OK to me.

Thanks,
Jarek P.