From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH v3] ipv6: Fix problem with expired dst cache Date: Wed, 29 Feb 2012 04:14:19 -0800 Message-ID: <1330517659.2610.31.camel@edumazet-laptop> References: <4F4DEF2F.7010109@cn.fujitsu.com> <1330510045-23618-1-git-send-email-gaofeng@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: davem@davemloft.net, netdev@vger.kernel.org To: Gao feng Return-path: Received: from mail-pw0-f46.google.com ([209.85.160.46]:60960 "EHLO mail-pw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756270Ab2B2MOY (ORCPT ); Wed, 29 Feb 2012 07:14:24 -0500 Received: by mail-pw0-f46.google.com with SMTP id up15so4526624pbc.19 for ; Wed, 29 Feb 2012 04:14:24 -0800 (PST) In-Reply-To: <1330510045-23618-1-git-send-email-gaofeng@cn.fujitsu.com> Sender: netdev-owner@vger.kernel.org List-ID: Le mercredi 29 f=C3=A9vrier 2012 =C3=A0 18:07 +0800, Gao feng a =C3=A9c= rit : > If the ipv6 dst cache which copy from the dst generated by ICMPV6 RA = packet. > this dst cache will not check expire because it has no RTF_EXPIRES fl= ag. > So this dst cache will always be used until the dst gc run. >=20 > Change the struct dst_entry,add a union contains new pointer from and= expires. > When rt6_info.rt6i_flags has no RTF_EXPIRES flag,the dst.expires has = no use. > we can use this field to point to where the dst cache copy from. > The dst.from is only used in IPV6. >=20 > In func rt6_check_expired check if rt6_info.dst.from is expired. >=20 > In func ip6_rt_copy only set dst.from when the ort has flag RTF_ADDRC= ONF > and RTF_DEFAULT.then hold the ort. >=20 > In func ip6_dst_destroy release the ort. >=20 > Add some functions to operate the RTF_EXPIRES flag and expires(from) > and change the code to use these new adding functions. >=20 > Signed-off-by: Gao feng > --- > include/net/dst.h | 11 ++++++++++- > include/net/ip6_fib.h | 41 +++++++++++++++++++++++++++++++++++++++= ++ > net/ipv6/addrconf.c | 9 +++------ > net/ipv6/ip6_fib.c | 3 +-- > net/ipv6/route.c | 49 +++++++++++++++++++++++++++++++--------= ---------- > 5 files changed, 86 insertions(+), 27 deletions(-) >=20 > diff --git a/include/net/dst.h b/include/net/dst.h > index 344c8dd..5147839 100644 > --- a/include/net/dst.h > +++ b/include/net/dst.h > @@ -35,7 +35,16 @@ struct dst_entry { > struct net_device *dev; > struct dst_ops *ops; > unsigned long _metrics; > - unsigned long expires; > + > + union { > + unsigned long expires; > + /* > + * from is used only for dst cache witch copy form > + * the dst generated by ipv6 RA. > + * from is set only when rt6_info has no RTF_EXPIRES flag. I am not an english native but really this comment should be reworded..= =2E > + */ > + void *from; > + }; > struct dst_entry *path; > struct neighbour __rcu *_neighbour; > #ifdef CONFIG_XFRM > diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h > index b26bb81..86cf1ac 100644 > --- a/include/net/ip6_fib.h > +++ b/include/net/ip6_fib.h > @@ -123,6 +123,47 @@ static inline struct inet6_dev *ip6_dst_idev(str= uct dst_entry *dst) > return ((struct rt6_info *)dst)->rt6i_idev; > } > =20 > +static inline void rt6_clean_expires(struct rt6_info *rt) > +{ > + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) > + dst_release(&rt->dst); > + > + rt->rt6i_flags &=3D ~RTF_EXPIRES; > + rt->dst.expires =3D 0; > +} > + > +static inline void rt6_set_expires(struct rt6_info *rt, unsigned lon= g expires) > +{ > + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) > + dst_release(&rt->dst); > + > + rt->rt6i_flags |=3D RTF_EXPIRES; > + rt->dst.expires =3D expires; > +} > + > +static inline void rt6_update_expires(struct rt6_info *rt, int timeo= ut) > +{ > + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) > + dst_release(&rt->dst); > + > + dst_set_expires(&rt->dst, timeout); > + rt->rt6i_flags |=3D RTF_EXPIRES; > +} why rt6_update_expires() takes an "int timeout", promoted to "unsigned long expires" ? Do you have a 32bit machine by any chance ? Why is it needed at all, it seems rt6_update_expires() is redundant wit= h dst_set_expires() > + > +static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info= *from) > +{ > + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) { > + if (from =3D=3D rt->dst.from) > + return; after a "return;" you dont need an "else"=20 > + else > + dst_release((struct dst_entry *) &rt->dst.from); Really this cast hides a real bug... Was this patch tested ? > + } > + > + rt->rt6i_flags &=3D ~RTF_EXPIRES; > + rt->dst.from =3D (void *) from; > + dst_hold(&from->dst); You hold a reference on the "from" dst, which is fine, but some previou= s releases are done on dst_release(&rt->dst). So you dont release the right dst and bad things happen. I am not really convinced by this patch, too many issues in it. Please take the time to make sure you submit a nice one on your next submission. This part of the code is complex and need top quality patches.