| Message ID | 20200811195003.1812-4-mathieu.desnoyers@efficios.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/3] selftests: Add VRF icmp error route lookup test | expand |
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); > >
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; + 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; icmpv6_send(skb, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT, 0); __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
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(-)