fix(tun): drop non-IP frames at the hub instead of writing them to the device

A frame that is not a parseable IPv4 or IPv6 packet is exactly what the kernel refuses. Measured on a real up tun device: the version nibble is the only thing it inspects, and anything but 4 or 6 comes back EINVAL at any length -- 1421, 1500 and 2000-byte frames with a valid header all go through untouched. That error left fromSpoke, unwound Handle, and took a 54-minute peer stream down with it. The spoke has always refused these frames before its device write, which is why a phone logs "unknown packet, discarded" all day and never dies while the hub had no equivalent. destinationOf returning !ok is exactly the kernel's reject condition, so that branch now drops and counts the frame instead of handing it to the device. This stops the malformed frame from killing the service; it does not fix what put one on the wire. The candidate cause is concurrent serveEdge writers racing on one local conn with no send lock on streamConn.Write, which is still unproven. Silently accepted sub-MTU spliced frames also remain -- the kernel does not validate TotalLen either.
2026-10-19 16:16:32 +00:00 · 2026-10-11 02:30:15 +00:00
parent a654aee7af
commit 4f143aa694
2 changed files with 185 additions and 0 deletions
@@ -0,0 +1,136 @@
+package tun
+
+import (
+	"context"
+	"net"
+	"testing"
+	"time"
+
+	xlogger "github.com/go-gost/x/logger"
+)
+
+// recordDevice remembers what the hub handed to the device, so a test can
+// assert a frame never reached it.
+type recordDevice struct {
+	nopDevice
+	wrote [][]byte
+}
+
+func (d *recordDevice) Write(p []byte) (int, error) {
+	d.wrote = append(d.wrote, append([]byte(nil), p...))
+	return len(p), nil
+}
+
+// A frame that is not a parseable IPv4 or IPv6 packet is one the kernel will
+// refuse with EINVAL — measured, not assumed: on a real up tun device the
+// version nibble is the only thing it checks, and anything other than 4 or 6
+// comes back EINVAL at any length. Handing one to the device is what killed
+// the hub, because that error propagates out of fromSpoke and tears down the
+// peer's whole stream. So the frame is dropped and counted instead, exactly
+// as an oversized one already is.
+func TestP2PNonIPDatagramKeepsStream(t *testing.T) {
+	garbage := func(n int, first byte) []byte {
+		p := make([]byte, n)
+		for i := range p {
+			p[i] = 0xFF
+		}
+		p[0] = first
+		return p
+	}
+
+	tests := []struct {
+		name string
+		pkt  []byte
+	}{
+		{"version 5", garbage(60, 0x55)},
+		{"version 7", garbage(60, 0x75)},
+		{"all zeroes", garbage(60, 0x00)},
+		{"empty", []byte{}},
+		{"oversized and non-IP", garbage(20530, 0x55)},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			dev := &recordDevice{}
+			var warns []string
+			hub := newP2PHub(dev, newTestRouter(), nil,
+				func(s string) { warns = append(warns, s) })
+			_, peerEnd := newDatagramPipe(4)
+			s := newPeerStream(context.Background(), "peer-a", peerEnd)
+
+			if err := hub.fromSpoke(s, tc.pkt); err != nil {
+				t.Fatalf("fromSpoke: %v, want nil (drop, don't kill the stream)", err)
+			}
+			if len(dev.wrote) != 0 {
+				t.Fatalf("device got %d writes, want 0", len(dev.wrote))
+			}
+			if len(warns) != 1 {
+				t.Fatalf("warnings = %d, want 1 (a dropped frame must stay visible)", len(warns))
+			}
+
+			// The guard must not have cost the peer its link: the next
+			// well-formed packet still has to reach the device.
+			good := udpPacket("10.0.0.1", "10.0.0.9", 1, 3)
+			if err := hub.fromSpoke(s, good); err != nil {
+				t.Fatalf("fromSpoke (valid packet after a bad one): %v", err)
+			}
+			if len(dev.wrote) != 1 {
+				t.Fatalf("device got %d writes, want 1", len(dev.wrote))
+			}
+		})
+	}
+}
+
+// The spoke's downlink is the same hazard and already carries this guard —
+// which is why the phone logs "unknown packet, discarded" all day and never
+// dies, while the hub has no equivalent. This pins that asymmetry: if the
+// spoke's check is ever dropped, the device must still receive nothing, and
+// the run must end on the deadline rather than on an EINVAL from the kernel.
+func TestTransportClientDropsNonIPFrames(t *testing.T) {
+	device, kernelSide := net.Pipe()
+	ccA, ccB := net.Pipe()
+	defer device.Close()
+	defer ccA.Close()
+
+	h := newTestHandler(nil, nil)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	deviceGot := make(chan []byte, 1)
+	go func() {
+		var acc []byte
+		buf := make([]byte, 64)
+		kernelSide.SetReadDeadline(time.Now().Add(500 * time.Millisecond))
+		for {
+			n, err := kernelSide.Read(buf[:])
+			if n > 0 {
+				acc = append(acc, buf[:n]...)
+			}
+			if err != nil {
+				break
+			}
+		}
+		deviceGot <- acc
+	}()
+
+	errCh := make(chan error, 1)
+	go func() { errCh <- h.transportClient(ctx, device, ccA, xlogger.Nop()) }()
+
+	bad := make([]byte, 60)
+	for i := range bad {
+		bad[i] = 0x55 // version 5: not IPv4, not IPv6
+	}
+	if _, err := ccB.Write(bad); err != nil {
+		t.Fatal(err)
+	}
+	// A well-formed packet behind it must still arrive: if the guard dropped
+	// the link instead of the frame, this never lands.
+	good := realIPv4Packet()
+	if _, err := ccB.Write(good); err != nil {
+		t.Fatal(err)
+	}
+
+	got := <-deviceGot
+	if len(got) != len(good) {
+		t.Fatalf("device received %d bytes, want %d (only the well-formed packet)", len(got), len(good))
+	}
+}
@@ -220,6 +220,13 @@ type p2pHub struct {
 	overMu    sync.Mutex
 	overCount int
 	overSince time.Time
+
+	// nonIPMu guards the tally that countNonIP keeps. Same shape again: a peer
+	// whose link is losing frame boundaries sends these continuously, and one
+	// line per packet would bury the log it is meant to explain.
+	nonIPMu    sync.Mutex
+	nonIPCount int
+	nonIPSince time.Time
 }

 func newP2PHub(device io.ReadWriter, router *peerRouter, ownNets []net.IPNet, warn func(string)) *p2pHub {
@@ -375,6 +382,33 @@ func (h *p2pHub) countOversize(size int) {
 		time.Since(since).Round(time.Millisecond).String())
 }

