From a393d4819aa85f4d0ed3b0d657cd0a9d6171c035 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 8 Aug 2023 23:29:54 +0000 Subject: [PATCH] tun/netstack: fix race during `(*netTun).Close()` During `Close()`, it's possible for `WriteNotify()` to race a channel send with the channel close. See the race below: ``` ================== WARNING: DATA RACE Write at 0x00c000055450 by goroutine 18: runtime.closechan() /opt/hostedtoolcache/go/1.20.7/x64/src/runtime/chan.go:357 +0x0 golang.zx2c4.com/wireguard/tun/netstack.(*netTun).Close() /home/runner/go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.0-20230325221338-052af4a8072b/tun/netstack/tun.go:178 +0xde golang.zx2c4.com/wireguard/device.(*Device).Close() /home/runner/go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.0-20230325221338-052af4a8072b/device/device.go:379 +0x181 github.com/coder/wgtunnel/tunneld.(*API).Close() /home/runner/go/pkg/mod/github.com/coder/wgtunnel@v0.1.5/tunneld/tunneld.go:78 +0x64 github.com/coder/coder/coderd/devtunnel_test.newTunnelServer.func2() /home/runner/actions-runner/_work/coder/coder/coderd/devtunnel/tunnel_test.go:211 +0x30 testing.(*common).Cleanup.func1() /opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1150 +0x193 testing.(*common).runCleanup() /opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1328 +0x1e9 testing.tRunner.func2() /opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1570 +0x52 runtime.deferreturn() /opt/hostedtoolcache/go/1.20.7/x64/src/runtime/panic.go:476 +0x32 testing.(*T).Run.func1() /opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1629 +0x47 Previous read at 0x00c000055450 by goroutine 244: runtime.chansend() /opt/hostedtoolcache/go/1.20.7/x64/src/runtime/chan.go:160 +0x0 golang.zx2c4.com/wireguard/tun/netstack.(*netTun).WriteNotify() /home/runner/go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.0-20230325221338-052af4a8072b/tun/netstack/tun.go:165 +0xe6 gvisor.dev/gvisor/pkg/tcpip/link/channel.(*queue).Write() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/link/channel/channel.go:100 +0x183 gvisor.dev/gvisor/pkg/tcpip/link/channel.(*Endpoint).WritePackets() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/link/channel/channel.go:258 +0xb7 gvisor.dev/gvisor/pkg/tcpip/stack.(*delegatingQueueingDiscipline).WritePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/stack/nic.go:146 +0x97 gvisor.dev/gvisor/pkg/tcpip/stack.(*nic).writeRawPacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/stack/nic.go:392 +0x84 gvisor.dev/gvisor/pkg/tcpip/stack.(*nic).writePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/stack/nic.go:386 +0x59 gvisor.dev/gvisor/pkg/tcpip/stack.(*nic).WritePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/stack/nic.go:347 +0x22b gvisor.dev/gvisor/pkg/tcpip/network/ipv6.(*endpoint).writePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/network/ipv6/ipv6.go:878 +0x408 gvisor.dev/gvisor/pkg/tcpip/network/ipv6.(*endpoint).WritePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/network/ipv6/ipv6.go:829 +0x2d7 gvisor.dev/gvisor/pkg/tcpip/stack.(*Route).WritePacket() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/stack/route.go:495 +0xf8 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.sendTCP() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/connect.go:918 +0x3fb gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).sendTCP() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/connect.go:816 +0x199 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).sendRaw() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/connect.go:985 +0x53a gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*sender).sendSegmentFromPacketBuffer() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/snd.go:1686 +0x26d gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*sender).sendSegment() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/snd.go:1647 +0x386 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*sender).maybeSendSegment() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/snd.go:877 +0x704 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*sender).sendData() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/snd.go:981 +0x4fa gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).sendData() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/connect.go:1009 +0xc4 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).shutdownLocked() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/endpoint.go:2536 +0x44f gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).closeLocked() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/endpoint.go:1063 +0xa9 gvisor.dev/gvisor/pkg/tcpip/transport/tcp.(*endpoint).Close() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/transport/tcp/endpoint.go:1033 +0x5d gvisor.dev/gvisor/pkg/tcpip/adapters/gonet.(*TCPConn).Close() /home/runner/go/pkg/mod/github.com/coder/gvisor@v0.0.0-20230714132058-be2e4ac102c3/pkg/tcpip/adapters/gonet/gonet.go:417 +0x4a github.com/coder/wgtunnel/tunneld.(*tracingConnWrapper).Close() /home/runner/go/pkg/mod/github.com/coder/wgtunnel@v0.1.5/tunneld/tracing.go:39 +0x7d net/http.(*persistConn).closeLocked() /opt/hostedtoolcache/go/1.20.7/x64/src/net/http/transport.go:2732 +0x222 net/http.(*persistConn).readLoopPeekFailLocked() /opt/hostedtoolcache/go/1.20.7/x64/src/net/http/transport.go:2267 +0x344 net/http.(*persistConn).readLoop() /opt/hostedtoolcache/go/1.20.7/x64/src/net/http/transport.go:2111 +0x1029 net/http.(*Transport).dialConn.func5() /opt/hostedtoolcache/go/1.20.7/x64/src/net/http/transport.go:1765 +0x39 ``` Signed-off-by: Colin Adler --- tun/netstack/tun.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tun/netstack/tun.go b/tun/netstack/tun.go index 596cfcd8a..8c6c894de 100644 --- a/tun/netstack/tun.go +++ b/tun/netstack/tun.go @@ -19,6 +19,7 @@ import ( "regexp" "strconv" "strings" + "sync" "syscall" "time" @@ -42,7 +43,10 @@ import ( type netTun struct { ep *channel.Endpoint stack *stack.Stack + notifyHandle *channel.NotificationHandle events chan tun.Event + pktMu sync.RWMutex + pktClosed bool incomingPacket chan *bufferv2.View mtu int dnsServers []netip.Addr @@ -70,7 +74,7 @@ func CreateNetTUN(localAddresses, dnsServers []netip.Addr, mtu int) (tun.Device, if tcpipErr != nil { return nil, nil, fmt.Errorf("could not enable TCP SACK: %v", tcpipErr) } - dev.ep.AddNotify(dev) + dev.notifyHandle = dev.ep.AddNotify(dev) tcpipErr = dev.stack.CreateNIC(1, dev.ep) if tcpipErr != nil { return nil, nil, fmt.Errorf("CreateNIC: %v", tcpipErr) @@ -162,11 +166,16 @@ func (tun *netTun) WriteNotify() { view := pkt.ToView() pkt.DecRef() - tun.incomingPacket <- view + tun.pktMu.RLock() + if !tun.pktClosed { + tun.incomingPacket <- view + } + tun.pktMu.RUnlock() } func (tun *netTun) Close() error { tun.stack.RemoveNIC(1) + tun.ep.RemoveNotify(tun.notifyHandle) if tun.events != nil { close(tun.events) @@ -174,9 +183,10 @@ func (tun *netTun) Close() error { tun.ep.Close() - if tun.incomingPacket != nil { - close(tun.incomingPacket) - } + tun.pktMu.Lock() + tun.pktClosed = true + close(tun.incomingPacket) + tun.pktMu.Unlock() return nil }