Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tun: fix race during WriteNotify and Close #25

Open
wants to merge 1 commit into
base: tailscale
Choose a base branch
from
Open

tun: fix race during WriteNotify and Close #25

wants to merge 1 commit into from

Conversation

spikecurtis
Copy link

During Close(), it's possible for WriteNotify() to race a channel send with the channel close. We noticed while testing our https://github.com/coder/wgtunnel project with the go race detector.

==================
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

@kylecarbs
Copy link

Hey @jwhited 👋 Could someone from Tailscale take a look?

@spikecurtis
Copy link
Author

@bradfitz can you take a look?

@raggi
Copy link
Member

raggi commented Apr 29, 2024

i think we want to use stack.wait after stack.close to prevent this, or stack.destroy that does both

@spikecurtis
Copy link
Author

Not sure the full implications of that change, since at present the stack is never closed. I can try it out and see if anything breaks...

@spikecurtis
tun: fix race during WriteNotify and Close
a4cb23a 
Signed-off-by: Spike Curtis <spike@coder.com>
@spikecurtis
Copy link
Author

@raggi please take another look

@spikecurtis spikecurtis mentioned this pull request May 8, 2024
Merged
@spikecurtis spikecurtis mentioned this pull request May 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@spikecurtis @kylecarbs @raggi