+// countNonIP records one frame that was not a parseable IP packet, reporting
+// the first at once and the rest once per window — the same shape as the two
+// tallies above, for the same reason: the condition is a symptom of a link
+// losing its framing, so it arrives in bursts and has to stay countable
+// without becoming the log.
+func (h *p2pHub) countNonIP(size int) {
+	h.nonIPMu.Lock()
+	now := time.Now()
+	if h.nonIPSince.IsZero() {
+		h.nonIPSince = now
+		h.nonIPCount = 1
+		h.nonIPMu.Unlock()
+		h.warnf("non-IP frame (%d bytes), discarded", size)
+		return
+	}
+	h.nonIPCount++
+	if now.Sub(h.nonIPSince) < unroutedWindow {
+		h.nonIPMu.Unlock()
+		return
+	}
+	n, since := h.nonIPCount, h.nonIPSince
+	h.nonIPSince, h.nonIPCount = now, 0
+	h.nonIPMu.Unlock()
+	h.warnf("non-IP frames discarded: %d in %s", n,
+		time.Since(since).Round(time.Millisecond).String())
+}
+
 // fromSpoke handles one datagram from one peer: a keepalive is answered on the
 // peer it arrived from, a packet for another registered peer is delivered
 // straight to that peer's stream, and anything else is the device's.
@@ -450,6 +484,21 @@ func (h *p2pHub) fromSpoke(s *peerStream, pkt []byte) error {
 			h.countUnrouted(dst)
 			return nil
 		}
+	} else {
+		// Not a parseable IPv4 or IPv6 packet, which is the one thing the
+		// kernel is certain to refuse. Measured on a real up tun device: the
+		// version nibble is the *only* thing it inspects — anything but 4 or 6
+		// comes back EINVAL at any length, while 1421, 1500 and 2000-byte
+		// frames with a valid header all go through untouched. So this branch,
+		// not size, is the one that killed the hub: the error left fromSpoke,
+		// unwound Handle, and took a 54-minute peer stream down with it.
+		//
+		// Dropping here is also the symmetric choice: the spoke already refuses
+		// the same frames before its device write (client.go), which is why a
+		// phone logs "unknown packet, discarded" all day and never dies while
+		// the hub had no equivalent.
+		h.countNonIP(len(pkt))
+		return nil
 	}

 	h.wmu.Lock()