The keepalive deadlock fixed in #20 was one instance of a class the kernel
module rules out by construction. This commit adopts the same two rules
instead of working around the symptom.
1. Nothing reached from a timer callback may wait. In the kernel the
callback runs in softirq: alloc_skb is GFP_ATOMIC and gives up, and
wg_queue_enqueue_per_device_and_peer fails with -ENOSPC/-EPIPE and the
packet is dropped or marked PACKET_STATE_DEAD. #20 made the keepalive
allocation non-blocking but left the rest of the path waiting: a
container allocation on the nonce-overflow branch and the two bounded
queue sends in SendStagedPackets. Any of them, parked while holding
the timer's runningLock, wedges Timer.DelSync and with it Peer.Stop,
RemovePeer and Device.Close. SendKeepalive now runs
sendStagedPackets(wait=false): a full pool or queue drops the batch;
if the sequential sender already holds the batch it is emptied and
unlocked so the sender passes over it. The public SendStagedPackets
keeps waiting, so the TUN reader keeps its backpressure.
2. A peer holds at most MAX_STAGED_PACKETS (128) packets while waiting
for a handshake; wg_xmit drops the oldest before adding a batch. Our
staged queue was bounded in batches, not packets, and a GSO batch can
carry 128 packets, so one peer that never completes its handshake
could pin thousands of buffers and drain a capped pool for every other
peer. StagePackets now enforces the packet bound with a per-peer
counter, dropping the oldest batches first. That, not the handshake
retry policy, is how the kernel keeps a dead peer from hurting the
rest of the device.
With the bound in place the handshakeAttempts change from #20 is not
needed and is reverted: outbound traffic resets the counter again, as it
does upstream and in wg_packet_send_queued_handshake_initiation, and
SendHandshakeInitiation goes back to the upstream isRetry signature so
these lines no longer diverge on merges.
The revert also removes a problem that change introduced. It relied on
the give-up branch of expiredRetransmitHandshake to release a dead
peer's buffers, and that branch only runs after MaxTimerHandshakes
failed retries - about 90 seconds. Until then the pool stayed drained,
the TUN reader stayed parked and no peer on the device could send. And
once the branch had run, nothing reset the counter again while traffic
kept flowing: the next packet sent one initiation and re-armed the
retransmit timer, which found the counter already past the limit and
gave up after a single try, flushing the staged queue, deleting the
keepalive timer and logging "giving up" every five seconds for as long
as the peer was down and had traffic. Upstream and the kernel give each
burst of new traffic a full 90-second budget instead, and now so do we.
Tests: TestStagedSendKeepsHandshakeAttempts is replaced by
TestStagedSendResetsHandshakeAttempts; TestStagePacketsBoundedPerPeer
checks the packet bound and that dropped buffers return to the pool;
TestSendKeepaliveWithFullOutboundQueue wedges the sequential sender and
checks the keepalive still returns.
* device: add non-blocking pool accessors
TryGet, and the Device-level wrappers around it, report failure instead of
waiting on the pool's condition variable. Nothing calls them yet: this is a
pure addition so the change of behavior in the keepalive path lands on its own.
* device: never block on the buffer pool while sending a keepalive
SendKeepalive took buffers with the blocking pool accessors. It runs inside a
keepalive timer callback, which holds the timer's runningLock, and Timer.DelSync
waits on that lock. So on a Device whose pool is capped and exhausted -
PreallocatedBuffersPerPool non-zero, plus a peer that stages packets while it
waits for a handshake that never completes - the callback parks in WaitPool.Get
and Peer.Stop can never finish. Peer removal, and therefore Device.Close, are
stuck for the lifetime of the process, and the buffers those operations would
have freed are the ones the callback is waiting for.
Use the non-blocking accessors and skip the keepalive when the pool is dry.
* device: name the handshake-initiation flag after what it does
isRetry only ever decided whether handshakeAttempts is cleared; it said nothing
about the packet or the timers. Rename it to resetAttempts and invert every
call site accordingly. Behavior is unchanged: each caller keeps the same effect
on the counter it had before.
* device: let a peer give up on a handshake it keeps failing
expiredRetransmitHandshake gives up after MaxTimerHandshakes attempts, flushes
the staged queue and stops the keepalive timer. That is the mechanism that frees
the buffers held for a peer which never completes a handshake, and it never ran:
SendStagedPackets asked for a counter reset, so every packet aimed at the down
peer put the attempt count back to zero and, with traffic still arriving, the
limit was never reached.
Stop resetting from the data path. The counter is still cleared by
timersHandshakeComplete, timersStart and the rekey paths, which are the events
that actually mean the peer is reachable.
* device: cover the capped-pool keepalive deadlock
Two regression tests, both failing before the previous two commits:
SendKeepalive on a device whose pool is drained never returns, and
SendStagedPackets resets handshakeAttempts so the give-up-and-flush path is
never reached. Plus unit coverage for TryGet at capacity and on an uncapped
pool.
* device: reproduce the deadlock on a live device pair
A functional test next to the unit ones: two devices with a completed
handshake, a third peer that can never handshake, real traffic and real timers.
The shape matters and the first attempt at it did not reproduce anything. A
peer whose staged queue is full never allocates for a keepalive - SendKeepalive
returns early - so the peer that drains the pool is not the peer that gets
stuck. The victim is one with a live session and an empty staged queue: its
keepalive does allocate, parks in the exhausted pool holding the timer's
runningLock, and removing that peer then waits on DelSync forever.
Without the keepalive fix this fails with "removing a peer never completed"
after the 10s guard; with it the removal returns and the test takes ~2.5s. The
cap is lifted in a cleanup so a failing run can still close the devices instead
of hanging the package.
Close closes its channels unguarded, so a second call panics the process with
close of closed channel rather than failing the one caller that raced.
Nothing above this layer can promise a single call. Device.Close closes the
device it was handed and is reached both from an ordinary teardown and from
RoutineReadFromTUN, while embedders hold their own references and cannot
coordinate. Guarding per instance above the tun, as netbird does with a
sync.Once in its device filter, cannot stop two wrappers closing one tun.
TestNetTunCloseTwice panics without this change. The concurrent test covers the
teardown against RoutineReadFromTUN race, and the third checks the guard still
closes the channels rather than skipping teardown.
* device: send immediate handshake when endpoint address changes
When a peer's endpoint is changed via UAPI to a new, non-nil address
(e.g. an external signal learned a now-reachable address) while a
handshake retry cycle is still mid-RekeyTimeout, force a handshake
initiation right away instead of waiting out the remaining ~5s.
* device: avoid throttle hack when initiating handshake on endpoint change
Extract the initiation send into sendHandshakeInitiation() and have the
endpoint-change path call it directly with a real lastSentHandshake
timestamp, instead of back-dating lastSentHandshake to bypass the
RekeyTimeout throttle.
* device: only initiate handshake on endpoint change when no usable session
Guard the endpoint-change handshake with needsHandshake() so an address
change on a live, working session does not trigger an unnecessary rekey.
There is a possible deadlock in `device.Close()` when you try to close
the device very soon after its start. The problem is that two different
methods acquire the same locks in different order:
1. device.Close()
- device.ipcMutex.Lock()
- device.state.Lock()
2. device.changeState(deviceState)
- device.state.Lock()
- device.ipcMutex.Lock()
Reproducer:
func TestDevice_deadlock(t *testing.T) {
d := randDevice(t)
d.Close()
}
Problem:
$ go clean -testcache && go test -race -timeout 3s -run TestDevice_deadlock ./device | grep -A 10 sync.runtime_SemacquireMutex
sync.runtime_SemacquireMutex(0xc000117d20?, 0x94?, 0x0?)
/usr/local/opt/go/libexec/src/runtime/sema.go:77 +0x25
sync.(*Mutex).lockSlow(0xc000130518)
/usr/local/opt/go/libexec/src/sync/mutex.go:171 +0x213
sync.(*Mutex).Lock(0xc000130518)
/usr/local/opt/go/libexec/src/sync/mutex.go:90 +0x55
golang.zx2c4.com/wireguard/device.(*Device).Close(0xc000130500)
/Users/martin.basovnik/git/basovnik/wireguard-go/device/device.go:373 +0xb6
golang.zx2c4.com/wireguard/device.TestDevice_deadlock(0x0?)
/Users/martin.basovnik/git/basovnik/wireguard-go/device/device_test.go:480 +0x2c
testing.tRunner(0xc00014c000, 0x131d7b0)
--
sync.runtime_SemacquireMutex(0xc000130564?, 0x60?, 0xc000130548?)
/usr/local/opt/go/libexec/src/runtime/sema.go:77 +0x25
sync.(*Mutex).lockSlow(0xc000130750)
/usr/local/opt/go/libexec/src/sync/mutex.go:171 +0x213
sync.(*Mutex).Lock(0xc000130750)
/usr/local/opt/go/libexec/src/sync/mutex.go:90 +0x55
sync.(*RWMutex).Lock(0xc000130750)
/usr/local/opt/go/libexec/src/sync/rwmutex.go:147 +0x45
golang.zx2c4.com/wireguard/device.(*Device).upLocked(0xc000130500)
/Users/martin.basovnik/git/basovnik/wireguard-go/device/device.go:179 +0x72
golang.zx2c4.com/wireguard/device.(*Device).changeState(0xc000130500, 0x1)
Signed-off-by: Martin Basovnik <martin.basovnik@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Only bother updating the rxBytes counter once we've processed a whole
vector, since additions are atomic.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Peer.RoutineSequentialReceiver() deals with packet vectors and does not
need to perform timer and endpoint operations for every packet in a
given vector. Changing these per-packet operations to per-vector
improves throughput by as much as 10% in some environments.
Signed-off-by: Jordan Whited <jordan@tailscale.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Access to Peer.endpoint was previously synchronized by Peer.RWMutex.
This has now moved to Peer.endpoint.Mutex. Peer.SendBuffers() is now the
sole caller of Endpoint.ClearSrc(), which is signaled via a new bool,
Peer.endpoint.clearSrcOnTx. Previous Callers of Endpoint.ClearSrc() now
set this bool, primarily via peer.markEndpointSrcForClearing().
Peer.SetEndpointFromPacket() clears Peer.endpoint.clearSrcOnTx when an
updated conn.Endpoint is stored. This maintains the same event order as
before, i.e. a conn.Endpoint received after peer.endpoint.clearSrcOnTx
is set, but before the next Peer.SendBuffers() call results in the
latest conn.Endpoint source being used for the next packet transmission.
These changes result in throughput improvements for single flow,
parallel (-P n) flow, and bidirectional (--bidir) flow iperf3 TCP/UDP
tests as measured on both Linux and Windows. Latency under load improves
especially for high throughput Linux scenarios. These improvements are
likely realized on all platforms to some degree, as the changes are not
platform-specific.
Co-authored-by: James Tucker <james@tailscale.com>
Signed-off-by: James Tucker <james@tailscale.com>
Signed-off-by: Jordan Whited <jordan@tailscale.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Implement UDP GSO and GRO for the Linux tun.Device, which is made
possible by virtio extensions in the kernel's TUN driver starting in
v6.2.
secnetperf, a QUIC benchmark utility from microsoft/msquic@8e1eb1a, is
used to demonstrate the effect of this commit between two Linux
computers with i5-12400 CPUs. There is roughly ~13us of round trip
latency between them. secnetperf was invoked with the following command
line options:
-stats:1 -exec:maxtput -test:tput -download:10000 -timed:1 -encrypt:0
The first result is from commit 2e0774f without UDP GSO/GRO on the TUN.
[conn][0x55739a144980] STATS: EcnCapable=0 RTT=3973 us
SendTotalPackets=55859 SendSuspectedLostPackets=61
SendSpuriousLostPackets=59 SendCongestionCount=27
SendEcnCongestionCount=0 RecvTotalPackets=2779122
RecvReorderedPackets=0 RecvDroppedPackets=0
RecvDuplicatePackets=0 RecvDecryptionFailures=0
Result: 3654977571 bytes @ 2922821 kbps (10003.972 ms).
The second result is with UDP GSO/GRO on the TUN.
[conn][0x56493dfd09a0] STATS: EcnCapable=0 RTT=1216 us
SendTotalPackets=165033 SendSuspectedLostPackets=64
SendSpuriousLostPackets=61 SendCongestionCount=53
SendEcnCongestionCount=0 RecvTotalPackets=11845268
RecvReorderedPackets=25267 RecvDroppedPackets=0
RecvDuplicatePackets=0 RecvDecryptionFailures=0
Result: 15574671184 bytes @ 12458214 kbps (10001.222 ms).
Signed-off-by: Jordan Whited <jordan@tailscale.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
The length of a packet read from the underlying TUN device may exceed
the length of a supplied buffer when MTU exceeds device.MaxMessageSize.
Reviewed-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Signed-off-by: Jordan Whited <jordan@tailscale.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
GRO requires big allocations to be efficient. This isn't great, as there
might be Android memory usage issues. So we should revisit this commit.
But at least it gets things working again.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Zoltan Papp
Riccardo Manfrin
Maycon Santos
Viktor Liu
Viktor Liu
pascal-fischer
Martin Basovnik
Jason A. Donenfeld
Jordan Whited
