Re: [PATCH net] ipv4: fix route lookups when handling ICMP redirects and PMTU updates

From: David Ahern <dsahern@kernel.org>
To: Guillaume Nault <gnault@redhat.com>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Subject: Re: [PATCH net] ipv4: fix route lookups when handling ICMP redirects and PMTU updates
Date: Mon, 28 Feb 2022 10:31:58 -0700	[thread overview]
Message-ID: <922b4932-fcd5-d362-4679-6689046560c7@kernel.org> (raw)
In-Reply-To: <cffd245430d10fa2a14c32d1c768eef7cfeb8963.1646068241.git.gnault@redhat.com>

On 2/28/22 10:16 AM, Guillaume Nault wrote:
> Fixes: d3a25c980fc2 ("ipv4: Fix nexthop exception hash computation.")

That does not seem related to tos in the flow struct at all.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index f33ad1f383b6..d5d058de3664 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -499,6 +499,15 @@ void __ip_select_ident(struct net *net, struct iphdr *iph, int segs)
>  }
>  EXPORT_SYMBOL(__ip_select_ident);
>  
> +static void ip_rt_fix_tos(struct flowi4 *fl4)

make this a static inline in include/net/flow.h and update
flowi4_init_output and flowi4_update_output to use it. That should cover
a few of the cases below leaving just  ...

> +{
> +	__u8 tos = RT_FL_TOS(fl4);
> +
> +	fl4->flowi4_tos = tos & IPTOS_RT_MASK;
> +	fl4->flowi4_scope = tos & RTO_ONLINK ?
> +			    RT_SCOPE_LINK : RT_SCOPE_UNIVERSE;
> +}
> +
>  static void __build_flow_key(const struct net *net, struct flowi4 *fl4,
>  			     const struct sock *sk,
>  			     const struct iphdr *iph,
> @@ -824,6 +833,7 @@ static void ip_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_buf
>  	rt = (struct rtable *) dst;
>  
>  	__build_flow_key(net, &fl4, sk, iph, oif, tos, prot, mark, 0);
> +	ip_rt_fix_tos(&fl4);
>  	__ip_do_redirect(rt, skb, &fl4, true);
>  }
>  
> @@ -1048,6 +1058,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
>  	struct flowi4 fl4;
>  
>  	ip_rt_build_flow_key(&fl4, sk, skb);
> +	ip_rt_fix_tos(&fl4);
>  
>  	/* Don't make lookup fail for bridged encapsulations */
>  	if (skb && netif_is_any_bridge_port(skb->dev))
> @@ -1122,6 +1133,8 @@ void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu)
>  			goto out;
>  
>  		new = true;
> +	} else {
> +		ip_rt_fix_tos(&fl4);
>  	}
>  
>  	__ip_rt_update_pmtu((struct rtable *)xfrm_dst_path(&rt->dst), &fl4, mtu);
> @@ -2603,7 +2616,6 @@ static struct rtable *__mkroute_output(const struct fib_result *res,
>  struct rtable *ip_route_output_key_hash(struct net *net, struct flowi4 *fl4,
>  					const struct sk_buff *skb)
>  {
> -	__u8 tos = RT_FL_TOS(fl4);
>  	struct fib_result res = {
>  		.type		= RTN_UNSPEC,
>  		.fi		= NULL,
> @@ -2613,9 +2625,7 @@ struct rtable *ip_route_output_key_hash(struct net *net, struct flowi4 *fl4,
>  	struct rtable *rth;
>  
>  	fl4->flowi4_iif = LOOPBACK_IFINDEX;
> -	fl4->flowi4_tos = tos & IPTOS_RT_MASK;
> -	fl4->flowi4_scope = ((tos & RTO_ONLINK) ?
> -			 RT_SCOPE_LINK : RT_SCOPE_UNIVERSE);
> +	ip_rt_fix_tos(fl4);

... this one to call the new helper.

>  
>  	rcu_read_lock();
>  	rth = ip_route_output_key_hash_rcu(net, fl4, &res, skb);