Over time, other magicsock refactors have made Start effectively a
no-op, except that some other functions choose to panic if called
before Start.
Signed-off-by: David Anderson <danderson@tailscale.com>
We were returning an error almost, but not quite like errConnClosed in
a single codepath, which could still trip the panic on reconfig in the
test logic.
Signed-off-by: David Anderson <danderson@tailscale.com>
Our prod code doesn't eagerly handshake, because our disco layer enables
on-demand handshaking. Configuring both peers to eagerly handshake leads
to WireGuard handshake races that make TestTwoDevicePing flaky.
Signed-off-by: David Anderson <danderson@tailscale.com>
It only existed to override one test-only behavior with a
different test-only behavior, in both cases working around
an annoying feature of our CI environments. Instead, handle
that weirdness entirely in the test code, with a tweaked
TestOnlyPacketListener that gets injected.
Signed-off-by: David Anderson <danderson@tailscale.com>
The docstring said it was meant for use in tests, but it's specifically a
special codepath that is _only_ used in tests, so make the claim stronger.
Signed-off-by: David Anderson <danderson@tailscale.com>
Instead of using the legacy codepath, teach discoEndpoint to handle
peers that have a home DERP, but no disco key. We can still communicate
with them, but only over DERP.
Signed-off-by: David Anderson <danderson@tailscale.com>
This log is quite verbose, it was only to be left in for one
unstable build to help debug a user issue.
This reverts commit 1dd2552032.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
Intended to help in resolving customer issue with
DNS caching.
We currently exec `ipconfig /flushdns` from two
places:
- SetDNS(), which logs before invoking
- here in router_windows, which doesn't
We'd like to see a positive indication in logs that flushdns
is being run.
As this log is expected to be spammy, it is proposed to
leave this in just long enough to do an unstable 1.13.x build
and then revert it. They won't run an unsigned image that
I build.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
The number of peers we have will be pretty stable across time.
Allocate roughly the right slice size.
This reduces memory usage when there are many peers.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Two optimizations.
Use values instead of pointers.
We were using pointers to make track the "peer in progress" easier.
It's not too hard to do it manually, though.
Make two passes through the data, so that we can size our
return value accurately from the beginning.
This is cheap enough compared to the allocation,
which grows linearly in the number of peers,
that it is worth doing.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Still very much a prototype (hard-coded IPs, etc) but should be
non-invasive enough to submit at this point and iterate from here.
Updates #2589
Co-Author: David Crawshaw <crawshaw@tailscale.com>
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This is a simplified rate limiter geared for exactly our needs:
A fast, mono.Time-based rate limiter for use in tstun.
It was generated by stripping down the x/time/rate rate limiter
to just our needs and switching it to use mono.Time.
It removes one time.Now call per packet.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
magicsock makes multiple calls to Now per packet.
Move to mono.Now. Changing some of the calls to
use package mono has a cascading effect,
causing non-per-packet call sites to also switch.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
TCP was done in 662fbd4a09.
This does the same for UDP.
Tested by hand. Integration tests will have to come later. I'd wanted
to do it in this commit, but the SOCKS5 server needed for interop
testing between two userspace nodes doesn't yet support UDP and I
didn't want to invent some whole new userspace packet injection
interface at this point, as SOCKS seems like a better route, but
that's its own bug.
Fixes#2302
RELNOTE=netstack mode can now UDP relay to subnets
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Prep for #1591 which will need to make Linux's router react to changes
that the link monitor observes.
The router package already depended on the monitor package
transitively. Now it's explicit.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The fact that Hash returns a [sha256.Size]byte leaks details about
the underlying hash implementation. This could very well be any other
hashing algorithm with a possible different block size.
Abstract this implementation detail away by declaring an opaque type
that is comparable. While we are changing the signature of UpdateHash,
rename it to just Update to reduce stutter (e.g., deephash.Update).
Signed-off-by: Joe Tsai <joetsai@digital-static.net>
The earlier 2ba36c294b started listening
for ip rule changes and only cared about DELRULE events, buts its subscription
included all rule events, including new ones, which meant we were then
catching our own ip rule creations and logging about how they were unknown.
Stop that log spam.
Updates #1591
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
For debugging & working on #1591 where certain versions of systemd-networkd
delete Tailscale's ip rule entries.
Updates #1591
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
logBufWriter had no serialization.
It just so happens that none of its users currently ever log concurrently.
Make it safe for concurrent use.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
As Brad suggested, mem.RO allows for a lot of easy perf gains. There were also some smaller
changes outside of mem.RO, such as using hex.Decode instead of hex.DecodeString.
```
name old time/op new time/op delta
FromUAPI-8 14.7µs ± 3% 12.3µs ± 4% -16.58% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
FromUAPI-8 9.52kB ± 0% 7.04kB ± 0% -26.05% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
FromUAPI-8 77.0 ± 0% 29.0 ± 0% -62.34% (p=0.008 n=5+5)
```
Signed-off-by: julianknodt <julianknodt@gmail.com>
Adds a benchmark for FromUAPI in wgcfg.
It appears that it's not actually that slow, the main allocations are from the scanner and new
config.
Updates #1912.
Signed-off-by: julianknodt <julianknodt@gmail.com>
The DERPTestPort int meant two things before: which port to use, and
whether to disable TLS verification. Users would like to set the port
without disabling TLS, so break it into two options.
Updates #1264
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
For instance, ephemeral nodes with only IPv6 addresses can now
SOCKS5-dial out to names like "foo" and resolve foo's IPv6 address
rather than foo's IPv4 address and get a "no route"
(*tcpip.ErrNoRoute) error from netstack's dialer.
Per https://github.com/tailscale/tailscale/issues/2268#issuecomment-870027626
which is only part of the isuse.
Updates #2268
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
After allowing for custom DERP maps, it's convenient to be able to see their latency in
netcheck. This adds a query to the local tailscaled for the current DERPMap.
Updates #1264
Signed-off-by: julianknodt <julianknodt@gmail.com>
This uses a debug envvar to optionally disable filter logging rate
limits by setting the environment variable
TS_DEBUG_FILTER_RATE_LIMIT_LOGS to "all", and if it matches,
the code will effectively disable the limits on the log rate by
setting the limit to 1 millisecond. This should make sure that all
filter logs will be captured.
Signed-off-by: Christine Dodrill <xe@tailscale.com>
Unused so far, but eventually we'll want this for SOCKS5 UDP binds (we
currently only do TCP with SOCKS5), and also for #2102 for forwarding
MagicDNS upstream to Tailscale IPs over netstack.
Updates #2102
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The route creation for the `tun` device was augmented in #1469 but
didn't account for adding IPv4 vs. IPv6 routes. There are 2 primary
changes as a result:
* Ensure that either `-inet` or `-inet6` was used in the
[`route(8)`](https://man.openbsd.org/route) command
* Use either the `localAddr4` or `localAddr6` for the gateway argument
depending which destination network is being added
The basis for the approach is based on the implementation from
`router_userspace_bsd.go`, including the `inet()` helper function.
Fixes#2048
References #1469
Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
It is a bit faster.
But more importantly, it matches upstream byte-for-byte,
which ensures there'll be no corner cases in which we disagree.
name old time/op new time/op delta
SetPeers-8 3.58µs ± 0% 3.16µs ± 2% -11.74% (p=0.016 n=4+5)
name old alloc/op new alloc/op delta
SetPeers-8 2.53kB ± 0% 2.53kB ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
SetPeers-8 99.0 ± 0% 99.0 ± 0% ~ (all equal)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Pull in the latest version of wireguard-windows.
Switch to upstream wireguard-go.
This requires reverting all of our import paths.
Unfortunately, this has to happen at the same time.
The wireguard-go change is very low risk,
as that commit matches our fork almost exactly.
(The only changes are import paths, CI files, and a go.mod entry.)
So if there are issues as a result of this commit,
the first place to look is wireguard-windows changes.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We repeat many peers each time we call SetPeers.
Instead of constructing strings for them from scratch every time,
keep strings alive across iterations.
name old time/op new time/op delta
SetPeers-8 3.58µs ± 1% 2.41µs ± 1% -32.60% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
SetPeers-8 2.53kB ± 0% 1.30kB ± 0% -48.73% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
SetPeers-8 99.0 ± 0% 16.0 ± 0% -83.84% (p=0.000 n=10+10)
We could reduce alloc/op 12% and allocs/op 23% if strs had
type map[string]strCache instead of map[string]*strCache,
but that wipes out the execution time impact.
Given that re-use is the most common scenario, let's optimize for it.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Our wireguard-go fork used different values from upstream for
package device's memory limits on iOS.
This was the last blocker to removing our fork.
These values are now vars rather than consts for iOS.
c27ff9b9f6
Adjust them on startup to our preferred values.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
magicsock.Conn.ParseEndpoint requires a peer's public key,
disco key, and legacy ip/ports in order to do its job.
We currently accomplish that by:
* adding the public key in our wireguard-go fork
* encoding the disco key as magic hostname
* using a bespoke comma-separated encoding
It's a bit messy.
Instead, switch to something simpler: use a json-encoded struct
containing exactly the information we need, in the form we use it.
Our wireguard-go fork still adds the public key to the
address when it passes it to ParseEndpoint, but now the code
compensating for that is just a couple of simple, well-commented lines.
Once this commit is in, we can remove that part of the fork
and remove the compensating code.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The new code is ugly, but much faster and leaner.
name old time/op new time/op delta
SetPeers-8 7.81µs ± 1% 3.59µs ± 1% -54.04% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
SetPeers-8 7.68kB ± 0% 2.53kB ± 0% -67.08% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
SetPeers-8 237 ± 0% 99 ± 0% -58.23% (p=0.000 n=10+10)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Because it showed up on hello profiles.
Cycle through some moderate-sized sets of peers.
This should cover the "small tweaks to netmap"
and the "up/down cycle" cases.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Yes, it printed, but that was an implementation detail for hashing.
And coming optimization will make it print even less.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
On benchmark completion, we shut down the wgengine.
If we happen to poll for status during shutdown,
we get an "engine closing" error.
It doesn't hurt anything; ignore it.
Fixestailscale/corp#1776
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Without any synchronization here, the "first packet" callback can
be delayed indefinitely, while other work continues.
Since the callback starts the benchmark timer, this could skew results.
Worse, if the benchmark manages to complete before the benchmark timer begins,
it'll cause a data race with the benchmark shutdown performed by package testing.
That is what is reported in #1881.
This is a bit unfortunate, in that it means that users of TrafficGen have
to be careful to keep this callback speedy and lightweight and to avoid deadlocks.
Fixes#1881
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
It is possible to get multiple status callbacks from an Engine.
We need to wait for at least one from each Engine.
Without limiting to one per Engine,
wait.Wait can exit early or can panic due to a negative counter.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This reduces the speed with which these benchmarks exhaust their supply fds.
Not to zero unfortunately, but it's still helpful when doing long runs.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Fields rename only.
Part of the general effort to make our code agnostic about endpoint formatting.
It's just a name, but it will soon be a misleading one; be more generic.
Do this as a separate commit because it generates a lot of whitespace changes.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Upstream wireguard-go renamed the interface method
from CreateEndpoint to ParseEndpoint.
I missed some comments. Fix them.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
By using conn.NewDefaultBind, this test requires that our endpoints
be comprehensible to wireguard-go. Instead, use a no-op bind that
treats endpoints as opaque strings.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Legacy endpoints (addrSet) currently reconstruct their dst string when requested.
Instead, store the dst string we were given to begin with.
In addition to being simpler and cheaper, this makes less code
aware of how to interpret endpoint strings.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Prefer the error from the actual wireguard-go device method call,
not {To,From}UAPI, as those tend to be less interesting I/O errors.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
When wireguard-go's UAPI interface fails with an error, ReconfigDevice hangs.
Fix that by buffering the channel and closing the writer after the call.
The code now matches the corresponding code in DeviceConfig, where I got it right.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
It is unused, and has been since early Feb 2021 (Tailscale 1.6).
We can't get delete the DeviceOptions entirely yet;
first #1831 and #1839 need to go in, along with some wireguard-go changes.
Deleting this chunk of code now will make the later commits more clearly correct.
Pingers can now go too.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The earlier eb06ec172f fixed
the flaky SSH issue (tailscale/corp#1725) by making sure that packets
addressed to Tailscale IPs in hybrid netstack mode weren't delivered
to netstack, but another issue remained:
All traffic handled by netstack was also potentially being handled by
the host networking stack, as the filter hook returned "Accept", which
made it keep processing. This could lead to various random racey chaos
as a function of OS/firewalls/routes/etc.
Instead, once we inject into netstack, stop our caller's packet
processing.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Prior to wireguard-go using printf-style logging,
all wireguard-go logging occurred using format string "%s".
We fixed that but continued to use %s when we rewrote
peer identifiers into Tailscale style.
This commit removes that %sl, which makes rate limiting work correctly.
As a happy side-benefit, it should generate less garbage.
Instead of replacing all wireguard-go peer identifiers
that might occur anywhere in a fully formatted log string,
assume that they only come from args.
Check all args for things that look like *device.Peers
and replace them with appropriately reformatted strings.
There is a variety of ways that this could go wrong
(unusual format verbs or modifiers, peer identifiers
occurring as part of a larger printed object, future API changes),
but none of them occur now, are likely to be added,
or would be hard to work around if they did.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The "stop phrases" we use all occur in wireguard-go in the format string.
We can avoid doing a bunch of fmt.Sprintf work when they appear.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
For historical reasons, we ended up with two near-duplicate
copies of curve25519 key types, one in the wireguard-go module
(wgcfg) and one in the tailscale module (types/wgkey).
Then we moved wgcfg to the tailscale module.
We can now remove the wgcfg key type in favor of wgkey.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
One of the consequences of the bind refactoring in 6f23087175
is that attempting to bind an IPv6 socket will always
result in c.pconn6.pconn being non-nil.
If the bind fails, it'll be set to a placeholder packet conn
that blocks forever.
As a result, we can always run ReceiveIPv6 and health check it.
This removes IPv4/IPv6 asymmetry and also will allow health checks
to detect any IPv6 receive func failures.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
It must be an IP address; enforce that at the type level.
Suggested-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
We had two separate code paths for the initial UDP listener bind
and any subsequent rebinds.
IPv6 got left out of the rebind code.
Rather than duplicate it there, unify the two code paths.
Then improve the resulting code:
* Rebind had nested listen attempts to try the user-specified port first,
and then fall back to :0 if that failed. Convert that into a loop.
* Initial bind tried only the user-specified port.
Rebind tried the user-specified port and 0.
But there are actually three ports of interest:
The one the user specified, the most recent port in use, and 0.
We now try all three in order, as appropriate.
* In the extremely rare case in which binding to port 0 fails,
use a dummy net.PacketConn whose reads block until close.
This will keep the wireguard-go receive func goroutine alive.
As a pleasant side-effect of this, if we decide that
we need to resuscitate #1796, it will now be much easier.
Fixes#1799
Co-authored-by: David Anderson <danderson@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
Assume it'll stay at 0 forever, so hard-code it
and delete code conditional on it being non-0.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
It was set to context.Background by all callers, for the same reasons.
Set it locally instead, to simplify call sites.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The old implementation knew too much about how wireguard-go worked.
As a result, it missed genuine problems that occurred due to unrelated bugs.
This fourth attempt to fix the health checks takes a black box approach.
A receive func is healthy if one (or both) of these conditions holds:
* It is currently running and blocked.
* It has been executed recently.
The second condition is required because receive functions
are not continuously executing. wireguard-go calls them and then
processes their results before calling them again.
There is a theoretical false positive if wireguard-go go takes
longer than one minute to process the results of a receive func execution.
If that happens, we have other problems.
Updates #1790
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
They were not doing their job.
They need yet another conceptual re-think.
Start by clearing the decks.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
We had a long-standing bug in which our TUN events channel
was being received from simultaneously in two places.
The first is wireguard-go.
At wgengine/userspace.go:366, we pass e.tundev to wireguard-go,
which starts a goroutine (RoutineTUNEventReader)
that receives from that channel and uses events to adjust the MTU
and bring the device up/down.
At wgengine/userspace.go:374, we launch a goroutine that
receives from e.tundev, logs MTU changes, and triggers
state updates when up/down changes occur.
Events were getting delivered haphazardly between the two of them.
We don't really want wireguard-go to receive the up/down events;
we control the state of the device explicitly by calling device.Up.
And the userspace.go loop MTU logging duplicates logging that
wireguard-go does when it received MTU updates.
So this change splits the single TUN events channel into up/down
and other (aka MTU), and sends them to the parties that ought
to receive them.
I'm actually a bit surprised that this hasn't caused more visible trouble.
If a down event went to wireguard-go but the subsequent up event
went to userspace.go, we could end up with the wireguard-go device disappearing.
I believe that this may also (somewhat accidentally) be a fix for #1790.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The old decay-based one took a while to converge. This new one (based
very loosely on TCP BBR) seems to converge quickly on what seems to be
the best speed.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
This tries to generate traffic at a rate that will saturate the
receiver, without overdoing it, even in the event of packet loss. It's
unrealistically more aggressive than TCP (which will back off quickly
in case of packet loss) but less silly than a blind test that just
generates packets as fast as it can (which can cause all the CPU to be
absorbed by the transmitter, giving an incorrect impression of how much
capacity the total system has).
Initial indications are that a syscall about every 10 packets (TCP bulk
delivery) is roughly the same speed as sending every packet through a
channel. A syscall per packet is about 5x-10x slower than that.
The whole tailscale wireguard-go + magicsock + packet filter
combination is about 4x slower again, which is better than I thought
we'd do, but probably has room for improvement.
Note that in "full" tailscale, there is also a tundev read/write for
every packet, effectively doubling the syscall overhead per packet.
Given these numbers, it seems like read/write syscalls are only 25-40%
of the total CPU time used in tailscale proper, so we do have
significant non-syscall optimization work to do too.
Sample output:
$ GOMAXPROCS=2 go test -bench . -benchtime 5s ./cmd/tailbench
goos: linux
goarch: amd64
pkg: tailscale.com/cmd/tailbench
cpu: Intel(R) Core(TM) i7-4785T CPU @ 2.20GHz
BenchmarkTrivialNoAlloc/32-2 56340248 93.85 ns/op 340.98 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTrivialNoAlloc/124-2 57527490 99.27 ns/op 1249.10 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTrivialNoAlloc/1024-2 52537773 111.3 ns/op 9200.39 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTrivial/32-2 41878063 135.6 ns/op 236.04 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTrivial/124-2 41270439 138.4 ns/op 896.02 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTrivial/1024-2 36337252 154.3 ns/op 6635.30 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkBlockingChannel/32-2 12171654 494.3 ns/op 64.74 MB/s 0 %lost 1791 B/op 0 allocs/op
BenchmarkBlockingChannel/124-2 12149956 507.8 ns/op 244.17 MB/s 0 %lost 1792 B/op 1 allocs/op
BenchmarkBlockingChannel/1024-2 11034754 528.8 ns/op 1936.42 MB/s 0 %lost 1792 B/op 1 allocs/op
BenchmarkNonlockingChannel/32-2 8960622 2195 ns/op 14.58 MB/s 8.825 %lost 1792 B/op 1 allocs/op
BenchmarkNonlockingChannel/124-2 3014614 2224 ns/op 55.75 MB/s 11.18 %lost 1792 B/op 1 allocs/op
BenchmarkNonlockingChannel/1024-2 3234915 1688 ns/op 606.53 MB/s 3.765 %lost 1792 B/op 1 allocs/op
BenchmarkDoubleChannel/32-2 8457559 764.1 ns/op 41.88 MB/s 5.945 %lost 1792 B/op 1 allocs/op
BenchmarkDoubleChannel/124-2 5497726 1030 ns/op 120.38 MB/s 12.14 %lost 1792 B/op 1 allocs/op
BenchmarkDoubleChannel/1024-2 7985656 1360 ns/op 752.86 MB/s 13.57 %lost 1792 B/op 1 allocs/op
BenchmarkUDP/32-2 1652134 3695 ns/op 8.66 MB/s 0 %lost 176 B/op 3 allocs/op
BenchmarkUDP/124-2 1621024 3765 ns/op 32.94 MB/s 0 %lost 176 B/op 3 allocs/op
BenchmarkUDP/1024-2 1553750 3825 ns/op 267.72 MB/s 0 %lost 176 B/op 3 allocs/op
BenchmarkTCP/32-2 11056336 503.2 ns/op 63.60 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTCP/124-2 11074869 533.7 ns/op 232.32 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkTCP/1024-2 8934968 671.4 ns/op 1525.20 MB/s 0 %lost 0 B/op 0 allocs/op
BenchmarkWireGuardTest/32-2 1403702 4547 ns/op 7.04 MB/s 14.37 %lost 467 B/op 3 allocs/op
BenchmarkWireGuardTest/124-2 780645 7927 ns/op 15.64 MB/s 1.537 %lost 420 B/op 3 allocs/op
BenchmarkWireGuardTest/1024-2 512671 11791 ns/op 86.85 MB/s 0.5206 %lost 411 B/op 3 allocs/op
PASS
ok tailscale.com/wgengine/bench 195.724s
Updates #414.
Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>
The existing implementation was completely, embarrassingly conceptually broken.
We aren't able to see whether wireguard-go's receive function goroutines
are running or not. All we can do is model that based on what we have done.
This commit fixes that model.
Fixes#1781
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
Avery reported a sub-ms health transition from "receiveIPv4 not running" to "ok".
To avoid these transient false-positives, be more precise about
the expected lifetime of receive funcs. The problematic case is one in which
they were started but exited prior to a call to connBind.Close.
Explicitly represent started vs running state, taking care with the order of updates.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
The connection failure diagnostic code was never updated enough for
exit nodes, so disable its misleading output when the node it picks
(incorrectly) to diagnose is only an exit node.
Fixes#1754
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
I've spent two days searching for a theoretical wireguard-go bug
around receive functions exiting early.
I've found many bugs, but none of the flavor we're looking for.
Restore wireguard-go's logging around starting and stopping receive functions,
so that we can definitively rule in or out this particular theory.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
I see a bunch of these in some logs I'm looking at,
separated only by a few seconds.
Log the error so we can tell what's going on here.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
These were getting rate-limited for nodes with many peers.
Consolate the output into single lines, which are nicer anyway.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
We were accidentally logging oldPort -> oldPort.
Log oldPort as well as c.port; if we failed to get the preferred port
in a previous rebind, oldPort might differ from c.port.
Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
Track endpoints internally with a new tailcfg.Endpoint type that
includes a typed netaddr.IPPort (instead of just a string) and
includes a type for how that endpoint was discovered (STUN, local,
etc).
Use []tailcfg.Endpoint instead of []string internally.
At the last second, send it to the control server as the existing
[]string for endpoints, but also include a new parallel
MapRequest.EndpointType []tailcfg.EndpointType, so the control server
can start filtering out less-important endpoint changes from
new-enough clients. Notably, STUN-discovered endpoints can be filtered
out from 1.6+ clients, as they can discover them amongst each other
via CallMeMaybe disco exchanges started over DERP. And STUN endpoints
change a lot, causing a lot of MapResposne updates. But portmapped
endpoints are worth keeping for now, as they they work right away
without requiring the firewall traversal extra RTT dance.
End result will be less control->client bandwidth. (despite negligible
increase in client->control bandwidth)
Updates tailscale/corp#1543
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
On FreeBSD, we add the interface IP as a /48 to work around a kernel
bug, so we mustn't then try to add a /48 route to the Tailscale ULA,
since that will fail as a dupe.
Signed-off-by: David Anderson <danderson@tailscale.com>
It existed to work around the frequent opening and closing
of the conn.Bind done by wireguard-go.
The preceding commit removed that behavior,
so we can simply close the connections
when we are done with them.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We don't use the port that wireguard-go passes to us (via magicsock.connBind.Open).
We ignore it entirely and use the port we selected.
When we tell wireguard-go that we're changing the listen_port,
it calls connBind.Close and then connBind.Open.
And in the meantime, it stops calling the receive functions,
which means that we stop receiving and processing UDP and DERP packets.
And that is Very Bad.
That was never a problem prior to b3ceca1dd7,
because we passed the SkipBindUpdate flag to our wireguard-go fork,
which told wireguard-go not to re-bind on listen_port changes.
That commit eliminated the SkipBindUpdate flag.
We could write a bunch of code to work around the gap.
We could add background readers that process UDP and DERP packets when wireguard-go isn't.
But it's simpler to never create the conditions in which wireguard-go rebinds.
The other scenario in which wireguard-go re-binds is device.Down.
Conveniently, we never call device.Down. We go from device.Up to device.Close,
and the latter only when we're shutting down a magicsock.Conn completely.
Rubber-ducked-by: Avery Pennarun <apenwarr@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The shim implements both network and DNS configurators,
and feeds both into a single callback that receives
both configs.
Signed-off-by: David Anderson <danderson@tailscale.com>
Upstream wireguard-go has changed its receive model.
NewDevice now accepts a conn.Bind interface.
The conn.Bind is stateless; magicsock.Conns are stateful.
To work around this, we add a connBind type that supports
cheap teardown and bring-up, backed by a Conn.
The new conn.Bind allows us to specify a set of receive functions,
rather than having to shoehorn everything into ReceiveIPv4 and ReceiveIPv6.
This lets us plumbing DERP messages directly into wireguard-go,
instead of having to mux them via ReceiveIPv4.
One consequence of the new conn.Bind layer is that
closing the wireguard-go device is now indistinguishable
from the routine bring-up and tear-down normally experienced
by a conn.Bind. We thus have to explicitly close the magicsock.Conn
when the close the wireguard-go device.
One downside of this change is that we are reliant on wireguard-go
to call receiveDERP to process DERP messages. This is fine for now,
but is perhaps something we should fix in the future.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The code is not obviously better or worse, but this makes the little warning
triangle in my editor go away, and the distraction removal is worth it.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Google Cloud Run does not implement NETLINK_ROUTE RTMGRP.
If initialization of the netlink socket or group membership
fails, fall back to a polling implementation.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
The resolver still only supports a single upstream config, and
ipn/wgengine still have to split up the DNS config, but this moves
closer to unifying the DNS configs.
As a handy side-effect of the refactor, IPv6 MagicDNS records exist
now.
Signed-off-by: David Anderson <danderson@tailscale.com>
The call to appendEndpoint updates cpeer.Endpoints.
Then it is overwritten in the next line.
The only errors from appendEndpoint occur when
the host/port pair is malformed, but that cannot happen.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
It can end up executing an a new goroutine,
at which point instead of immediately stopping test execution, it hangs.
Since this is unexpected anyway, panic instead.
As a bonus, it makes call sites nicer and removes a kludge comment.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
So we have a documented & tested way to check whether we're in
netstack mode. To be used by future commits.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
For discovery when an explicit hostname/IP is known. We'll still
also send it via control for finding peers by a list.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
"Fake" doesn't mean a lot any more, given that many components
of the engine can be faked out, including in valid production
configurations like userspace-networking.
Signed-off-by: David Anderson <danderson@tailscale.com>
This makes setup more explicit in prod codepaths, without
requiring a bunch of arguments or helpers for tests and
userspace mode.
Signed-off-by: David Anderson <danderson@tailscale.com>
This works around the close syscall being slow.
We can revert this if we find a fix or if Apple makes close fast again.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The tstun packagen contains both constructors for generic tun
Devices, and a wrapper that provides additional functionality.
Signed-off-by: David Anderson <danderson@tailscale.com>
Now callers (wgengine/monitor) don't need to mutate the state to remove
boring interfaces before calling State.Equal. Instead, the methods
to remove boring interfaces from the State are removed, as is
the reflect-using Equal method itself, and in their place is
a new EqualFiltered method that takes a func predicate to match
interfaces to compare.
And then the FilterInteresting predicate is added for use
with EqualFiltered to do the job that that wgengine/monitor
previously wanted.
Now wgengine/monitor can keep the full interface state around,
including the "boring" interfaces, which we'll need for peerapi on
macOS/iOS to bind to the interface index of the utunN device.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
e.g.
$ tailscale ping 1.1.1.1
exit node found but not enabled
$ tailscale ping 10.2.200.2
node "tsbfvlan2" found, but not using its 10.2.200.0/24 route
$ sudo tailscale up --accept-routes
$ tailscale ping 10.2.200.2
pong from tsbfvlan2 (100.124.196.94) via 10.2.200.34:41641 in 1ms
$ tailscale ping mon.ts.tailscale.com
pong from monitoring (100.88.178.64) via DERP(sfo) in 83ms
pong from monitoring (100.88.178.64) via DERP(sfo) in 21ms
pong from monitoring (100.88.178.64) via [2604:a880:4:d1::37:d001]:41641 in 22ms
This necessarily moves code up from magicsock to wgengine, so we can
look at the actual wireguard config.
Fixes#1564
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Add proto to flowtrack.Tuple.
Add types/ipproto leaf package to break a cycle.
Server-side ACL work remains.
Updates #1516
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This reverts the revert commit 84aba349d9.
And changes us to use inet.af/netstack.
Updates #1518
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Should help iOS battery life on NEProvider.wake/skip events
with useless route updates that shouldn't cause re-STUNs.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
gVisor fixed their google/gvisor#1446 so we can include gVisor mode
on 32-bit machines.
A few minor upstream API changes, as normal.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
No server support yet, but we want Tailscale 1.6 clients to be able to respond
to them when the server can do it.
Updates #1310
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
There was a logical race where Conn.Rebind could acquire the
RebindingUDPConn mutex, close the connection, fail to rebind, release
the mutex, and then because the mutex was no longer held, ReceiveIPv4
wouldn't retry reads that failed with net.ErrClosed, letting that
error back to wireguard-go, which would then stop running that receive
IP goroutine.
Instead, keep the RebindingUDPConn mutex held for the entirety of the
replacement in all cases.
Updates tailscale/corp#1289
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
interfaces.State.String tries to print a concise summary of the
network state, removing any interfaces that don't have any or any
interesting IP addresses. On macOS and iOS, for instance, there are a
ton of misc things.
But the link monitor based its are-there-changes decision on
interfaces.State.Equal, which just used reflect.DeepEqual, including
comparing all the boring interfaces. On macOS, when turning wifi on or off, there
are a ton of misc boring interface changes, resulting in hitting an earlier
check I'd added on suspicion this was happening:
[unexpected] network state changed, but stringification didn't
This fixes that by instead adding a new
interfaces.State.RemoveUninterestingInterfacesAndAddresses method that
does, uh, that. Then use that in the monitor. So then when Equal is
used later, it's DeepEqualing the already-cleaned version with only
interesting interfaces.
This makes cmd/tailscaled debug --monitor much less noisy.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Also change the type to netaddr.IP while here, because it made sorting
easier.
Updates tailscale/corp#1397
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
The Engine.LinkChange method was recently removed in
e3df29d488 while misremembering how
Android's link state mechanism worked.
Rather than do some last minute rearchitecting of link state on
Android before Tailscale 1.6, restore the old Engine.LinkChange hook
for now so the Android client doesn't need any changes. But change how
it's implemented to instead inject an event into the link monitor.
Fixes#1427
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
FreeBSD tun devices don't work with the way we implement IPv6
https://github.com/tailscale/tailscale/issues/1307
At least for now, remove any IPv6 addresses from the netmap.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
This is necessary because either protocol can be disabled globally by a
Windows registry policy, at which point trying to touch that address
family results in "Element not found" errors. This change skips programming
address families that Windows tell us are unavailable.
Fixes#1396.
Signed-off-by: David Anderson <danderson@tailscale.com>
Not great, but lets people working on new ports get going more quickly
without having to do everything up front.
As the link monitor is getting used more, I felt bad having a useless
implementation.
Updates #815
Updates #1427
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
We used to allow that, but now it just crashes.
Separately I need to figure out why it got into this path at all,
which is #1416.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Tor has a location-hidden service feature that enables users to host services
from inside the Tor network. Each of these gets a unique DNS name that ends with
.onion. As it stands now, if a misbehaving application somehow manages to make
a .onion DNS request to our DNS server, we will forward that to the DNS server,
which could leak that to malicious third parties. See the recent bug Brave had
with this[1] for more context.
RFC 7686 suggests that name resolution APIs and libraries MUST respond with
NXDOMAIN unless they can actually handle Tor lookups. We can't handle .onion
lookups, so we reject them.
[1]: https://twitter.com/albinowax/status/1362737949872431108Fixestailscale/corp#1351
Signed-off-by: Christine Dodrill <xe@tailscale.com>
Don't use os.NewFile or (*os.File).Close on the AF_ROUTE socket. It
apparently does weird things to the fd and at least doesn't seem to
close it. Just use the unix package.
The test doesn't actually fail reliably before the fix, though. It
was an attempt. But this fixes the integration tests.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Gets it out of wgengine so the Engine isn't responsible for being a
callback registration hub for it.
This also removes the Engine.LinkChange method, as it's no longer
necessary. The monitor tells us about changes; it doesn't seem to
need any help. (Currently it was only used by Swift, but as of
14dc790137 we just do the same from Go)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
And add a --socks5-server flag.
And fix a race in SOCKS5 replies where the response header was written
concurrently with the copy from the backend.
Co-authored with Naman Sood.
Updates #707
Updates #504
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Previously tailscaled on macOS was running "/sbin/route monitor" as a
child process, but child processes aren't allowed in the Network
Extension / App Store sandbox. Instead, just do what "/sbin/route monitor"
itself does: unix.Socket(unix.AF_ROUTE, unix.SOCK_RAW, 0) and read that.
We also parse it now, but don't do anything with the parsed results yet.
We will over time, as we have with Linux netlink messages over time.
Currently any message is considered a signal to poll and see what changed.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Currently it assumes exactly 1 registered callback. This changes it to
support 0, 1, or more than 1.
This is a step towards plumbing wgengine/monitor into more places (and
moving some of wgengine's interface state fetching into monitor in a
later step)
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
UIs need to see the full unedited netmap in order to know what exit nodes they
can offer to the user.
Signed-off-by: David Anderson <danderson@tailscale.com>
* move probing out of netcheck into new net/portmapper package
* use PCP ANNOUNCE op codes for PCP discovery, rather than causing
short-lived (sub-second) side effects with a 1-second-expiring map +
delete.
* track when we heard things from the router so we can be less wasteful
in querying the router's port mapping services in the future
* use portmapper from magicsock to map a public port
Fixes#1298Fixes#1080Fixes#1001
Updates #864
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This makes cidrDiff do as much as possible before failing, and makes a
delete of an already-deleted rule be a no-op. We should never do this
ourselves, but other things on the system can, and this should help us
recover a bit.
Also adds the start of root-requiring tests.
TODO: hook into wgengine/monitor and notice when routes are changed
behind our back, and invalidate our routes map and re-read from
kernel (via the ip command) at least on the next reconfig call.
Updates tailscale/corp#1338
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Tangentially related to #987, #177, #594, #925, #505
Motivated by rebooting a launchd-controlled tailscaled and it going
into SetNetworkUp(false) mode immediately because there really is no
network up at system boot, but then it got stuck in that paused state
forever, without a monitor implementation.
When a handshake race occurs, a queued data packet can get lost.
TestTwoDevicePing expected that the very first data packet would arrive.
This caused occasional flakes.
Change TestTwoDevicePing to repeatedly re-send packets
and succeed when one of them makes it through.
This is acceptable (vs making WireGuard not drop the packets)
because this only affects communication with extremely old clients.
And those extremely old clients will eventually connect,
because the kernel will retry sends on timeout.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We modified the standard net package to not allocate a *net.UDPAddr
during a call to (*net.UDPConn).ReadFromUDP if the caller's use
of the *net.UDPAddr does not cause it to escape.
That is https://golang.org/cl/291390.
This is the companion change to magicsock.
There are two changes required.
First, call ReadFromUDP instead of ReadFrom, if possible.
ReadFrom returns a net.Addr, which is an interface, which always allocates.
Second, reduce the lifetime of the returned *net.UDPAddr.
We do this by immediately converting it into a netaddr.IPPort.
We left the existing RebindingUDPConn.ReadFrom method in place,
as it is required to satisfy the net.PacketConn interface.
With the upstream change and both of these fixes in place,
we have removed one large allocation per packet received.
name old time/op new time/op delta
ReceiveFrom-8 16.7µs ± 5% 16.4µs ± 8% ~ (p=0.310 n=5+5)
name old alloc/op new alloc/op delta
ReceiveFrom-8 112B ± 0% 64B ± 0% -42.86% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
ReceiveFrom-8 3.00 ± 0% 2.00 ± 0% -33.33% (p=0.008 n=5+5)
Co-authored-by: Sonia Appasamy <sonia@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
addrSet maintained duplicate lists of netaddr.IPPorts and net.UDPAddrs.
Unify to use the netaddr type only.
This makes (*Conn).ReceiveIPvN a bit uglier,
but that'll be cleaned up in a subsequent commit.
This is preparatory work to remove an allocation from ReceiveIPv4.
Co-authored-by: Sonia Appasamy <sonia@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
I based my estimation of the required timeout based on locally
observed behavior. But CI machines are worse than my local machine.
16s was enough to reduce flakiness but not eliminate it. Bump it up again.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
It only affects 'go install ./...', etc, and only on darwin/arm64 (M1 Macs) where
the go-ole package doesn't compile.
No need to build it.
Updates #943
This was in place because retrieved allowed_ips was very expensive.
Upstream changed the data structure to make them cheaper to compute.
This commit is an experiment to find out whether they're now cheap enough.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We removed the "fast retry" code from our wireguard-go fork.
As a result, pings can take longer to transit when retries are required.
Allow that.
Fixes#1277
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The fix can make this test run unconditionally.
This moves code from 5c619882bc for
testability but doesn't fix it yet. The #1282 problem remains (when I
wrote its wake-up mechanism, I forgot there were N DERP readers
funneling into 1 UDP reader, and the code just isn't correct at all
for that case).
Also factor out some test helper code from BenchmarkReceiveFrom.
The refactoring in magicsock.go for testability should have no
behavior change.
Magicsock started dropping all traffic internally when Tailscale is
shut down, to avoid spurious wireguard logspam. This made the benchmark
not receive anything. Setting a dummy private key is sufficient to get
magicsock to pass traffic for benchmarking purposes.
Fixes#1270.
Signed-off-by: David Anderson <danderson@tailscale.com>
And move a couple other types down into leafier packages.
Now cmd/tailscale doesn't bring in netlink, magicsock, wgengine, etc.
Fixes#1181
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Unused for now, but I want to backport this commit to 1.4 so 1.6 can
start sending these and then at least 1.4 logs will stringify nicely.
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Use tb.Cleanup to simplify both the API and the implementation.
One behavior change: When the number of goroutines shrinks, don't log.
I've never found these logs to be useful, and they frequently add noise.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Previously we disabled v6 support if the disable_policy knob was
missing in /proc, but some kernels support policy routing without
exposing the toggle. So instead, treat disable_policy absence as a
"maybe", and make the direct `ip -6 rule` probing a bit more
elaborate to compensate.
Fixes#1241.
Signed-off-by: David Anderson <danderson@tailscale.com>
This is mostly code movement from the wireguard-go repo.
Most of the new wgcfg package corresponds to the wireguard-go wgcfg package.
wgengine/wgcfg/device{_test}.go was device/config{_test}.go.
There were substantive but simple changes to device_test.go to remove
internal package device references.
The API of device.Config (now wgcfg.DeviceConfig) grew an error return;
we previously logged the error and threw it away.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
On Windows, configureInterface starts a goroutine reconfiguring the
Windows firewall.
But if configureInterface fails later, that goroutine kept running and
likely failing forever, spamming logs. Make it stop quietly if its
launching goroutine filed.
Rewrite log lines on the fly, based on the set of known peers.
This enables us to use upstream wireguard-go logging,
but maintain the Tailscale-style peer public key identifiers
that the rest of our systems (and people) expect.
Fixes#1183
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Also, don't try to use IPv6 LinkLocalUnicast addresses for now. Like endpoints
exchanged with control, we share them but don't yet use them.
Updates #1172
c8c493f3d9 made it always say
`created=false` which scared me when I saw it, as that would've implied
things were broken much worse. Fortunately the logging was just wrong.
DstToString is used in two places in wireguard-go: Logging and uapi.
We are switching to use uapi for wireguard-go config.
To preserve existing behavior, we need the full set of addrs.
And for logging, having the full set of addrs seems useful.
(The Addrs method itself is slated for removal. When that happens,
the implementation will move to DstToString.)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
To save CPU and wakeups, don't run the DERP cleanup timer regularly
unless there is a non-home DERP connection open.
Also eliminates the goroutine, moving to a time.AfterFunc.
Updates #1034
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This reverts commit 08baa17d9a.
It caused deadlocks due to lock ordering violations.
It was not the right fix, and thus should simply be reverted
while we look for the right fix (if we haven't already found it
in the interim; we've fixed other logging-after-test issues).
Fixes#1161
context.cancelCtx.Done involves a mutex and isn't as cheap as I
previously assumed. Convert the donec method into a struct field and
store the channel value once. Our one magicsock.Conn gets one pointer
larger, but it cuts ~1% of the CPU time of the ReceiveFrom benchmark
and removes a bubble from the --svg output :)
This test serves two purposes:
+ check that Write() returns an error if the tstun has been
closed.
+ ensure that the close-related code in tstun is exercised in
a test case. We were getting spurious code coverage adds/drops
based on timing of when the test case finished.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
22507adf54 stopped relying on
our fork of wireguard-go's UpdateDst callback.
As a result, we can unwind that code,
and the extra return value of ReceiveIPv{4,6}.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
TwoDevicePing is explicitly testing the behavior of the legacy codepath, everything
else is happy to assume that code no longer exists.
Signed-off-by: David Anderson <danderson@tailscale.com>
Previously, this benchmark relied on behavior of the legacy
receive codepath, which I changed in 22507adf. With this
change, the benchmark instead relies on the new active discovery
path.
Signed-off-by: David Anderson <danderson@tailscale.com>
This prevents us from continuing to do unnecessary work
(including logging) after the connection has closed.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This adds a new IP Protocol type, TSMP on protocol number 99 for
sending inter-tailscale messages over WireGuard, currently just for
why a peer rejects TCP SYNs (ACL rejection, shields up, and in the
future: nothing listening, something listening on that port but wrong
interface, etc)
Updates #1094
Updates tailscale/corp#1185
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Commit 68ddf1 removed code that reads
`SOFTWARE\Tailscale IPN\SearchList` registry value. But the commit
left code that writes that value.
So now this package writes and never reads the value.
Remove the code to stop pointless work.
Updates #853
Signed-off-by: Alex Brainman <alex.brainman@gmail.com>
This eliminates a dependency on wgcfg.Endpoint,
as part of the effort to eliminate our wireguard-go fork.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This makes connectivity between ancient and new tailscale nodes slightly
worse in some cases, but only in cases where the ancient version would
likely have failed to get connectivity anyway.
Signed-off-by: David Anderson <danderson@tailscale.com>
This is what every other DNS resolver I could find does, so tsdns
should do it to. This also helps avoid weird error messages about
non-existent records being unimplemented, and thus fixes#848.
Signed-off-by: Smitty <me@smitop.com>
In sendDiscoMessage there is a check of whether the connection is
closed, which is not being reliably exercised by other tests.
This shows up in code coverage reports, the lines of code in
sendDiscoMessage are alternately added and subtracted from
code coverage.
Add a test to specifically exercise and verify this code path.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
In derpWriteChanOfAddr when we call derphttp.NewRegionClient(),
there is a check of whether the connection is already errored and
if so it returns before grabbing the lock. The lock might already
be held and would be a deadlock.
This corner case is not being reliably exercised by other tests.
This shows up in code coverage reports, the lines of code in
derpWriteChanOfAddr are alternately added and subtracted from
code coverage.
Add a test to specifically exercise this code path, and verify that
it doesn't deadlock.
This is the best tradeoff I could come up with:
+ the moment code calls Err() to check if there is an error, we
grab the lock to make sure it would deadlock if it tries to grab
the lock itself.
+ if a new call to Err() is added in this code path, only the
first one will be covered and the rest will not be tested.
+ this test doesn't verify whether code is checking for Err() in
the right place, which ideally I guess it would.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
netaddr.IP no longer allocates, so don't need a cache or all its associated
code/complexity.
This totally removes groupcache/lru from the deps.
Also go mod tidy.
* wengine/netstack: bump gvisor to latest version
Signed-off-by: Naman Sood <naman@tailscale.com>
* update dependencies
Signed-off-by: Naman Sood <naman@tailscale.com>
* Don't change hardcoded IP
Signed-off-by: Naman Sood <naman@tailscale.com>
Not usefully functional yet (mostly a proof of concept), but getting
it submitted for some work @namansood is going to do atop this.
Updates #707
Updates #634
Updates #48
Updates #835
* show DNS name over hostname, removing domain's common MagicDNS suffix.
only show hostname if there's no DNS name.
but still show shared devices' MagicDNS FQDN.
* remove nerdy low-level details by default: endpoints, DERP relay,
public key. They're available in JSON mode still for those who need
them.
* only show endpoint or DERP relay when it's active with the goal of
making debugging easier. (so it's easier for users to understand
what's happening) The asterisks are gone.
* remove Tx/Rx numbers by default for idle peers; only show them when
there's traffic.
* include peers' owner login names
* add CLI option to not show peers (matching --self=true, --peers= also
defaults to true)
* sort by DNS/host name, not public key
* reorder columns
The log lines that wireguard-go prints as it starts
and stops its worker routines are mostly noise.
They also happen after other work is completed,
which causes failures in some of the log testing packages.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This appears to have been the intent of the previous code,
but in practice, it only returned A records.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
To be honest I'm not fond of Golden Bytes tests like this, but
not so much as to want to rewrite the whole test. The DNS byte
format is essentially immutable at this point, the encoded bytes
aren't going to change. The rest of the test assumptions about
hostnames might, but we can fix that when it comes.
Signed-off-by: Denton Gentry <dgentry@tailscale.com>
eccc167 introduced closeHandle which opened the handle,
but never closed it.
Windows handles should be closed.
Updates #921
Signed-off-by: Alex Brainman <alex.brainman@gmail.com>
Previously the client had heuristics to calculate which DNS search domains
to set, based on the peers' names. Unfortunately that prevented us from
doing some things we wanted to do server-side related to node sharing.
So, bump MapRequest.Version to 9 to signal that the client only uses the
explicitly configured DNS search domains and doesn't augment it with its own
list.
Updates tailscale/corp#1026
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This is a replacement for the key-related parts
of the wireguard-go wgcfg package.
This is almost a straight copy/paste from the wgcfg package.
I have slightly changed some of the exported functions and types
to avoid stutter, added and tweaked some comments,
and removed some now-unused code.
To avoid having wireguard-go depend on this new package,
wgcfg will keep its key types.
We translate into and out of those types at the last minute.
These few remaining uses will be eliminated alongside
the rest of the wgcfg package.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The windows key timeout is longer than the wgengine watchdog timeout,
which means we never reach the timeout, instead the process exits.
Reduce the timeout so if we do hit it, at least the process continues.
Signed-off-by: David Crawshaw <crawshaw@tailscale.com>
On Win10, there's a hardcoded GUID and this works.
On Win7, this GUID changes and we need to ask the tun for its
LUID and convert that from the GUID.
This commit uses the computed GUID that is placed in InterfaceName.
Diagnosed by Jason Donnenfeld. (Thanks!)
Lazy wg configuration now triggers if a peer has only endpoint
addresses (/32 for IPv4, /128 for IPv6). Subnet routers still
trigger eager configuration to avoid the need for a CIDR match
in the hot packet path.
Signed-off-by: David Anderson <danderson@tailscale.com>
The previous code used a lot of whole-function variables and shared
behavior that only triggered based on prior action from a single codepath.
Instead of that, move the small amounts of "shared" code into each switch
case.
Signed-off-by: David Anderson <danderson@tailscale.com>
Before, tailscaled would log every 10 seconds when the periodic noteRecvActivity
call happens. This is noisy, but worse it's misleading, because the message
suggests that the disco code is starting a lazy config run for a missing peer,
whereas in fact it's just an internal piece of keepalive logic.
With this change, we still log when going from 0->1 tunnel for the peer, but
not every 10s thereafter.
Signed-off-by: David Anderson <danderson@tailscale.com>
While the code was correct, I broke it during a refactoring and
tests didn't detect it. This fixes that glitch.
Signed-off-by: David Anderson <danderson@tailscale.com>
Doesn't materially affect benchmarks, but shrinks match6 by 30 instructions
and halves memory loads.
Part of #19.
Signed-off-by: David Anderson <danderson@tailscale.com>
Part of #19.
name old time/op new time/op delta
Filter/icmp4-8 32.2ns ± 3% 32.5ns ± 2% ~ (p=0.524 n=10+8)
Filter/icmp6-8 49.7ns ± 6% 43.1ns ± 4% -13.12% (p=0.000 n=9+10)
Signed-off-by: David Anderson <danderson@tailscale.com>
The packet filter still rejects all IPv6, but decodes enough from v6
packets to do something smarter in a followup.
name time/op
Decode/tcp4-8 28.8ns ± 2%
Decode/tcp6-8 20.6ns ± 1%
Decode/udp4-8 28.2ns ± 1%
Decode/udp6-8 20.0ns ± 6%
Decode/icmp4-8 21.7ns ± 2%
Decode/icmp6-8 14.1ns ± 2%
Decode/unknown-8 9.43ns ± 2%
Signed-off-by: David Anderson <danderson@tailscale.com>
In tests, we force binding to localhost to avoid OS firewall warning
dialogs.
But for IPv6, we were trying (and failing) to bind to 127.0.0.1.
You'd think we'd just say "localhost", but that's apparently ill
defined. See
https://tools.ietf.org/html/draft-ietf-dnsop-let-localhost-be-localhost
and golang/go#22826. (It's bitten me in the past, but I can't
remember specific bugs.)
So use "::1" explicitly for "udp6", which makes the test quieter.
The goal is to move some of the shenanigans we have elsewhere into the filter
package, so that all the weird things to do with poking at the filter is in
a single place, behind clean APIs.
Signed-off-by: David Anderson <danderson@tailscale.com>
We still use the packet.* alloc-free types in the data path, but
the compilation from netaddr to packet happens within the filter
package.
Signed-off-by: David Anderson <danderson@tailscale.com>
os.IsNotExist doesn't unwrap errors. errors.Is does.
The ioutil.ReadFile ones happened to be fine but I changed them so
we're consistent with the rule: if the error comes from os, you can
use os.IsNotExist, but from any other package, use errors.Is.
(errors.Is always would also work, but not worth updating all the code)
The motivation here was that we were logging about failure to migrate
legacy relay node prefs file on startup, even though the code tried
to avoid that.
See golang/go#41122
David Anderson