From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gao feng Subject: Re: [PATCH v3] ipv6: Fix problem with expired dst cache Date: Thu, 01 Mar 2012 08:43:20 +0800 Message-ID: <4F4EC628.1030402@cn.fujitsu.com> References: <4F4DEF2F.7010109@cn.fujitsu.com> <1330510045-23618-1-git-send-email-gaofeng@cn.fujitsu.com> <1330517659.2610.31.camel@edumazet-laptop> 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: Eric Dumazet Return-path: Received: from cn.fujitsu.com ([222.73.24.84]:61307 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S932094Ab2CAAm0 convert rfc822-to-8bit (ORCPT ); Wed, 29 Feb 2012 19:42:26 -0500 In-Reply-To: <1330517659.2610.31.camel@edumazet-laptop> Sender: netdev-owner@vger.kernel.org List-ID: Hi Eric: =E4=BA=8E 2012=E5=B9=B402=E6=9C=8829=E6=97=A5 20:14, Eric Dumazet =E5=86= =99=E9=81=93: > Le mercredi 29 f=C3=A9vrier 2012 =C3=A0 18:07 +0800, Gao feng a =C3=A9= crit : >> 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 >=20 >=20 >> + * the dst generated by ipv6 RA. >> + * from is set only when rt6_info has no RTF_EXPIRES flag. >=20 >=20 > I am not an english native but really this comment should be reworded= =2E.. >=20 Sorry...I will reworded it. >=20 >> + */ >> + 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(st= ruct 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 lo= ng 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 time= out) >> +{ >> + 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; >> +} >=20 > why rt6_update_expires() takes an "int timeout", promoted to "unsigne= d > long expires" ? Do you have a 32bit machine by any chance ? Because dst_set_expires takes an "int timeout". rt6_update_expires provides an interface to change rt->rt6i_flags and r= t->dst.expires together. Just like rt6_clean_expires,rt6_set_expires... >=20 > Why is it needed at all, it seems rt6_update_expires() is redundant w= ith > dst_set_expires() >=20 >> + >> +static inline void rt6_set_from(struct rt6_info *rt, struct rt6_inf= o *from) >> +{ >> + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) { >> + if (from =3D=3D rt->dst.from) >> + return; >=20 > after a "return;" you dont need an "else"=20 >=20 >> + else >> + dst_release((struct dst_entry *) &rt->dst.from); >=20 > Really this cast hides a real bug... Was this patch tested ? >=20 I am very sorry for this,it's my fault. >> + } >> + >> + rt->rt6i_flags &=3D ~RTF_EXPIRES; >> + rt->dst.from =3D (void *) from; >> + dst_hold(&from->dst); >=20 > You hold a reference on the "from" dst, which is fine, but some previ= ous > releases are done on dst_release(&rt->dst). So you dont release the > right dst and bad things happen. >=20 > I am not really convinced by this patch, too many issues in it. >=20 > 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. >=20 Thanks Eric,I will change this patch carefully,and resend it after test= =2E