Re: [PATCH v3] ipv6: Fix problem with expired dst cache

[Gao feng <gaofeng@cn.fujitsu.com>]

Hi Eric:

于 2012年02月29日 20:14, Eric Dumazet 写道:
> Le mercredi 29 février 2012 à 18:07 +0800, Gao feng a é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
> 
> 
>> +		 * 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...
> 

Sorry...I will reworded it.
> 
>> +		 */
>> +		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 ?

Because dst_set_expires takes an "int timeout".
rt6_update_expires provides an interface to change rt->rt6i_flags and rt->dst.expires together.
Just like rt6_clean_expires,rt6_set_expires...

> 
> 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 ?
> 

I am very sorry for this,it's my fault.

>> +	}
>> +
>> +	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.
> 


Thanks Eric,I will change this patch carefully,and resend it after test.