The declaration of err in
nextByte, err := buffered.ReadByte
shadows the declaration of err in
op, err := buffered.ReadString('\n')
above. As a result, the assignments to err in
err = ipcErrorf(ipc.IpcErrorInvalid, "trailing character in UAPI get: %c", nextByte)
and in
err = device.IpcGetOperation(buffered.Writer)
do not modify the correct err variable.
Found by staticcheck.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Plenty more to go, but a start:
name old time/op new time/op delta
UAPIGet-4 6.37µs ± 2% 5.56µs ± 1% -12.70% (p=0.000 n=8+8)
name old alloc/op new alloc/op delta
UAPIGet-4 1.98kB ± 0% 1.22kB ± 0% -38.71% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
UAPIGet-4 42.0 ± 0% 35.0 ± 0% -16.67% (p=0.000 n=10+10)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Unify the handling of unexpected UAPI errors.
The comment that says "should never happen" is incorrect;
this could happen due to I/O errors. Correct it.
Change error message capitalization for consistency.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The goal of this change is to make the structure
of IpcSetOperation easier to follow.
IpcSetOperation contains a small state machine:
It starts by configuring the device,
then shifts to configuring one peer at a time.
Having the code all in one giant method obscured that structure.
Split out the parts into helper functions and encapsulate the peer state.
This makes the overall structure more apparent.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Expand IPCError to contain a wrapped error,
and add a helper to make constructing such errors easier.
Add a defer-based "log on returned error" to IpcSetOperation.
This lets us simplify all of the error return paths.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Until we depend on Go 1.16 (which isn't released yet), alias our own
variable to the private member of the net package. This will allow an
easy find replace to make this go away when we eventually switch to
1.16.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Now that we block when enqueueing to the decryption queue,
there is only one case in which we "drop" a inbound element,
when decryption fails.
We can use a simple, obvious, sync-free sentinel for that, elem.packet == nil.
Also, we can return the message buffer to the pool slightly later,
which further simplifies the code.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This is similar to commit e1fa1cc556,
but for the decryption channel.
It is an alternative fix to f9f655567930a4cd78d40fa4ba0d58503335ae6a.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Some users report seeing lines like:
> Routine: receive incoming IPv4 - stopped
Popping up unexpectedly. Let's sleep and try again before failing, and
also log the error, and perhaps we'll eventually understand this
situation better in future versions.
Because we have to distinguish between the socket being closed
explicitly and whatever error this is, we bump the module to require Go
1.16.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
It's possible for RoutineSequentialReceiver to try to lock an elem after
RoutineDecryption has exited. Before this meant we didn't then unlock
the elem, so the whole program deadlocked.
As well, it looks like the flush code (which is now potentially
unnecessary?) wasn't properly dropping the buffers for the
not-already-dropped case.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
These obviously don't perfectly capture real world performance,
in which syscalls and network links have a significant impact.
Nevertheless, they capture some of the internal performance factors,
and they're easy and convenient to work with.
Hat tip to Avery Pennarun for help designing the throughput benchmark.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
One of the first rules of WaitGroups is that you call wg.Add
outside of a goroutine, not inside it. Fix this embarrassing mistake.
This prevents an extremely rare race condition (2 per 100,000 runs)
which could occur when attempting to start a new peer
concurrently with shutting down a device.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This is particularly problematic on mobile,
where there is a fixed number of elements.
If most of them leak, it'll impact performance;
if all of them leak, the device will permanently deadlock.
I have a test that detects element leaks, which is how I found this one.
There are some remaining leaks that I have not yet tracked down,
but this is the most prominent by far.
I will commit the test when it passes reliably.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
bufio is not required.
strings.Builder is cheaper than bytes.Buffer for constructing strings.
io.Writer is more flexible than io.StringWriter,
and just as cheap (when used with io.WriteString).
Run gofmt.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This was shifted by 2 bytes when making persistent keepalive into a u32.
Fix it by placing it after the aligned region.
Fixes: e739ff7 ("device: fix persistent_keepalive_interval data races")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
People are actually hitting this condition, so make it uniform. Also,
change a printf into a println, to match the other conventions.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Found by the race detector and existing tests.
To avoid introducing a lock into this hot path,
calculate and cache whether any peers exist.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Access keypair.sendNonce atomically.
Eliminate one unnecessary initialization to zero.
Mutate handshake.lastSentHandshake with the mutex held.
Co-authored-by: David Anderson <danderson@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
They're called elem in most places.
Rename a few local variables to make it consistent.
This makes it easier to grep the code for things like elem.Drop.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This is a similar treatment to the handling of the encryption
channel found a few commits ago: Use the closing of the channel
to manage goroutine lifetime and shutdown.
It is considerably simpler because there is only a single writer.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The new test introduced in this commit used to deadlock about 1% of the time.
I believe that the deadlock occurs as follows:
* The test completes, calling device.Close.
* device.Close closes device.signals.stop.
* RoutineEncryption stops.
* The deferred function in RoutineEncryption drains device.queue.encryption.
* RoutineEncryption exits.
* A peer's RoutineNonce processes an element queued in peer.queue.nonce.
* RoutineNonce puts that element into the outbound and encryption queues.
* RoutineSequentialSender reads that elements from the outbound queue.
* It waits for that element to get Unlocked by RoutineEncryption.
* RoutineEncryption has already exited, so RoutineSequentialSender blocks forever.
* device.RemoveAllPeers calls peer.Stop on all peers.
* peer.Stop waits for peer.routines.stopping, which blocks forever.
Rather than attempt to add even more ordering to the already complex
centralized shutdown orchestration, this commit moves towards a
data-flow-oriented shutdown.
The device.queue.encryption gets closed when there will be no more writes to it.
All device.queue.encryption readers always read until the channel is closed and then exit.
We thus guarantee that any element that enters the encryption queue also exits it.
This removes the need for central control of the lifetime of RoutineEncryption,
removes the need to drain the encryption queue on shutdown, and simplifies RoutineEncryption.
This commit also fixes a data race. When RoutineSequentialSender
drains its queue on shutdown, it needs to lock the elem before operating on it,
just as the main body does.
The new test in this commit passed 50k iterations with the race detector enabled
and 150k iterations with the race detector disabled, with no failures.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Since we already have it packed into a uint64
in a known byte order, write it back out again
the same byte order instead of copying byte by byte.
This should also generate more efficient code,
because the compiler can do a single uint64 write,
instead of eight bounds checks and eight byte writes.
Due to a missed optimization, it actually generates a mishmash
of smaller writes: 1 byte, 4 bytes, 2 bytes, 1 byte.
This is https://golang.org/issue/41663.
The code is still better than before, and will get better yet
once that compiler bug gets fixed.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This makes it easier to work with configs in tests.
It'll see heavier use over upcoming commits;
this commit only adds the infrastructure.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Any io.Reader will do, and there are no performance concerns here.
This is technically backwards incompatible,
but it is very unlikely to break any existing code.
It is compatible with the existing uses in wireguard-{windows,android,apple}
and also will allow us to slightly simplify it if desired.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
