TheArchive/tailscale
1
0
Fork 0
mirror of https://github.com/tailscale/tailscale.git synced 2025-02-21 12:28:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

wgengine/magicsock: fix destination selection logic to work with DERP.

Browse Source 
The effect is subtle: when we're not spraying packets, and have not yet
figured out a curAddr, and we're not spraying, we end up sending to
whatever the first IP is in the iteration order. In English, that
means "when we have no idea where to send packets, and we've given
up on sending to everyone, just send to the first addr we see in
the list."

This is, in general, what we want, because the addrs are in sorted
preference order, low to high, and DERP is the least preferred
destination. So, when we have no idea where to send, send to DERP,
right?

... Except for very historical reasons, appendDests iterated through
addresses in _reverse_ order, most preferred to least preferred.
crawshaw@ believes this was part of the earliest handshaking
algorithm magicsock had, where it slowly iterated through possible
destinations and poked handshakes to them one at a time.

Anyway, because of this historical reverse iteration, in the case
described above of "we have no idea where to send", the code would
end up sending to the _most_ preferred candidate address, rather
than the _least_ preferred. So when in doubt, we'd end up firing
packets into the blackhole of some LAN address that doesn't work,
and connectivity would not work.

This case only comes up if all your non-DERP connectivity options
have failed, so we more or less failed to detect it because we
didn't have a pathological test box deployed. Worse, codependent
bug 2839854994f204a9e95e4d8d410490bb4f25e1fe made DERP accidentally
work sometimes anyway by incorrectly exploiting roamAddr behavior,
albeit at the cost of making DERP traffic symmetric. In fixing
DERP to once again be asymmetric, we effectively removed the
bandaid that was concealing this bug.

Signed-Off-By: David Anderson <danderson@tailscale.com>
This commit is contained in:
David Anderson 2020-03-05 18:57:45 -08:00 committed by Dave Anderson
parent 97e58ad44d
commit 05a52746a4
1 changed files with 26 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
wgengine/magicsock/magicsock.go
View File

@ -526,21 +526,37 @@ func appendDests(dsts []*net.UDPAddr, as *AddrSet, b []byte) (_ []*net.UDPAddr,
// Pick our destination address(es).
roamAddr = as.roamAddr
if roamAddr != nil {
dsts = append(dsts, roamAddr)
if !spray {
return dsts, roamAddr
switch {
case spray:
// This packet is being sprayed to all addresses.
for i := range as.addrs {
dsts = append(dsts, &as.addrs[i])
}
}
for i := len(as.addrs) - 1; i >= 0; i-- {
addr := &as.addrs[i]
if spray || as.curAddr == -1 || as.curAddr == i {
dsts = append(dsts, addr)
if as.roamAddr != nil {
dsts = append(dsts, as.roamAddr)
}
if !spray && len(dsts) != 0 {
case as.roamAddr != nil:
// We have a roaming address, prefer it over other addrs.
// TODO(danderson): this is not correct, there's no reason
// roamAddr should be special like this.
dsts = append(dsts, as.roamAddr)
case as.curAddr != -1:
if as.curAddr >= len(as.addrs) {
log.Printf("[unexpected] magicsock bug: as.curAddr >= len(as.addrs): %d >= %d", as.curAddr, len(as.addrs))
break
}
// No roaming addr, but we've seen packets from a known peer
// addr, so keep using that one.
dsts = append(dsts, &as.addrs[as.curAddr])
default:
// We know nothing about how to reach this peer, and we're not
// spraying. Use the first address in the array, which will
// usually be a DERP address that guarantees connectivity.
if len(as.addrs) > 0 {
dsts = append(dsts, &as.addrs[0])
}
}
if logPacketDests {
log.Printf("spray=%v; roam=%v; dests=%v", spray, roamAddr, dsts)
}