From afa323bd307095ca2a727084138ad108eb8a7e9e Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Thu, 12 Dec 2024 17:03:18 -0800 Subject: [PATCH] Replace most instances of IncRef with Clone. Incrementing the reference count of a packet as a means of granting ownership is unsafe when the packet is shared across gorountines. The underlying buffer's reference count is unchanged since it "technically" has the same owning PacketBuffer, which means different goroutines operating on the underlying buffer (and packet itself) race. Clones are roughly as fast as IncRefs because the PacketBuffers allocate from a pool and the underlying buffers are cloned with copy-on-write semantics. I've left IncRef in places where the original packet in obviously going out of scope at the end of the function or in some tests. Reported-by: syzbot+e026046f4bf8ad09ae1f@syzkaller.appspotmail.com Reported-by: syzbot+559365d6050db4b30e0f@syzkaller.appspotmail.com Reported-by: syzbot+63c78a2c88a5744c636b@syzkaller.appspotmail.com PiperOrigin-RevId: 705676806 --- pkg/tcpip/link/channel/channel.go | 5 +++-- pkg/tcpip/link/fdbased/endpoint_test.go | 6 ++---- pkg/tcpip/link/fdbased/processors.go | 3 +-- pkg/tcpip/link/veth/veth_test.go | 3 +-- .../network/internal/fragmentation/fragmentation_test.go | 9 ++++++++- pkg/tcpip/network/internal/fragmentation/reassembler.go | 4 ++-- pkg/tcpip/network/internal/testutil/testutil.go | 2 +- pkg/tcpip/stack/forwarding_test.go | 2 +- pkg/tcpip/stack/packet_buffer.go | 1 + pkg/tcpip/stack/pending_packets.go | 2 +- pkg/tcpip/transport/udp/forwarder.go | 2 +- 11 files changed, 22 insertions(+), 17 deletions(-) diff --git a/pkg/tcpip/link/channel/channel.go b/pkg/tcpip/link/channel/channel.go index f08d864e8..738f03dfc 100644 --- a/pkg/tcpip/link/channel/channel.go +++ b/pkg/tcpip/link/channel/channel.go @@ -86,11 +86,12 @@ func (q *queue) Write(pkt *stack.PacketBuffer) tcpip.Error { } wrote := false + p := pkt.Clone() select { - case q.c <- pkt.IncRef(): + case q.c <- p: wrote = true default: - pkt.DecRef() + p.DecRef() } notify := q.notify q.mu.RUnlock() diff --git a/pkg/tcpip/link/fdbased/endpoint_test.go b/pkg/tcpip/link/fdbased/endpoint_test.go index cb4fea915..d263cc2e6 100644 --- a/pkg/tcpip/link/fdbased/endpoint_test.go +++ b/pkg/tcpip/link/fdbased/endpoint_test.go @@ -133,8 +133,7 @@ func (c *testContext) cleanup() { } func (c *testContext) DeliverNetworkPacket(protocol tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) { - pkt.IncRef() - c.ch <- packetInfo{protocol, pkt} + c.ch <- packetInfo{protocol, pkt.Clone()} } func (c *testContext) DeliverLinkPacket(tcpip.NetworkProtocolNumber, *stack.PacketBuffer) { @@ -597,8 +596,7 @@ type fakeNetworkDispatcher struct { } func (d *fakeNetworkDispatcher) DeliverNetworkPacket(_ tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) { - pkt.IncRef() - d.pkts = append(d.pkts, pkt) + d.pkts = append(d.pkts, pkt.Clone()) } func (*fakeNetworkDispatcher) DeliverLinkPacket(tcpip.NetworkProtocolNumber, *stack.PacketBuffer) { diff --git a/pkg/tcpip/link/fdbased/processors.go b/pkg/tcpip/link/fdbased/processors.go index fbb57bf83..6ffb2b10f 100644 --- a/pkg/tcpip/link/fdbased/processors.go +++ b/pkg/tcpip/link/fdbased/processors.go @@ -167,8 +167,7 @@ func (m *processorManager) queuePacket(pkt *stack.PacketBuffer, hasEthHeader boo p := &m.processors[pIdx] p.mu.Lock() defer p.mu.Unlock() - pkt.IncRef() - p.pkts.PushBack(pkt) + p.pkts.PushBack(pkt.IncRef()) m.ready[pIdx] = true } diff --git a/pkg/tcpip/link/veth/veth_test.go b/pkg/tcpip/link/veth/veth_test.go index 985c9e8d2..ff42753f5 100644 --- a/pkg/tcpip/link/veth/veth_test.go +++ b/pkg/tcpip/link/veth/veth_test.go @@ -49,8 +49,7 @@ type testNetworkDispatcher struct { } func (d *testNetworkDispatcher) DeliverNetworkPacket(_ tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) { - pkt.IncRef() - d.ch <- pkt + d.ch <- pkt.Clone() d.wg.Wait() } diff --git a/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go b/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go index 4dc5ddfd5..5287f1bf4 100644 --- a/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go +++ b/pkg/tcpip/network/internal/fragmentation/fragmentation_test.go @@ -582,7 +582,9 @@ type testTimeoutHandler struct { } func (h *testTimeoutHandler) OnReassemblyTimeout(pkt *stack.PacketBuffer) { - h.pkt = pkt + if pkt != nil { + h.pkt = pkt.Clone() + } } func TestTimeoutHandler(t *testing.T) { @@ -673,6 +675,11 @@ func TestTimeoutHandler(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { handler := &testTimeoutHandler{pkt: nil} + defer func() { + if handler.pkt != nil { + handler.pkt.DecRef() + } + }() f := NewFragmentation(minBlockSize, HighFragThreshold, LowFragThreshold, reassembleTimeout, &faketime.NullClock{}, handler) diff --git a/pkg/tcpip/network/internal/fragmentation/reassembler.go b/pkg/tcpip/network/internal/fragmentation/reassembler.go index 9aaad7632..233f43a00 100644 --- a/pkg/tcpip/network/internal/fragmentation/reassembler.go +++ b/pkg/tcpip/network/internal/fragmentation/reassembler.go @@ -135,7 +135,7 @@ func (r *reassembler) process(first, last uint16, more bool, proto uint8, pkt *s last: last, filled: true, final: currentHole.final, - pkt: pkt.IncRef(), + pkt: pkt.Clone(), } r.filled++ // For IPv6, it is possible to have different Protocol values between @@ -150,7 +150,7 @@ func (r *reassembler) process(first, last uint16, more bool, proto uint8, pkt *s if r.pkt != nil { r.pkt.DecRef() } - r.pkt = pkt.IncRef() + r.pkt = pkt.Clone() r.proto = proto } break diff --git a/pkg/tcpip/network/internal/testutil/testutil.go b/pkg/tcpip/network/internal/testutil/testutil.go index 2ac2db8aa..3e8ab794b 100644 --- a/pkg/tcpip/network/internal/testutil/testutil.go +++ b/pkg/tcpip/network/internal/testutil/testutil.go @@ -79,7 +79,7 @@ func (ep *MockLinkEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpi return n, ep.err } ep.allowPackets-- - ep.WrittenPackets = append(ep.WrittenPackets, pkt.IncRef()) + ep.WrittenPackets = append(ep.WrittenPackets, pkt.Clone()) n++ } return n, nil diff --git a/pkg/tcpip/stack/forwarding_test.go b/pkg/tcpip/stack/forwarding_test.go index 1d05197c6..9bc710390 100644 --- a/pkg/tcpip/stack/forwarding_test.go +++ b/pkg/tcpip/stack/forwarding_test.go @@ -319,7 +319,7 @@ func (e *fwdTestLinkEndpoint) WritePackets(pkts PacketBufferList) (int, tcpip.Er n := 0 for _, pkt := range pkts.AsSlice() { select { - case e.C <- pkt: + case e.C <- pkt.IncRef(): default: } diff --git a/pkg/tcpip/stack/packet_buffer.go b/pkg/tcpip/stack/packet_buffer.go index 24956e71b..a26669a14 100644 --- a/pkg/tcpip/stack/packet_buffer.go +++ b/pkg/tcpip/stack/packet_buffer.go @@ -381,6 +381,7 @@ func (pk *PacketBuffer) Clone() *PacketBuffer { newPk.Hash = pk.Hash newPk.Owner = pk.Owner newPk.GSOOptions = pk.GSOOptions + newPk.EgressRoute = pk.EgressRoute newPk.NetworkProtocolNumber = pk.NetworkProtocolNumber newPk.dnatDone = pk.dnatDone newPk.snatDone = pk.snatDone diff --git a/pkg/tcpip/stack/pending_packets.go b/pkg/tcpip/stack/pending_packets.go index 8b68916c6..03c81d056 100644 --- a/pkg/tcpip/stack/pending_packets.go +++ b/pkg/tcpip/stack/pending_packets.go @@ -149,7 +149,7 @@ func (f *packetsPendingLinkResolution) enqueue(r *Route, pkt *PacketBuffer) tcpi packets, ok := f.mu.packets[ch] packets = append(packets, pendingPacket{ routeInfo: routeInfo, - pkt: pkt.IncRef(), + pkt: pkt.Clone(), }) if len(packets) > maxPendingPacketsPerResolution { diff --git a/pkg/tcpip/transport/udp/forwarder.go b/pkg/tcpip/transport/udp/forwarder.go index d702c54ad..e439153ca 100644 --- a/pkg/tcpip/transport/udp/forwarder.go +++ b/pkg/tcpip/transport/udp/forwarder.go @@ -47,7 +47,7 @@ func (f *Forwarder) HandlePacket(id stack.TransportEndpointID, pkt *stack.Packet f.handler(&ForwarderRequest{ stack: f.stack, id: id, - pkt: pkt.IncRef(), + pkt: pkt.Clone(), }) return true