diff --git a/nogo.yaml b/nogo.yaml index 650ed5bbc..82c2264a4 100644 --- a/nogo.yaml +++ b/nogo.yaml @@ -253,3 +253,7 @@ analyzers: suppress: - "comment on exported type Translation" # Intentional. - "comment on exported type PinnedRange" # Intentional. + ST1016: # CheckReceiverNamesIdentical + internal: + exclude: + - pkg/tcpip/stack/packet_buffer.go # TODO(b/233086175): Remove. diff --git a/pkg/tcpip/link/channel/channel.go b/pkg/tcpip/link/channel/channel.go index 104a35964..1e2843bef 100644 --- a/pkg/tcpip/link/channel/channel.go +++ b/pkg/tcpip/link/channel/channel.go @@ -63,7 +63,7 @@ func (q *queue) Read() stack.PacketBufferPtr { case p := <-q.c: return p default: - return stack.PacketBufferPtr{} + return nil } } @@ -72,7 +72,7 @@ func (q *queue) ReadContext(ctx context.Context) stack.PacketBufferPtr { case pkt := <-q.c: return pkt case <-ctx.Done(): - return stack.PacketBufferPtr{} + return nil } } diff --git a/pkg/tcpip/link/qdisc/fifo/packet_buffer_circular_list.go b/pkg/tcpip/link/qdisc/fifo/packet_buffer_circular_list.go index 9c9661efb..462ec058b 100644 --- a/pkg/tcpip/link/qdisc/fifo/packet_buffer_circular_list.go +++ b/pkg/tcpip/link/qdisc/fifo/packet_buffer_circular_list.go @@ -71,10 +71,10 @@ func (pl *packetBufferCircularList) pushBack(pb stack.PacketBufferPtr) { //go:nosplit func (pl *packetBufferCircularList) removeFront() stack.PacketBufferPtr { if pl.isEmpty() { - return stack.PacketBufferPtr{} + return nil } ret := pl.pbs[pl.head] - pl.pbs[pl.head] = stack.PacketBufferPtr{} + pl.pbs[pl.head] = nil pl.head = (pl.head + 1) % len(pl.pbs) pl.size-- return ret diff --git a/pkg/tcpip/link/qdisc/fifo/qdisc_test.go b/pkg/tcpip/link/qdisc/fifo/qdisc_test.go index 024b64edf..ed0becf2f 100644 --- a/pkg/tcpip/link/qdisc/fifo/qdisc_test.go +++ b/pkg/tcpip/link/qdisc/fifo/qdisc_test.go @@ -83,7 +83,7 @@ func TestWriteRefusedAfterClosed(t *testing.T) { linkEp := fifo.New(nil, 1, 2) linkEp.Close() - err := linkEp.WritePacket(stack.PacketBufferPtr{}) + err := linkEp.WritePacket(nil) _, ok := err.(*tcpip.ErrClosedForSend) if !ok { t.Errorf("got err = %s, want %s", err, &tcpip.ErrClosedForSend{}) diff --git a/pkg/tcpip/network/internal/fragmentation/fragmentation.go b/pkg/tcpip/network/internal/fragmentation/fragmentation.go index 86695d73b..db24ac22f 100644 --- a/pkg/tcpip/network/internal/fragmentation/fragmentation.go +++ b/pkg/tcpip/network/internal/fragmentation/fragmentation.go @@ -158,25 +158,25 @@ func (f *Fragmentation) Process( id FragmentID, first, last uint16, more bool, proto uint8, pkt stack.PacketBufferPtr) ( stack.PacketBufferPtr, uint8, bool, error) { if first > last { - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("first=%d is greater than last=%d: %w", first, last, ErrInvalidArgs) + return nil, 0, false, fmt.Errorf("first=%d is greater than last=%d: %w", first, last, ErrInvalidArgs) } if first%f.blockSize != 0 { - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("first=%d is not a multiple of block size=%d: %w", first, f.blockSize, ErrInvalidArgs) + return nil, 0, false, fmt.Errorf("first=%d is not a multiple of block size=%d: %w", first, f.blockSize, ErrInvalidArgs) } fragmentSize := last - first + 1 if more && fragmentSize%f.blockSize != 0 { - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("fragment size=%d bytes is not a multiple of block size=%d on non-final fragment: %w", fragmentSize, f.blockSize, ErrInvalidArgs) + return nil, 0, false, fmt.Errorf("fragment size=%d bytes is not a multiple of block size=%d on non-final fragment: %w", fragmentSize, f.blockSize, ErrInvalidArgs) } if l := pkt.Data().Size(); l != int(fragmentSize) { - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("got fragment size=%d bytes not equal to the expected fragment size=%d bytes (first=%d last=%d): %w", l, fragmentSize, first, last, ErrInvalidArgs) + return nil, 0, false, fmt.Errorf("got fragment size=%d bytes not equal to the expected fragment size=%d bytes (first=%d last=%d): %w", l, fragmentSize, first, last, ErrInvalidArgs) } f.mu.Lock() if f.reassemblers == nil { - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("Release() called before fragmentation processing could finish") + return nil, 0, false, fmt.Errorf("Release() called before fragmentation processing could finish") } r, ok := f.reassemblers[id] @@ -201,7 +201,7 @@ func (f *Fragmentation) Process( f.mu.Lock() f.release(r, false /* timedOut */) f.mu.Unlock() - return stack.PacketBufferPtr{}, 0, false, fmt.Errorf("fragmentation processing error: %w", err) + return nil, 0, false, fmt.Errorf("fragmentation processing error: %w", err) } f.mu.Lock() f.memSize += memConsumed @@ -253,12 +253,12 @@ func (f *Fragmentation) release(r *reassembler, timedOut bool) { } if !r.pkt.IsNil() { r.pkt.DecRef() - r.pkt = stack.PacketBufferPtr{} + r.pkt = nil } for _, h := range r.holes { if !h.pkt.IsNil() { h.pkt.DecRef() - h.pkt = stack.PacketBufferPtr{} + h.pkt = nil } } r.holes = nil diff --git a/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go b/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go index 4a3fd6656..e801da23a 100644 --- a/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go +++ b/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go @@ -632,7 +632,7 @@ func TestTimeoutHandler(t *testing.T) { }, }, wantError: false, - wantPkt: stack.PacketBufferPtr{}, + wantPkt: nil, }, { name: "second pkt is ignored", @@ -664,7 +664,7 @@ func TestTimeoutHandler(t *testing.T) { }, }, wantError: true, - wantPkt: stack.PacketBufferPtr{}, + wantPkt: nil, }, } @@ -672,7 +672,7 @@ func TestTimeoutHandler(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - handler := &testTimeoutHandler{pkt: stack.PacketBufferPtr{}} + handler := &testTimeoutHandler{pkt: nil} f := NewFragmentation(minBlockSize, HighFragThreshold, LowFragThreshold, reassembleTimeout, &faketime.NullClock{}, handler) @@ -703,7 +703,7 @@ func TestTimeoutHandler(t *testing.T) { } func TestFragmentSurvivesReleaseJob(t *testing.T) { - handler := &testTimeoutHandler{pkt: stack.PacketBufferPtr{}} + handler := &testTimeoutHandler{pkt: nil} c := faketime.NewManualClock() f := NewFragmentation(minBlockSize, HighFragThreshold, LowFragThreshold, reassembleTimeout, c, handler) pkt := pkt(2, "01") diff --git a/pkg/tcpip/network/internal/fragmentation/reassembler.go b/pkg/tcpip/network/internal/fragmentation/reassembler.go index a9ba0587b..873e034f6 100644 --- a/pkg/tcpip/network/internal/fragmentation/reassembler.go +++ b/pkg/tcpip/network/internal/fragmentation/reassembler.go @@ -67,7 +67,7 @@ func (r *reassembler) process(first, last uint16, more bool, proto uint8, pkt st // A concurrent goroutine might have already reassembled // the packet and emptied the heap while this goroutine // was waiting on the mutex. We don't have to do anything in this case. - return stack.PacketBufferPtr{}, 0, false, 0, nil + return nil, 0, false, 0, nil } var holeFound bool @@ -91,12 +91,12 @@ func (r *reassembler) process(first, last uint16, more bool, proto uint8, pkt st // https://github.com/torvalds/linux/blob/38525c6/net/ipv4/inet_fragment.c#L349 if first < currentHole.first || currentHole.last < last { // Incoming fragment only partially fits in the free hole. - return stack.PacketBufferPtr{}, 0, false, 0, ErrFragmentOverlap + return nil, 0, false, 0, ErrFragmentOverlap } if !more { if !currentHole.final || currentHole.filled && currentHole.last != last { // We have another final fragment, which does not perfectly overlap. - return stack.PacketBufferPtr{}, 0, false, 0, ErrFragmentConflict + return nil, 0, false, 0, ErrFragmentConflict } } @@ -155,12 +155,12 @@ func (r *reassembler) process(first, last uint16, more bool, proto uint8, pkt st } if !holeFound { // Incoming fragment is beyond end. - return stack.PacketBufferPtr{}, 0, false, 0, ErrFragmentConflict + return nil, 0, false, 0, ErrFragmentConflict } // Check if all the holes have been filled and we are ready to reassemble. if r.filled < len(r.holes) { - return stack.PacketBufferPtr{}, 0, false, memConsumed, nil + return nil, 0, false, memConsumed, nil } sort.Slice(r.holes, func(i, j int) bool { diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go index 4843f60ca..c5c5563c5 100644 --- a/pkg/tcpip/network/ipv4/icmp.go +++ b/pkg/tcpip/network/ipv4/icmp.go @@ -361,8 +361,7 @@ func (e *endpoint) handleICMP(pkt stack.PacketBufferPtr) { // It's possible that a raw socket expects to receive this. e.dispatcher.DeliverTransportPacket(header.ICMPv4ProtocolNumber, pkt) - pkt = stack.PacketBufferPtr{} - _ = pkt // Suppress unused variable warning. + pkt = nil sent := e.stats.icmp.packetsSent if !e.protocol.allowICMPReply(header.ICMPv4EchoReply, header.ICMPv4UnusedCode) { diff --git a/pkg/tcpip/stack/BUILD b/pkg/tcpip/stack/BUILD index 8cddaf961..2ae482e38 100644 --- a/pkg/tcpip/stack/BUILD +++ b/pkg/tcpip/stack/BUILD @@ -179,7 +179,7 @@ go_template_instance( prefix = "packetBuffer", template = "//pkg/refs:refs_template", types = { - "T": "packetBuffer", + "T": "PacketBuffer", }, ) diff --git a/pkg/tcpip/stack/gro.go b/pkg/tcpip/stack/gro.go index acbb8f297..0ddbb5799 100644 --- a/pkg/tcpip/stack/gro.go +++ b/pkg/tcpip/stack/gro.go @@ -249,7 +249,7 @@ func (gb *groBucket) found(gd *groDispatcher, groPkt *groPacket, flushGROPkt boo // Add flags from the packet to the GRO packet. groPkt.tcpHdr.SetFlags(uint8(groPkt.tcpHdr.Flags() | (flags & (header.TCPFlagFin | header.TCPFlagPsh)))) - pkt = PacketBufferPtr{} + pkt = nil } // Flush if the packet isn't the same size as the previous packets or diff --git a/pkg/tcpip/stack/packet_buffer.go b/pkg/tcpip/stack/packet_buffer.go index 8cf77616b..9e8567c24 100644 --- a/pkg/tcpip/stack/packet_buffer.go +++ b/pkg/tcpip/stack/packet_buffer.go @@ -35,7 +35,7 @@ const ( var pkPool = sync.Pool{ New: func() any { - return &packetBuffer{} + return &PacketBuffer{} }, } @@ -59,14 +59,9 @@ type PacketBufferOptions struct { } // PacketBufferPtr is a pointer to a PacketBuffer. -// -// +stateify savable -type PacketBufferPtr struct { - // packetBuffer is the underlying packet buffer. - *packetBuffer -} +type PacketBufferPtr = *PacketBuffer -// A packetBuffer contains all the data of a network packet. +// A PacketBuffer contains all the data of a network packet. // // As a PacketBuffer traverses up the stack, it may be necessary to pass it to // multiple endpoints. @@ -108,7 +103,7 @@ type PacketBufferPtr struct { // starting offset of each header in `buf`. // // +stateify savable -type packetBuffer struct { +type PacketBuffer struct { _ sync.NoCopy packetBufferRefs @@ -178,7 +173,7 @@ type packetBuffer struct { // NewPacketBuffer creates a new PacketBuffer with opts. func NewPacketBuffer(opts PacketBufferOptions) PacketBufferPtr { - pk := pkPool.Get().(*packetBuffer) + pk := pkPool.Get().(*PacketBuffer) pk.reset() if opts.ReserveHeaderBytes != 0 { v := bufferv2.NewViewSize(opts.ReserveHeaderBytes) @@ -191,36 +186,31 @@ func NewPacketBuffer(opts PacketBufferOptions) PacketBufferPtr { pk.NetworkPacketInfo.IsForwardedPacket = opts.IsForwardedPacket pk.onRelease = opts.OnRelease pk.InitRefs() - return PacketBufferPtr{ - packetBuffer: pk, - } + return pk } // IncRef increments the PacketBuffer's refcount. func (pk PacketBufferPtr) IncRef() PacketBufferPtr { pk.packetBufferRefs.IncRef() - return PacketBufferPtr{ - packetBuffer: pk.packetBuffer, - } + return pk } // DecRef decrements the PacketBuffer's refcount. If the refcount is // decremented to zero, the PacketBuffer is returned to the PacketBuffer // pool. -func (pk *PacketBufferPtr) DecRef() { +func (pk PacketBufferPtr) DecRef() { pk.packetBufferRefs.DecRef(func() { if pk.onRelease != nil { pk.onRelease() } pk.buf.Release() - pkPool.Put(pk.packetBuffer) + pkPool.Put(pk) }) - pk.packetBuffer = nil } -func (pk *packetBuffer) reset() { - *pk = packetBuffer{} +func (pk PacketBufferPtr) reset() { + *pk = PacketBuffer{} } // ReservedHeaderBytes returns the number of bytes initially reserved for @@ -374,7 +364,7 @@ func (pk PacketBufferPtr) headerView(typ headerType) bufferv2.View { // Clone makes a semi-deep copy of pk. The underlying packet payload is // shared. Hence, no modifications is done to underlying packet payload. func (pk PacketBufferPtr) Clone() PacketBufferPtr { - newPk := pkPool.Get().(*packetBuffer) + newPk := pkPool.Get().(*PacketBuffer) newPk.reset() newPk.buf = pk.buf.Clone() newPk.reserved = pk.reserved @@ -394,9 +384,7 @@ func (pk PacketBufferPtr) Clone() PacketBufferPtr { newPk.NetworkPacketInfo = pk.NetworkPacketInfo newPk.tuple = pk.tuple newPk.InitRefs() - return PacketBufferPtr{ - packetBuffer: newPk, - } + return newPk } // ReserveHeaderBytes prepends reserved space for headers at the front @@ -429,16 +417,14 @@ func (pk PacketBufferPtr) Network() header.Network { // See PacketBuffer.Data for details about how a packet buffer holds an inbound // packet. func (pk PacketBufferPtr) CloneToInbound() PacketBufferPtr { - newPk := pkPool.Get().(*packetBuffer) + newPk := pkPool.Get().(*PacketBuffer) newPk.reset() newPk.buf = pk.buf.Clone() newPk.InitRefs() // Treat unfilled header portion as reserved. newPk.reserved = pk.AvailableHeaderBytes() newPk.tuple = pk.tuple - return PacketBufferPtr{ - packetBuffer: newPk, - } + return newPk } // DeepCopyForForwarding creates a deep copy of the packet buffer for @@ -476,7 +462,7 @@ func (pk PacketBufferPtr) DeepCopyForForwarding(reservedHeaderBytes int) PacketB // IsNil returns whether the pointer is logically nil. func (pk PacketBufferPtr) IsNil() bool { - return pk.packetBuffer == nil + return pk == nil } // headerInfo stores metadata about a header in a packet. diff --git a/pkg/tcpip/stack/packet_buffer_list.go b/pkg/tcpip/stack/packet_buffer_list.go index 6f09b804d..31107c3ba 100644 --- a/pkg/tcpip/stack/packet_buffer_list.go +++ b/pkg/tcpip/stack/packet_buffer_list.go @@ -37,7 +37,7 @@ func (pl *PacketBufferList) AsSlice() []PacketBufferPtr { func (pl *PacketBufferList) Reset() { for i, pb := range pl.pbs { pb.DecRef() - pl.pbs[i] = PacketBufferPtr{} + pl.pbs[i] = nil } pl.pbs = pl.pbs[:0] } diff --git a/pkg/tcpip/stack/packet_buffer_unsafe.go b/pkg/tcpip/stack/packet_buffer_unsafe.go index f0160ea87..ddfb80045 100644 --- a/pkg/tcpip/stack/packet_buffer_unsafe.go +++ b/pkg/tcpip/stack/packet_buffer_unsafe.go @@ -17,12 +17,12 @@ package stack import "unsafe" // PacketBufferStructSize is the minimal size of the packet buffer overhead. -const PacketBufferStructSize = int(unsafe.Sizeof(packetBuffer{})) +const PacketBufferStructSize = int(unsafe.Sizeof(PacketBuffer{})) // ID returns a unique ID for the underlying storage of the packet. // // Two PacketBufferPtrs have the same IDs if and only if they point to the same // location in memory. func (pk PacketBufferPtr) ID() uintptr { - return uintptr(unsafe.Pointer(pk.packetBuffer)) + return uintptr(unsafe.Pointer(pk)) } diff --git a/pkg/tcpip/transport/internal/network/endpoint.go b/pkg/tcpip/transport/internal/network/endpoint.go index fa062ea9d..49ab1d0c4 100644 --- a/pkg/tcpip/transport/internal/network/endpoint.go +++ b/pkg/tcpip/transport/internal/network/endpoint.go @@ -272,7 +272,7 @@ func (c *WriteContext) TryNewPacketBuffer(reserveHdrBytes int, data bufferv2.Buf defer e.sendBufferSizeInUseMu.Unlock() if !e.hasSendSpaceRLocked() { - return stack.PacketBufferPtr{} + return nil } // Note that we allow oversubscription - if there is any space at all in the diff --git a/pkg/tcpip/transport/tcp/segment.go b/pkg/tcpip/transport/tcp/segment.go index 1bb3a490b..976f79f32 100644 --- a/pkg/tcpip/transport/tcp/segment.go +++ b/pkg/tcpip/transport/tcp/segment.go @@ -194,6 +194,7 @@ func (s *segment) DecRef() { } } s.pkt.DecRef() + s.pkt = nil segmentPool.Put(s) }) }