small bugs

overlay/tio/tio.go

@@ -27,6 +27,12 @@ type Capabilities struct {
 // Queue is a readable/writable Poll queue.
 // Concurrency contract: a single read goroutine drives Read; plain Write is safe for concurrent callers;
 // WriteGSO (on Queues that implement GSOWriter) is single-writer per queue.
+//
+// Close on an individual Queue does NOT unblock a Read parked in poll — closing an fd
+// never wakes its pollers. Orderly teardown goes through the owning QueueSet's Close,
+// which first signals a shared shutdown eventfd every reader polls alongside its own fd.
+// That eventfd is a set-wide kill switch: once signaled, every Queue in the set returns
+// os.ErrClosed from Read, so it cannot be used to stop a single Queue.
 type Queue interface {
 	io.Closer
 

overlay/tio/tio_gso_linux.go

@@ -51,27 +51,18 @@ var validVnetHdr = [virtio.Size]byte{unix.VIRTIO_NET_HDR_F_DATA_VALID}
 
 // Offload wraps a TUN file descriptor with poll-based reads. The FD provided will be changed to non-blocking.
 // A shared eventfd allows Close to wake all readers blocked in poll.
+//
+// Field order is deliberate: the read-mostly fds and the writer-owned GSO scratch fill
+// the first cache line, and the state the reader mutates per packet (rxOff, pending,
+// readIovs) all sits after it, so per-packet reader stores never invalidate the line
+// concurrent Write callers load fd from.
 type Offload struct {
 	fd         int
 	shutdownFd int
-	closed     atomic.Bool
-	rxBuf      []byte   // backing store for kernel-handed packets read this drain
-	rxOff      int      // cursor into rxBuf for the current Read drain
-	pending    []Packet // packets returned from the most recent Read
-
-	// readVnetScratch holds the 10-byte virtio_net_hdr split off the front of
-	// every TUN read via readv(2). Decoupling the header from the packet body
-	// lets us read the body directly into rxBuf at the current rxOff with
-	// no userspace copy on the GSO_NONE fast path.
-	readVnetScratch [virtio.Size]byte
-	// readIovs is the readv(2) iovec scratch wired once at construction,
-	// iovec[0] points at readVnetScratch
-	// iovec[1].Base/Len is updated per read to address the current rxBuf slot.
-	readIovs [2]unix.Iovec
-
 	// usoEnabled records whether the kernel agreed to TUN_F_USO* on this FD,
 	// so writers can decide whether emitting GSO_UDP_L4 superpackets is safe.
 	usoEnabled bool
+	closed     atomic.Bool
 
 	// gsoHdrBuf is a per-queue 10-byte scratch for the virtio_net_hdr emitted
 	// by WriteGSO. Kept separate from the read-only package-level validVnetHdr
@@ -82,6 +73,20 @@ type Offload struct {
 	// gsoMaxIovs at construction; never grown. WriteGSO returns an error
 	// (and drops the call) if a caller hands it more fragments than fit.
 	gsoIovs []unix.Iovec
+
+	rxBuf   []byte   // backing store for kernel-handed packets read this drain
+	rxOff   int      // cursor into rxBuf for the current Read drain
+	pending []Packet // packets returned from the most recent Read
+
+	// readVnetScratch holds the 10-byte virtio_net_hdr split off the front of
+	// every TUN read via readv(2). Decoupling the header from the packet body
+	// lets us read the body directly into rxBuf at the current rxOff with
+	// no userspace copy on the GSO_NONE fast path.
+	readVnetScratch [virtio.Size]byte
+	// readIovs is the readv(2) iovec scratch wired once at construction,
+	// iovec[0] points at readVnetScratch
+	// iovec[1].Base/Len is updated per read to address the current rxBuf slot.
+	readIovs [2]unix.Iovec
 }
 
 func newOffload(fd int, shutdownFd int, usoEnabled bool) (*Offload, error) {
@@ -129,7 +134,7 @@ func (r *Offload) readPacket(block bool) (int, error) {
 		n, _, errno := syscall.Syscall(unix.SYS_READV, uintptr(r.fd), uintptr(unsafe.Pointer(&r.readIovs[0])), uintptr(len(r.readIovs)))
 		if errno == 0 {
 			if int(n) < virtio.Size {
-				return 0, io.ErrShortWrite
+				return 0, fmt.Errorf("tun read shorter than virtio_net_hdr: %d bytes", n)
 			}
 			return int(n) - virtio.Size, nil
 		}
@@ -378,8 +383,9 @@ func (r *Offload) Close() error {
 	}
 
 	// shutdownFd is owned by the container, so we should not close it
-	// Close the underlying fd but do NOT null r.fd: a reader may still be loading it in readOne, and mutating the field would race that load.
+	// Close the underlying fd but do NOT null r.fd: a reader may still be loading it in readPacket, and mutating the field would race that load.
-	// That reader gets EBADF -> os.ErrClosed (or wakes via the shutdown eventfd's ppoll first).
+	// That reader gets EBADF -> os.ErrClosed on its next syscall. A reader already parked in
+	// poll is NOT woken by this close; only the QueueSet's shutdown eventfd wake does that (see Queue.Close docs).
 	// closed.Swap already guarantees we only close once.
 	return unix.Close(r.fd)
 }

overlay/tio/tio_poll_linux.go

@@ -105,8 +105,9 @@ func (t *Poll) Close() error {
 	}
 
 	// shutdownFd is owned by the container, so we should not close it
-	// Close the underlying fd but do NOT null r.fd: a reader may still be loading it in readOne, and mutating the field would race that load.
+	// Close the underlying fd but do NOT null t.fd: a reader may still be loading it in readOne, and mutating the field would race that load.
-	// That reader gets EBADF -> os.ErrClosed (or wakes via the shutdown eventfd's ppoll first).
+	// That reader gets EBADF -> os.ErrClosed on its next syscall. A reader already parked in
+	// poll is NOT woken by this close; only the QueueSet's shutdown eventfd wake does that (see Queue.Close docs).
 	// closed.Swap already guarantees we only close once.
 	return unix.Close(t.fd)
 }

overlay/tio/tun_linux_offload_test.go

@@ -397,11 +397,12 @@ func TestSegmentUDPv4(t *testing.T) {
 		if totalLen != uint16(28+gso) {
 			t.Errorf("seg %d: total_len=%d want %d", i, totalLen, 28+gso)
 		}
-		// kernel UDP-GSO does NOT bump the IPv4 ID across segments; every
+		// Software UDP GSO bumps the IPv4 ID per segment exactly like TSO
-		// segment carries the same ID as the seed.
+		// (inet_gso_segment's fixed-ID case is TCP-only); wireguard-go's
+		// gsoSplit increments unconditionally too.
 		id := binary.BigEndian.Uint16(seg[4:6])
-		if id != 0x4242 {
+		if id != 0x4242+uint16(i) {
-			t.Errorf("seg %d: ip id=%#x want %#x", i, id, 0x4242)
+			t.Errorf("seg %d: ip id=%#x want %#x", i, id, 0x4242+uint16(i))
 		}
 		udpLen := binary.BigEndian.Uint16(seg[24:26])
 		if udpLen != uint16(8+gso) {

overlay/tio/virtio/segment_linux.go

@@ -83,6 +83,11 @@ func CheckValid(pkt []byte, hdr Hdr) error {
 	ipVersion := pkt[0] >> 4
 
 	gsoType := hdr.GSOType()
+	if gsoType != unix.VIRTIO_NET_HDR_GSO_NONE && hdr.GSOSize == 0 {
+		// A GSO type with no segment size would dodge IsSuperpacket() downstream and
+		// travel as a plain jumbo datagram with an unfinished checksum.
+		return fmt.Errorf("virtio GSO type %#x with zero gso_size", hdr.gsoType)
+	}
 	if hdr.HasECNFlag() && !(gsoType == unix.VIRTIO_NET_HDR_GSO_TCPV4 || gsoType == unix.VIRTIO_NET_HDR_GSO_TCPV6) {
 		return fmt.Errorf("virtio GSO_ECN qualifier on non-TCP GSO type %#x", hdr.gsoType)
 	}
@@ -167,18 +172,16 @@ func basePseudoSum(pkt []byte, isV4 bool, proto uint32) uint32 {
 
 // baseIPv4HdrSum folds the IPv4 header checksum over the fields that stay constant across segments.
 // csumStart is the L3 header length, which bounds a valid IHL.
-func baseIPv4HdrSum(pkt []byte, csumStart int, zeroID bool) (uint32, error) {
+func baseIPv4HdrSum(pkt []byte, csumStart int) (uint32, error) {
 	ihl := int(pkt[0]&0x0f) * 4
 	if ihl < ipv4HeaderMinLen || ihl > csumStart {
 		return 0, fmt.Errorf("bad IPv4 IHL: %d", ihl)
 	}
-	// total_len and the checksum field itself are always excluded, since both are rewritten per segment.
+	// total_len, the ID, and the checksum field itself are excluded: all three are rewritten per segment.
 	sum := uint32(checksum.Checksum(pkt[:ihl], 0))
 	sum += uint32(^binary.BigEndian.Uint16(pkt[ipv4TotalLenOff : ipv4TotalLenOff+2]))
 	sum += uint32(^binary.BigEndian.Uint16(pkt[ipv4ChecksumOff : ipv4ChecksumOff+2]))
-	if zeroID { //only zero the ID if requested
+	sum += uint32(^binary.BigEndian.Uint16(pkt[ipv4IDOff : ipv4IDOff+2]))
-		sum += uint32(^binary.BigEndian.Uint16(pkt[ipv4IDOff : ipv4IDOff+2]))
-	}
 	sum = (sum & 0xffff) + (sum >> 16)
 	sum = (sum & 0xffff) + (sum >> 16)
 	return sum, nil
@@ -235,7 +238,7 @@ func SegmentTCP(pkt []byte, hdrLenU, csumStartU, gsoSizeU uint16, yield func(seg
 		origIPID = binary.BigEndian.Uint16(pkt[ipv4IDOff : ipv4IDOff+2])
 		var err error
 		// TSO bumps the ID per segment, so it stays out of the base sum.
-		baseIPHdrSum, err = baseIPv4HdrSum(pkt, csumStart, true)
+		baseIPHdrSum, err = baseIPv4HdrSum(pkt, csumStart)
 		if err != nil {
 			return err
 		}
@@ -334,11 +337,14 @@ func SegmentUDP(pkt []byte, hdrLenU, csumStartU, gsoSizeU uint16, yield func(seg
 
 	baseProtoSum := basePseudoSum(pkt, isV4, unix.IPPROTO_UDP)
 
+	var origIPID uint16
 	var baseIPHdrSum uint32
 	if isV4 {
+		origIPID = binary.BigEndian.Uint16(pkt[ipv4IDOff : ipv4IDOff+2])
 		var err error
-		// UDP GSO holds the ID constant across the burst, so it stays in the base sum.
+		// Software UDP GSO bumps the ID per segment just like TSO
-		baseIPHdrSum, err = baseIPv4HdrSum(pkt, csumStart, false)
+		// (inet_gso_segment's fixed-ID case is TCP-only), so it stays out of the base sum.
+		baseIPHdrSum, err = baseIPv4HdrSum(pkt, csumStart)
 		if err != nil {
 			return err
 		}
@@ -367,8 +373,10 @@ func SegmentUDP(pkt []byte, hdrLenU, csumStartU, gsoSizeU uint16, yield func(seg
 		udpLen := udpHeaderLen + segPayLen
 
 		if isV4 {
+			segID := origIPID + uint16(i)
 			binary.BigEndian.PutUint16(seg[ipv4TotalLenOff:ipv4TotalLenOff+2], uint16(totalLen))
-			ipSum := baseIPHdrSum + uint32(totalLen)
+			binary.BigEndian.PutUint16(seg[ipv4IDOff:ipv4IDOff+2], segID)
+			ipSum := baseIPHdrSum + uint32(totalLen) + uint32(segID)
 			binary.BigEndian.PutUint16(seg[ipv4ChecksumOff:ipv4ChecksumOff+2], foldComplement(ipSum))
 		} else {
 			binary.BigEndian.PutUint16(seg[ipv6PayloadLenOff:ipv6PayloadLenOff+2], uint16(headerLen-ipv6FixedLen+segPayLen))

overlay/tio/virtio/segment_linux_test.go

@@ -305,9 +305,10 @@ func TestSegmentUDPHeaderNotCorrupted(t *testing.T) {
 				if dport := binary.BigEndian.Uint16(seg[22:24]); dport != 53 {
 					t.Errorf("seg %d: dport=%d want 53", i, dport)
 				}
-				// UDP-GSO keeps the same IPv4 ID across every segment.
+				// Software UDP GSO bumps the IPv4 ID per segment just like TSO
-				if id := binary.BigEndian.Uint16(seg[4:6]); id != 0x4242 {
+				// (inet_gso_segment's fixed-ID case is TCP-only).
-					t.Errorf("seg %d: ip id=%#x want 0x4242", i, id)
+				if id := binary.BigEndian.Uint16(seg[4:6]); id != 0x4242+uint16(i) {
+					t.Errorf("seg %d: ip id=%#x want %#x", i, id, 0x4242+uint16(i))
 				}
 
 				segPayLen := len(seg) - int(hdrLen)
@@ -459,7 +460,7 @@ func TestCheckValidMasksGSOECN(t *testing.T) {
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := CheckValid(tc.pkt, NewHeader(0, tc.gsoType, 0, 0, 0, 0))
+			err := CheckValid(tc.pkt, NewHeader(0, tc.gsoType, 0, 100, 0, 0))
 			if tc.wantErr && err == nil {
 				t.Errorf("CheckValid(gsoType=%#x) = nil, want error", tc.gsoType)
 			}
@@ -470,6 +471,16 @@ func TestCheckValidMasksGSOECN(t *testing.T) {
 	}
 }
 
+// TestCheckValidRejectsZeroGSOSize: a GSO-typed header with gso_size=0 must be
+// rejected. It would produce a Packet whose GSOInfo.IsSuperpacket() is false,
+// dodging both segmentation and FinishChecksum on its way downstream.
+func TestCheckValidRejectsZeroGSOSize(t *testing.T) {
+	v4pkt, _, _ := buildTCPv4Super(100)
+	if err := CheckValid(v4pkt, NewHeader(0, unix.VIRTIO_NET_HDR_GSO_TCPV4, 0, 0, 0, 0)); err == nil {
+		t.Fatal("CheckValid accepted a GSO-typed header with gso_size=0")
+	}
+}
+
 // TestFoldComplementMatchesReference checks the segmenter's fold-and-invert
 // against an independent RFC 1071 reference fold, hitting the carry edge
 // cases (values whose first fold produces another carry).
@@ -502,14 +513,12 @@ func TestFoldComplementMatchesReference(t *testing.T) {
 // arithmetic, which is faster but far less obvious — particularly for the TCP
 // flags byte, which is only half of a 16-bit word. These references exist so
 // that trade is checked rather than asserted.
-func referenceBaseIPv4HdrSum(pkt []byte, ihl int, zeroID bool) uint32 {
+func referenceBaseIPv4HdrSum(pkt []byte, ihl int) uint32 {
 	var ipTmp [ipv4HeaderMaxLen]byte
 	copy(ipTmp[:ihl], pkt[:ihl])
 	ipTmp[ipv4TotalLenOff], ipTmp[ipv4TotalLenOff+1] = 0, 0
 	ipTmp[ipv4ChecksumOff], ipTmp[ipv4ChecksumOff+1] = 0, 0
-	if zeroID {
+	ipTmp[ipv4IDOff], ipTmp[ipv4IDOff+1] = 0, 0
-		ipTmp[ipv4IDOff], ipTmp[ipv4IDOff+1] = 0, 0
-	}
 	return uint32(checksum.Checksum(ipTmp[:ihl], 0))
 }
 
@@ -535,26 +544,24 @@ func TestBaseSumsMatchZeroingReference(t *testing.T) {
 
 	t.Run("ipv4", func(t *testing.T) {
 		for ihl := ipv4HeaderMinLen; ihl <= ipv4HeaderMaxLen; ihl += 4 {
-			for _, zeroID := range []bool{true, false} {
+			for iter := 0; iter < 5000; iter++ {
-				for iter := 0; iter < 5000; iter++ {
+				pkt := make([]byte, ihl)
-					pkt := make([]byte, ihl)
+				for i := range pkt {
-					for i := range pkt {
+					pkt[i] = randByte(&state)
-						pkt[i] = randByte(&state)
+				}
-					}
+				pkt[0] = byte(0x40 | (ihl / 4))
-					pkt[0] = byte(0x40 | (ihl / 4))
 
-					want := referenceBaseIPv4HdrSum(pkt, ihl, zeroID)
+				want := referenceBaseIPv4HdrSum(pkt, ihl)
-					got, err := baseIPv4HdrSum(pkt, ihl, zeroID)
+				got, err := baseIPv4HdrSum(pkt, ihl)
-					if err != nil {
+				if err != nil {
-						t.Fatalf("ihl=%d: %v", ihl, err)
+					t.Fatalf("ihl=%d: %v", ihl, err)
-					}
+				}
-					// Compare the value that reaches the wire: the raw partial
+				// Compare the value that reaches the wire: the raw partial
-					// sums may legally differ by one's-complement -0 vs +0.
+				// sums may legally differ by one's-complement -0 vs +0.
-					for _, tl := range []uint32{20, 1500, 65535} {
+				for _, tl := range []uint32{20, 1500, 65535} {
-						for _, id := range []uint32{0, 0x4242, 0xffff} {
+					for _, id := range []uint32{0, 0x4242, 0xffff} {
-							if a, b := foldComplement(want+tl+id), foldComplement(got+tl+id); a != b {
+						if a, b := foldComplement(want+tl+id), foldComplement(got+tl+id); a != b {
-								t.Fatalf("ihl=%d zeroID=%v tl=%d id=%d: %#04x != %#04x", ihl, zeroID, tl, id, a, b)
+							t.Fatalf("ihl=%d tl=%d id=%d: %#04x != %#04x", ihl, tl, id, a, b)
-							}
 						}
 					}
 				}