wgengine/netstack: always respond to 4via6 echo requests (#5712)

As the comment in the code says, netstack should always respond to ICMP
echo requests to a 4via6 address, even if the netstack instance isn't
normally processing subnet traffic.

Follow-up to #5709

Change-Id: I504d0776c5824071b2a2e0e687bc33e24f6c4746
Signed-off-by: Andrew Dunham <andrew@tailscale.com>

wgengine/netstack/netstack.go

@@ -716,22 +716,21 @@ func (ns *Impl) shouldHandlePing(p *packet.Parsed) (_ netip.Addr, ok bool) {
 	if !p.IsEchoRequest() {
 		return netip.Addr{}, false
 	}
-	if !ns.ProcessSubnets {
-		return netip.Addr{}, false
-	}
 
 	destIP := p.Dst.Addr()
 
-	// For non-4via6 addresses, we don't handle pings if they're destined
+	// We need to handle pings for all 4via6 addresses, even if this
-	// for a Tailscale IP.
+	// netstack instance normally isn't responsible for processing subnets.
-	if !viaRange.Contains(destIP) {
+	//
-		if tsaddr.IsTailscaleIP(destIP) {
+	// For example, on Linux, subnet router traffic could be handled via
-			return netip.Addr{}, false
+	// tun+iptables rules for most packets, but we still need to handle
-		}
+	// ICMP echo requests over 4via6 since the host networking stack
-
+	// doesn't know what to do with a 4via6 address.
-		return destIP, true
+	//
-	}
+	// shouldProcessInbound returns 'true' to say that we should process
-
+	// all IPv6 packets with a destination address in the 'via' range, so
+	// check before we check the "ProcessSubnets" boolean below.
+	if viaRange.Contains(destIP) {
 		// The input echo request was to a 4via6 address, which we cannot
 		// simply ping as-is from this process. Translate the destination to an
 		// IPv4 address, so that our relayed ping (in userPing) is pinging the
@@ -743,6 +742,23 @@ func (ns *Impl) shouldHandlePing(p *packet.Parsed) (_ netip.Addr, ok bool) {
 		// case things are safe because the 'userPing' function doesn't make
 		// use of the input packet.
 		return tsaddr.UnmapVia(destIP), true
+	}
+
+	// If we get here, we don't do anything unless this netstack instance
+	// is responsible for processing subnet traffic.
+	if !ns.ProcessSubnets {
+		return netip.Addr{}, false
+	}
+
+	// For non-4via6 addresses, we don't handle pings if they're destined
+	// for a Tailscale IP.
+	if tsaddr.IsTailscaleIP(destIP) {
+		return netip.Addr{}, false
+	}
+
+	// This netstack instance is processing subnet traffic, so handle the
+	// ping ourselves.
+	return destIP, true
 }
 
 func netaddrIPFromNetstackIP(s tcpip.Address) netip.Addr {

wgengine/netstack/netstack_test.go

@@ -5,6 +5,7 @@
 package netstack
 
 import (
+	"fmt"
 	"net/netip"
 	"runtime"
 	"testing"
@@ -216,7 +217,9 @@ func TestShouldHandlePing(t *testing.T) {
 		}
 	})
 
-	t.Run("ICMP6-4via6", func(t *testing.T) {
+	// Handle pings for 4via6 addresses regardless of ProcessSubnets
+	for _, subnets := range []bool{true, false} {
+		t.Run("ICMP6-4via6-ProcessSubnets-"+fmt.Sprint(subnets), func(t *testing.T) {
 			// The 4via6 route 10.1.1.0/24 siteid 7, and then the IP
 			// 10.1.1.9 within that route.
 			dst := netip.MustParseAddr("fd7a:115c:a1e0:b1a:0:7:a01:109")
@@ -236,16 +239,16 @@ func TestShouldHandlePing(t *testing.T) {
 			pkt.Decode(icmpPing)
 
 			impl := makeNetstack(t, func(impl *Impl) {
-			impl.ProcessSubnets = true
+				impl.ProcessSubnets = subnets
 			})
 			pingDst, ok := impl.shouldHandlePing(pkt)
 
 			// Handled due to being 4via6
 			if !ok {
 				t.Errorf("expected shouldHandlePing==true")
-		}
+			} else if pingDst != expectedPingDst {
-		if pingDst != expectedPingDst {
 				t.Errorf("got dst %s; want %s", pingDst, expectedPingDst)
 			}
 		})
+	}
 }