Adding IPv4/IPv6 maintainers in CC, along with David Ahern's k.org email
address.
----- On Jul 20, 2020, at 6:11 PM, Mathieu Desnoyers
mathieu.desnoy...@efficios.com wrote:
> As per RFC792, ICMP errors should be sent to the source host.
>
> However, in configurations with Virtual Forwarding and Routing tables,
> looking up which routing table to use is currently done by using the
> destination net_device.
>
> commit 9d1a6c4ea43e ("net: icmp_route_lookup should use rt dev to
> determine L3 domain") changes the interfaces passed to
> l3mdev_master_ifindex() and inet_addr_type_dev_table() from skb_in->dev
> to skb_dst(skb_in)->dev in order to fix a NULL pointer dereference. This
> changes the interface used for routing table lookup from source to
> destination. Therefore, if the source and destination interfaces are
> within separate VFR, or one in the global routing table and the other in
> a VFR, looking up the source host in the destination interface's routing
> table is likely to fail.
>
> One observable effect of this issue is that traceroute does not work in
> the following cases:
>
> - Route leaking between global routing table and VRF
> - Route leaking between VRFs
>
> [ Note 1: I'm not entirely sure what routing table should be used when
> param->replyopts.opt.opt.srr is set ? Is it valid to honor Strict
> Source Route when sending an ICMP error ? ]
>
> [ Note 2: I notice that ipv6 icmp6_send() uses skb_dst(skb)->dev as
> argument to l3mdev_master_ifindex(). I'm not sure if it is correct ? ]
>
> [ This patch is only compile-tested. ]
>
> Fixes: 9d1a6c4ea43e ("net: icmp_route_lookup should use rt dev to determine L3
> domain")
> Link: https://tools.ietf.org/html/rfc792
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoy...@efficios.com>
> Cc: David Ahern <d...@cumulusnetworks.com>
> Cc: David S. Miller <da...@davemloft.net>
> Cc: netdev@vger.kernel.org
> ---
> net/ipv4/icmp.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index e30515f89802..3d1da70c7293 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -465,6 +465,7 @@ static struct rtable *icmp_route_lookup(struct net *net,
> int type, int code,
> struct icmp_bxm *param)
> {
> + struct net_device *route_lookup_dev;
> struct rtable *rt, *rt2;
> struct flowi4 fl4_dec;
> int err;
> @@ -479,7 +480,14 @@ static struct rtable *icmp_route_lookup(struct net *net,
> fl4->flowi4_proto = IPPROTO_ICMP;
> fl4->fl4_icmp_type = type;
> fl4->fl4_icmp_code = code;
> - fl4->flowi4_oif = l3mdev_master_ifindex(skb_dst(skb_in)->dev);
> + /*
> + * 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.
> + */
> + route_lookup_dev = skb_in->dev ? skb_in->dev : skb_dst(skb_in)->dev;
> + fl4->flowi4_oif = l3mdev_master_ifindex(route_lookup_dev);
>
> security_skb_classify_flow(skb_in, flowi4_to_flowi(fl4));
> rt = ip_route_output_key_hash(net, fl4, skb_in);
> @@ -503,7 +511,7 @@ static struct rtable *icmp_route_lookup(struct net *net,
> if (err)
> goto relookup_failed;
>
> - if (inet_addr_type_dev_table(net, skb_dst(skb_in)->dev,
> + if (inet_addr_type_dev_table(net, route_lookup_dev,
> fl4_dec.saddr) == RTN_LOCAL) {
> rt2 = __ip_route_output_key(net, &fl4_dec);
> if (IS_ERR(rt2))
> --
> 2.11.0
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
