Re: [PATCH 3/3] ipv6/icmp: l3mdev: Perform icmp error route lookup on source device routing table

From: David Ahern <dsahern@gmail.com>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	David Ahern <dsahern@kernel.org>
Cc: linux-kernel@vger.kernel.org,
	"David S . Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org,
	Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Subject: Re: [PATCH 3/3] ipv6/icmp: l3mdev: Perform icmp error route lookup on source device routing table
Date: Thu, 13 Aug 2020 17:21:55 -0600	[thread overview]
Message-ID: <2adc1bd3-53aa-0f3e-d5e4-740d11098685@gmail.com> (raw)
In-Reply-To: <20200811195003.1812-4-mathieu.desnoyers@efficios.com>

On 8/11/20 1:50 PM, Mathieu Desnoyers wrote:
> As per RFC4443, the destination address field for ICMPv6 error messages
> is copied from the source address field of the invoking packet.
> 
> In configurations with Virtual Routing and Forwarding tables, looking up
> which routing table to use for sending ICMPv6 error messages is
> currently done by using the destination net_device.
> 
> If the source and destination interfaces are within separate VRFs, or
> one in the global routing table and the other in a VRF, looking up the
> source address of the invoking packet in the destination interface's
> routing table will fail if the destination interface's routing table
> contains no route to the invoking packet's source address.
> 
> One observable effect of this issue is that traceroute6 does not work in
> the following cases:
> 
> - Route leaking between global routing table and VRF
> - Route leaking between VRFs
> 
> Preferably use the source device routing table when sending ICMPv6 error
> messages. If no source device is set, fall-back on the destination
> device routing table.
> 
> Link: https://tools.ietf.org/html/rfc4443
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> ---
>  net/ipv6/icmp.c       | 15 +++++++++++++--
>  net/ipv6/ip6_output.c |  2 --
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index a4e4912ad607..a971b58b0371 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -501,8 +501,19 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
>  	if (__ipv6_addr_needs_scope_id(addr_type)) {
>  		iif = icmp6_iif(skb);
>  	} else {
> -		dst = skb_dst(skb);
> -		iif = l3mdev_master_ifindex(dst ? dst->dev : skb->dev);
> +		struct net_device *route_lookup_dev = NULL;
> +
> +		/*
> +		 * The device used for looking up which routing table to use is
> +		 * preferably the source whenever it is set, which should
> +		 * ensure the icmp error can be sent to the source host, else
> +		 * fallback on the destination device.
> +		 */
> +		if (skb->dev)
> +			route_lookup_dev = skb->dev;

top of icmp6_send there is a check that skb->dev is set.

> +		else if (skb_dst(skb))
> +			route_lookup_dev = skb_dst(skb)->dev;
> +		iif = l3mdev_master_ifindex(route_lookup_dev);
>  	}
>  
>  	/*
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index c78e67d7747f..cd623068de53 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -468,8 +468,6 @@ int ip6_forward(struct sk_buff *skb)
>  	 *	check and decrement ttl
>  	 */
>  	if (hdr->hop_limit <= 1) {
> -		/* Force OUTPUT device used as source address */
> -		skb->dev = dst->dev;

I *think* this ok. Not clear to me why the forward path would change the
skb->dev like that. Goes back to beginning of the git history.

>  		icmpv6_send(skb, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT, 0);
>  		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
>  
> 