From 0b0e45582a0ef41e5f20ae3cebb747a47f23b91c Mon Sep 17 00:00:00 2001 From: JackDoan Date: Wed, 29 Jul 2026 14:33:20 -0500 Subject: [PATCH] small bugs --- overlay/tio/tio.go | 6 +++ overlay/tio/tio_gso_linux.go | 42 +++++++++------- overlay/tio/tio_poll_linux.go | 5 +- overlay/tio/tun_linux_offload_test.go | 9 ++-- overlay/tio/virtio/segment_linux.go | 26 ++++++---- overlay/tio/virtio/segment_linux_test.go | 61 +++++++++++++----------- 6 files changed, 89 insertions(+), 60 deletions(-) diff --git a/overlay/tio/tio.go b/overlay/tio/tio.go index 86ff5a34..3ebc21d6 100644 --- a/overlay/tio/tio.go +++ b/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 diff --git a/overlay/tio/tio_gso_linux.go b/overlay/tio/tio_gso_linux.go index 51244971..cce033ee 100644 --- a/overlay/tio/tio_gso_linux.go +++ b/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. - // That reader gets EBADF -> os.ErrClosed (or wakes via the shutdown eventfd's ppoll first). + // 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 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) } diff --git a/overlay/tio/tio_poll_linux.go b/overlay/tio/tio_poll_linux.go index 6966a7ae..8d09dc2d 100644 --- a/overlay/tio/tio_poll_linux.go +++ b/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. - // That reader gets EBADF -> os.ErrClosed (or wakes via the shutdown eventfd's ppoll first). + // 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 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) } diff --git a/overlay/tio/tun_linux_offload_test.go b/overlay/tio/tun_linux_offload_test.go index 26150809..36d927ae 100644 --- a/overlay/tio/tun_linux_offload_test.go +++ b/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 - // segment carries the same ID as the seed. + // Software UDP GSO bumps the IPv4 ID per segment exactly like TSO + // (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 { - t.Errorf("seg %d: ip id=%#x want %#x", i, id, 0x4242) + if id != 0x4242+uint16(i) { + 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) { diff --git a/overlay/tio/virtio/segment_linux.go b/overlay/tio/virtio/segment_linux.go index 6b0b7fab..c4e88313 100644 --- a/overlay/tio/virtio/segment_linux.go +++ b/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. - baseIPHdrSum, err = baseIPv4HdrSum(pkt, csumStart, false) + // Software UDP GSO bumps the ID per segment just like TSO + // (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)) diff --git a/overlay/tio/virtio/segment_linux_test.go b/overlay/tio/virtio/segment_linux_test.go index cbd9ddd4..e7d29997 100644 --- a/overlay/tio/virtio/segment_linux_test.go +++ b/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. - if id := binary.BigEndian.Uint16(seg[4:6]); id != 0x4242 { - t.Errorf("seg %d: ip id=%#x want 0x4242", i, id) + // Software UDP GSO bumps the IPv4 ID per segment just like TSO + // (inet_gso_segment's fixed-ID case is TCP-only). + 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++ { - pkt := make([]byte, ihl) - for i := range pkt { - pkt[i] = randByte(&state) - } - pkt[0] = byte(0x40 | (ihl / 4)) + for iter := 0; iter < 5000; iter++ { + pkt := make([]byte, ihl) + for i := range pkt { + pkt[i] = randByte(&state) + } + pkt[0] = byte(0x40 | (ihl / 4)) - want := referenceBaseIPv4HdrSum(pkt, ihl, zeroID) - got, err := baseIPv4HdrSum(pkt, ihl, zeroID) - if err != nil { - t.Fatalf("ihl=%d: %v", ihl, err) - } - // Compare the value that reaches the wire: the raw partial - // sums may legally differ by one's-complement -0 vs +0. - for _, tl := range []uint32{20, 1500, 65535} { - for _, id := range []uint32{0, 0x4242, 0xffff} { - 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) - } + want := referenceBaseIPv4HdrSum(pkt, ihl) + got, err := baseIPv4HdrSum(pkt, ihl) + if err != nil { + t.Fatalf("ihl=%d: %v", ihl, err) + } + // Compare the value that reaches the wire: the raw partial + // sums may legally differ by one's-complement -0 vs +0. + for _, tl := range []uint32{20, 1500, 65535} { + for _, id := range []uint32{0, 0x4242, 0xffff} { + if a, b := foldComplement(want+tl+id), foldComplement(got+tl+id); a != b { + t.Fatalf("ihl=%d tl=%d id=%d: %#04x != %#04x", ihl, tl, id, a, b) } } }