Re: [PATCH v3] ipv6: Fix problem with expired dst cache
From: Eric Dumazet <hidden>
Date: 2012-02-29 12:14:24
Le mercredi 29 février 2012 à 18:07 +0800, Gao feng a écrit :
quoted hunk ↗ jump to hunk
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 flag. So this dst cache will always be used until the dst gc run. 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. In func rt6_check_expired check if rt6_info.dst.from is expired. In func ip6_rt_copy only set dst.from when the ort has flag RTF_ADDRCONF and RTF_DEFAULT.then hold the ort. In func ip6_dst_destroy release the ort. Add some functions to operate the RTF_EXPIRES flag and expires(from) and change the code to use these new adding functions. Signed-off-by: Gao feng <redacted> --- 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(-)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...
quoted hunk ↗ jump to hunk
+ */ + void *from; + }; struct dst_entry *path; struct neighbour __rcu *_neighbour; #ifdef CONFIG_XFRMdiff --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(struct dst_entry *dst) return ((struct rt6_info *)dst)->rt6i_idev; } +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 &= ~RTF_EXPIRES; + rt->dst.expires = 0; +} + +static inline void rt6_set_expires(struct rt6_info *rt, unsigned long expires) +{ + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) + dst_release(&rt->dst); + + rt->rt6i_flags |= RTF_EXPIRES; + rt->dst.expires = expires; +} + +static inline void rt6_update_expires(struct rt6_info *rt, int timeout) +{ + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.from) + dst_release(&rt->dst); + + dst_set_expires(&rt->dst, timeout); + rt->rt6i_flags |= 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 with 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 == rt->dst.from)
+ return;after a "return;" you dont need an "else"
+ else + dst_release((struct dst_entry *) &rt->dst.from);
Really this cast hides a real bug... Was this patch tested ?
+ } + + rt->rt6i_flags &= ~RTF_EXPIRES; + rt->dst.from = (void *) from; + dst_hold(&from->dst);
You hold a reference on the "from" dst, which is fine, but some previous 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.