Development discussion of WireGuard
 help / color / mirror / Atom feed
From: lsc <lsc@lv6.tw>
To: wireguard@lists.zx2c4.com
Cc: jordan@tailscale.com, james@tailscale.com, Jason@zx2c4.com
Subject: wireguard-go: data race introduced in recent commit 9e2f386
Date: Thu, 23 Mar 2023 17:01:42 +0800	[thread overview]
Message-ID: <ac87f86f-6837-4e0e-ec34-1df35f52540e@lv6.tw> (raw)

Hi,

Not sure if this is the correct place for bug reports, hope it's okay :)

I found my wireguard-go server (built from current ToT 1417a47) always 
panicking when I ran speedtest on a client via the server. I rebuilt the 
server with data race detector (`go build -race`) and got the following 
trace:

==================
WARNING: DATA RACE
Read at 0x00c00043b2f8 by goroutine 49:
   golang.zx2c4.com/wireguard/conn.setSrcControl()
       /home/lsc36/wireguard-go/conn/sticky_linux.go:76 +0x8c
   golang.zx2c4.com/wireguard/conn.(*StdNetBind).send4()
       /home/lsc36/wireguard-go/conn/bind_std.go:346 +0x174
   golang.zx2c4.com/wireguard/conn.(*StdNetBind).Send()
       /home/lsc36/wireguard-go/conn/bind_std.go:332 +0x1d4
   golang.zx2c4.com/wireguard/device.(*Peer).SendBuffers()
       /home/lsc36/wireguard-go/device/peer.go:126 +0x18c
   golang.zx2c4.com/wireguard/device.(*Peer).RoutineSequentialSender()
       /home/lsc36/wireguard-go/device/send.go:519 +0x458
   golang.zx2c4.com/wireguard/device.(*Peer).Start.func2()
       /home/lsc36/wireguard-go/device/peer.go:198 +0x40

Previous write at 0x00c00043b2f8 by goroutine 47:
   golang.zx2c4.com/wireguard/conn.(*StdNetEndpoint).ClearSrc()
       /home/lsc36/wireguard-go/conn/bind_std.go:100 +0x48
   golang.zx2c4.com/wireguard/conn.getSrcFromControl()
       /home/lsc36/wireguard-go/conn/sticky_linux.go:18 +0x3c
   golang.zx2c4.com/wireguard/conn.(*StdNetBind).makeReceiveIPv4.func1()
       /home/lsc36/wireguard-go/conn/bind_std.go:232 +0x350
   golang.zx2c4.com/wireguard/device.(*Device).RoutineReceiveIncoming()
       /home/lsc36/wireguard-go/device/receive.go:107 +0x3ac
   golang.zx2c4.com/wireguard/device.(*Device).BindUpdate.func3()
       /home/lsc36/wireguard-go/device/device.go:530 +0x4c

Goroutine 49 (running) created at:
   golang.zx2c4.com/wireguard/device.(*Peer).Start()
       /home/lsc36/wireguard-go/device/peer.go:198 +0x300
   golang.zx2c4.com/wireguard/device.(*ipcSetPeer).handlePostConfig()
       /home/lsc36/wireguard-go/device/uapi.go:268 +0x164
   golang.zx2c4.com/wireguard/device.(*Device).IpcSetOperation()
       /home/lsc36/wireguard-go/device/uapi.go:160 +0x35c
   golang.zx2c4.com/wireguard/device.(*Device).IpcHandle()
       /home/lsc36/wireguard-go/device/uapi.go:428 +0x248
   main.main.func4.1()
       /home/lsc36/wireguard-go/main.go:245 +0x4c

Goroutine 47 (running) created at:
   golang.zx2c4.com/wireguard/device.(*Device).BindUpdate()
       /home/lsc36/wireguard-go/device/device.go:530 +0x4b8
   golang.zx2c4.com/wireguard/device.(*Device).handleDeviceLine()
       /home/lsc36/wireguard-go/device/uapi.go:223 +0x45c
   golang.zx2c4.com/wireguard/device.(*Device).IpcSetOperation()
       /home/lsc36/wireguard-go/device/uapi.go:183 +0x210
   golang.zx2c4.com/wireguard/device.(*Device).IpcHandle()
       /home/lsc36/wireguard-go/device/uapi.go:428 +0x248
   main.main.func4.1()
       /home/lsc36/wireguard-go/main.go:245 +0x4c
==================

The bug looks introduced by a recent commit 9e2f386 "conn, device, tun: 
implement vectorized I/O on Linux". Without much knowledge to the 
codebase, it seems to me that Endpoint is supposed to be guarded by the 
RWMutex embedded in Peer, yet the change introduced a write without 
locking the mutex (`ep.ClearSrc()` called from sticky_linux.go:18).

In addition, I found it weird that Endpoint objects are reused via 
`endpointPool`. The docstring suggests "Endpoints are immutable, so we 
can re-use them" but that doesn't seem true.

Added authors/reviewers of 9e2f386 to cc. Would you take a look?

Regards,
SC


             reply	other threads:[~2023-03-23  9:01 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-23  9:01 lsc [this message]
2023-03-23 17:00 ` Jordan Whited
2023-03-24  3:21   ` lsc
2023-03-24 13:44     ` Jason A. Donenfeld
2023-03-24 13:45       ` [PATCH wireguard-go] conn: fix StdNetEndpoint data race by dynamically allocating endpoints Jason A. Donenfeld

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ac87f86f-6837-4e0e-ec34-1df35f52540e@lv6.tw \
    --to=lsc@lv6.tw \
    --cc=Jason@zx2c4.com \
    --cc=james@tailscale.com \
    --cc=jordan@tailscale.com \
    --cc=wireguard@lists.zx2c4.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).