[net-next,4/6] ipv6: Only create RTF_CACHE routes after encountering pmtu exception

This patch creates a RTF_CACHE routes only after encountering a pmtu
exception.

After ip6_rt_update_pmtu() has inserted the RTF_CACHE route to the fib6
tree, the rt->rt6i_node->fn_sernum is bumped which will fail the
ip6_dst_check() and trigger a relookup.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv6/route.c | 96 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

On Tue, Apr 28, 2015 at 02:07:51PM -0700, Martin KaFai Lau wrote:
>  
> -static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
> -			       struct sk_buff *skb, u32 mtu)
> +static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
> +				 const struct ipv6hdr *iph, u32 mtu)
>  {
>  	struct rt6_info *rt6 = (struct rt6_info *)dst;
> +	struct net *net;
> +
> +	if (rt6->rt6i_flags & RTF_LOCAL)
> +		return;
>  
>  	dst_confirm(dst);
> -	if (mtu < dst_mtu(dst) && (rt6->rt6i_flags & RTF_CACHE)) {
> -		struct net *net = dev_net(dst->dev);
> +	mtu = max_t(u32, mtu, IPV6_MIN_MTU);
> +	if (mtu >= dst_mtu(dst))
> +		return;
>  
> -		rt6->rt6i_flags |= RTF_MODIFIED;
> -		if (mtu < IPV6_MIN_MTU)
> -			mtu = IPV6_MIN_MTU;
> +	if (!(rt6->rt6i_flags & RTF_CACHE)) {
> +		const struct in6_addr *daddr, *saddr;
> +		struct rt6_info *nrt6;
>  
> -		rt6->rt6i_pmtu = mtu;
> -		rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);
> +		if (iph) {
> +			daddr = &iph->daddr;
> +			saddr = &iph->saddr;
> +		} else if (sk) {
> +			daddr = &sk->sk_v6_daddr;
> +			saddr = &inet6_sk(sk)->saddr;
> +		} else {
> +			return;
> +		}
> +		nrt6 = ip6_pmtu_rt_cache_alloc(rt6, daddr, saddr);
> +		if (!nrt6)
> +			return;
> +		/* ip6_ins_rt(nrt6) will bump the rt6->rt6i_node->fn_sernum
> +		 * which will fail the next rt6_check() and invalidate the
> +		 * sk->sk_dst_cache.
> +		 */
> +		if (ip6_ins_rt(nrt6)) {
> +			dst_destroy(&nrt6->dst);

fib6_add() does a dst_free() on error, so calling dst_destroy()
here might result in a use after free.


> +			return;
> +		}
> +
> +		rt6 = nrt6;
> +		dst = &nrt6->dst;
>  	}
> +
> +	net = dev_net(dst->dev);
> +	rt6->rt6i_flags |= RTF_MODIFIED;
> +	rt6->rt6i_pmtu = mtu;
> +	rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);

The update of expires and the setting of rt6i_pmtu should
happen before the route is inserted with ip6_ins_rt().
This is because fib6_add_rt2node() tries to reuse old
expired routes if still in the fib tree, the necessary
informations are copied from the new route before it
returnes -EEXIST on the new route. If your new route
has no expires value set, fib6_add_rt2node() cleans
expires of the old route before it resues it.

Also rt6i_pmtu should be copied to the reused route in
fib6_add_rt2node(), this should be done already in your
first patchset. Otherwise we might use stale pmtu informations.

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

On Wed, Apr 29, 2015 at 01:39:18PM +0200, Steffen Klassert wrote:
> On Tue, Apr 28, 2015 at 02:07:51PM -0700, Martin KaFai Lau wrote:
> > +		if (ip6_ins_rt(nrt6)) {
> > +			dst_destroy(&nrt6->dst);
> 
> fib6_add() does a dst_free() on error, so calling dst_destroy()
> here might result in a use after free.
Good catch.

> 
> 
> > +			return;
> > +		}
> > +
> > +		rt6 = nrt6;
> > +		dst = &nrt6->dst;
> >  	}
> > +
> > +	net = dev_net(dst->dev);
> > +	rt6->rt6i_flags |= RTF_MODIFIED;
> > +	rt6->rt6i_pmtu = mtu;
> > +	rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);
> 
> The update of expires and the setting of rt6i_pmtu should
> happen before the route is inserted with ip6_ins_rt().
>
> This is because fib6_add_rt2node() tries to reuse old
> expired routes if still in the fib tree, the necessary
> informations are copied from the new route before it
> returnes -EEXIST on the new route. If your new route
> has no expires value set, fib6_add_rt2node() cleans
> expires of the old route before it resues it.
>
> Also rt6i_pmtu should be copied to the reused route in
> fib6_add_rt2node(), this should be done already in your
> first patchset. Otherwise we might use stale pmtu informations.
Good catch.

A similar race may also happen in the current ip6_pol_route()
where it may clear the RTF_EXPIRES of the existing pmtu clone.

Hence, copying rt6i_pmtu (at fib6_add_rt2node()) in the last patchset will
not be right.

I will do the copying and early-set-expire in this patchset instead.

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