Re: [PATCH net 2/2] ipv6: ip6_dst_check needs to check for expired dst_entries

[Valentijn Sessink <valentyn@blub.net>]

On 24-10-13 07:48, Hannes Frederic Sowa wrote:
> On receiving a packet too big icmp error we check if our current cached
> dst_entry in the socket is still valid. This validation check did not
> care about the expiration of the (cached) route.
> 
> The error path I traced down:
> The socket receives a packet too big mtu notification. It still has a
> valid dst_entry and thus issues the ip6_rt_pmtu_update on this dst_entry,
> setting RTF_EXPIRE and updates the dst.expiration value (which could
> fail because of not up-to-date expiration values, see previous patch).
> 
> In some seldom cases we race with a) the ip6_fib gc or b) another routing
> lookup which would result in a recreation of the cached rt6_info from its
> parent non-cached rt6_info. While copying the rt6_info we reinitialize the
> metrics store by copying it over from the parent thus invalidating the
> just installed pmtu update (both dsts use the same key to the inetpeer
> storage). The dst_entry with the just invalidated metrics data would
> just get its RTF_EXPIRES flag cleared and would continue to stay valid
> for the socket.
> 
> We should have not issued the pmtu update on the already expired dst_entry
> in the first placed. By checking the expiration on the dst entry and
> doing a relookup in case it is out of date we close the race because
> we would install a new rt6_info into the fib before we issue the pmtu
> update, thus closing this race.
> 
> Not reliably updating the dst.expire value was fixed by the patch "ipv6:
> reset dst.expires value when clearing expire flag".
> 
> Reported-by: Steinar H. Gunderson <sgunderson@bigfoot.com>
> Reported-by: Valentijn Sessink <valentyn@blub.net>
> Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> I would propose this patch for -stable.
> 
>  net/ipv6/route.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f54e3a1..04e17b3 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1087,10 +1087,13 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
>  	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
>  		return NULL;
>  
> -	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
> -		return dst;
> +	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
> +		return NULL;
>  
> -	return NULL;
> +	if (rt6_check_expired(rt))
> +		return NULL;
> +
> +	return dst;
>  }
>  
>  static struct dst_entry *ip6_negative_advice(struct dst_entry *dst)

Tested-by: Valentijn Sessink <valentyn@blub.net>