Date:	Wed, 29 Feb 2012 04:14:19 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Gao feng <gaofeng@...fujitsu.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v3] ipv6: Fix problem with expired dst cache

Le mercredi 29 février 2012 à 18:07 +0800, Gao feng a écrit :
> 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 <gaofeng@...fujitsu.com>
> ---
>  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...


> +		 */
> +		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(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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists