Re: [RFC PATCH net-next] ip6: Do not expire uncached routes for mtu invalidation

[Alex Gartrell <agartrell@fb.com>]

Thank you for taking a look, Eric.

I'll admit that I have a distinct lack of confidence that I've got the 
right solution to the problem here, but I've made it about as far as I 
can without getting your collective comments, so it's much appreciated.

On 9/8/14 3:30 AM, Eric Dumazet wrote:
> On Mon, 2014-09-08 at 01:34 -0700, Alex Gartrell wrote:
>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>> index 9bcb220..2f0d4d0 100644
>> --- a/include/net/ip6_fib.h
>> +++ b/include/net/ip6_fib.h
>> @@ -184,7 +184,8 @@ static inline void rt6_update_expires(struct rt6_info *rt0, int timeout)
>>   		rt0->dst.expires = rt->dst.expires;
>>
>>   	dst_set_expires(&rt0->dst, timeout);
>> -	rt0->rt6i_flags |= RTF_EXPIRES;
>> +	if (rt0->rt6i_flags & (RTF_CACHE | RTF_EXPIRES))
>> +		rt0->rt6i_flags |= RTF_EXPIRES;
>
> This looks wrong. What could be the point of settinf RTF_EXPIRES if its
> already set ?
>

This is a good point.  It was clearer to me at the time to include it 
(more similar to the old implementation which set the bit 
unconditionally), but I don't really care.

>>   }
>>
>>   static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from)
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f74b041..a509a06 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -947,8 +947,19 @@ restart:
>>   		nrt = rt6_alloc_cow(rt, &fl6->daddr, &fl6->saddr);
>>   	else if (!(rt->dst.flags & DST_HOST))
>>   		nrt = rt6_alloc_clone(rt, &fl6->daddr);
>> -	else
>> +	else {
>> +		if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.expires &&
>> +		    time_after(jiffies, rt->dst.expires)) {
>> +			/* Uncached routes may have expires set if we
>> +			 * intend to expire the MTU but not the dest
>> +			 * itself.  In that case, we should reset the mtu
>> +			 * before handing it back */
>> +			dst_metric_set(&rt->dst, RTAX_MTU, 0);
>> +			rt6_clean_expires(rt);
>> +			rt->rt6i_flags &= ~RTF_MODIFIED;
>
> Many cpus can perform this at the same time on same route, this looks
> racy.

Initially I was just going to agree with you here, but taking another 
look at ip_vs_xmit at least, there doesn't appear to be any special 
locking before invoking ->update_pmtu, which is playing with rt6i_flags 
and dst.expires as well.  Is that racy as well or is there something 
else I'm missing here?

There are other ways to skin this particular cat though, and I've got no 
specific attachment to any of them.  The most logical thing to do IMO is 
clone the route when it may be necessary to do so, but given the fact 
that that was very deliberately undone in 7343ff3 "ipv6: Don't create 
clones of host routes," I'm not sure that it's the right thing to do or 
that it won't require major surgery.

Thanks again,

-- 
Alex Gartrell <agartrell@fb.com>