Implementation of split real/virtual packet tunnel in WireGuardGo for IAN #10
Conversation
|
@mvd-ows, would be great if you could take a look at these changes. Since a description is lacking here, the main goal of these changes is to prep the WireGuard go shim for virtual networking. Due to reasons, we have to inject our own traffic into the tunnel, whilst still tunneling user traffic. To do this, we have chosen to use gvisor's userspace IP stack and implement our own @buggmagnet Do review the swift changes intently, and peruse the go changes at will. |
pinkisemils
requested review from
MarkusPettersson98
and removed request for
Serock3
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @mvd-ows)
Sources/WireGuardKitGo/router.go line 56 at r1 (raw file):
// BatchSize implements tun.Device. func (r *Router) BatchSize() int { return r.real.BatchSize()
Having thought about this long and hard, we should probably use a batch size of 128. Underruns just cost more static memory, overruns cost us more allocs since then we have to keep the overflow around.
Sources/WireGuardKitGo/router.go line 61 at r1 (raw file):
// Close implements tun.Device. func (r *Router) Close() error { // TODO: anything else we need to shut down here
We should probably add a test that checks for active go routines, as it's done in wireguard-go device tests too. https://github.com/WireGuard/wireguard-go/blob/master/device/device_test.go#L395-L421
Then we can be certain we are not leaking anything too.
Sources/WireGuardKitGo/router.go line 103 at r1 (raw file):
type PacketIdentifier [22]byte func humanReadableForm(pi PacketIdentifier) string {
This function isn't used anywhere anymore.
Sources/WireGuardKitGo/router.go line 126 at r1 (raw file):
const ProtocolUDP = 17 func getPorts(protocol byte, protocolHeader []byte) (uint16, uint16) {
The return arguments could be named to indicate what they mean. Or this could have a comment explaining what they mean, currently, it's very obtuse.
Sources/WireGuardKitGo/router.go line 164 at r1 (raw file):
} // TODO: skip the chain of IPv6 extension headers to get to the ports. // For now, we just ignore them and assume no ports if there are extension headers
There is a very useful library that we already depend on here - https://pkg.go.dev/gvisor.dev/gvisor/pkg/tcpip/header
This will parse the IPv6 and IPv4 headers properly. Would allow us to remove this TODO.
| return -4 | ||
| } | ||
| /// assign the same private IPs associated with your key | ||
| vtun, virtualNet, err := netstack.CreateNetTUN([]netip.Addr{privateAddr}, []netip.Addr{}, 1280) |
There was a problem hiding this comment.
That's a very low value for MTU. Is this adjusted after the device has been created?
| return -5 | ||
| } | ||
|
|
||
| if virtualNet == nil { |
There was a problem hiding this comment.
Why do you need to check virtualNet but not vtun?
If in some scenarios CreateNetTUN() can be said to have succeeded without returning a valid virtualNet, perhaps it would be more clear to instead return a *struct that holds vtun and virtualNet. The latter could then be pointer, and with applicable comments included about what to expect.
Sources/WireGuardKitGo/api-apple.go
Outdated
| err = dev.IpcSet(C.GoString(settings)) | ||
| if err != nil { | ||
| logger.Errorf("Unable to set IPC settings: %v", err) | ||
| unix.Close(dupTunFd) |
There was a problem hiding this comment.
dupTunFd is probably owned by tun at this point. Or you could just unconditionally close it after the call to CreateTUNFromFile().
Sources/WireGuardKitGo/api-apple.go
Outdated
| } | ||
| } | ||
| if i == math.MaxInt32 { | ||
| unix.Close(dupTunFd) |
Sources/WireGuardKitGo/router.go
Outdated
| return PacketHeaderData{nextHeader, srcPort, destAddress, destPort} | ||
| } | ||
|
|
||
| func fillPacketHeaderData(packet []byte, packetHeaderData *PacketHeaderData, offset int, isIncoming bool) bool { |
There was a problem hiding this comment.
Can't you subslice the packet on the outside and send one less argument?
Sources/WireGuardKitGo/router.go
Outdated
|
|
||
| func (r *Router) setVirtualRoute(header PacketHeaderData) { | ||
| identifier := header.asPacketIdentifier() | ||
| r.rxVirtualRoutes[identifier] = true |
There was a problem hiding this comment.
The standard map type is not safe for concurrent read/write. Not sure if applicable.
Sources/WireGuardKitGo/router.go
Outdated
| rw, err = r.real.Write(r.writeRealPackets, offset) | ||
| } | ||
| if rw < len(r.writeRealPackets) || err != nil { | ||
| return rw, err |
There was a problem hiding this comment.
Consider whether you should attempt to write to the virtual device as well, even when the primary write has failed.
Sources/WireGuardKitGo/router.go
Outdated
| if len(r.writeVirtualPackets) > 0 { | ||
| vw, err = r.virtual.Write(r.writeVirtualPackets, offset) | ||
| } | ||
| return rw + vw, err |
There was a problem hiding this comment.
Consider whether you should avoid relying on rw and vw being 0 in case of error. By convention, they are undefined.
Sources/WireGuardKitGo/router.go
Outdated
| func initializeReadPacketBuffer(size int) [][]byte { | ||
| buffer := make([][]byte, size, size) | ||
| for idx := range buffer { | ||
| buffer[idx] = make([]byte, 1500+maxOffset) |
There was a problem hiding this comment.
Using 1500 seems reasonable, but can be problematic. Most code that needs to store UDP packets allocate the maximum allowed size, which is 65K.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status: 1 of 7 files reviewed, 39 unresolved discussions (waiting on @acb-mv and @MarkusPettersson98)
Sources/WireGuardKitGo/api-apple.go line 152 at r1 (raw file):
} dupTunFd, err := unix.Dup(int(tunFd))
We should check that tunFd > 0 before calling dup on it
We also need to guarantee that tunFD < getdtablesize(), so we should fail in that case too.
I just noticed we don't do those checks in the wgTurnOn function either, we definitely should.
Sources/WireGuardKitGo/api-apple.go line 175 at r1 (raw file):
if err != nil { logger.Errorf("Failed to initialize virtual tunnel device: %v", err) return -5
missing unix.Close(dupTunFd)
Sources/WireGuardKitGo/api-apple.go line 180 at r1 (raw file):
if virtualNet == nil { logger.Errorf("Failed to initialize virtual tunnel device") return -6
missing unix.Close(dupTunFd)
Sources/WireGuardKitGo/api-apple.go line 208 at r1 (raw file):
} tunnelHandles[i] = tunnelHandle{dev, logger} // TODO: add a tunnel handle, or otherwise make sure we can create connections in the tunnel
We want to make sure we don't lose virtualNet here
Sources/WireGuardKitGo/go.mod line 20 at r1 (raw file):
golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect gvisor.dev/gvisor v0.0.0-20230927004350-cbd86285d259 // indirect
This will require us moving to go 1.20, we should make sure to track that change in the mullvad codebase.
Sources/WireGuardKitGo/router.go line 16 at r1 (raw file):
) const maxOffset = 128
The name doesn't exactly say how it's used.
Can we clarify it ?
Sources/WireGuardKitGo/router.go line 17 at r1 (raw file):
const maxOffset = 128 const expectedBufferCount = 1
Should we use instead conn.IdealBatchSize here ?
Sources/WireGuardKitGo/router.go line 62 at r1 (raw file):
func (r *Router) Close() error { // TODO: anything else we need to shut down here r.rxShutdown <- struct{}{}
Let's add a comment explaining why we have the same line repeated twice, to make sure we don't think it's a bug in the future if it's intentional
Sources/WireGuardKitGo/router.go line 68 at r1 (raw file):
if err1 != nil { return err1 } else {
We can remove this else statement here
Sources/WireGuardKitGo/router.go line 100 at r1 (raw file):
} // type (1 byte) + padding (1 byte) + src port (2 bytes) + dest addr + dest port
This should be protocol instead of type, or maybe protocol type
Sources/WireGuardKitGo/router.go line 101 at r1 (raw file):
// type (1 byte) + padding (1 byte) + src port (2 bytes) + dest addr + dest port type PacketIdentifier [22]byte
We should specify that dest addrs can either be 8 or 16 bytes (for IPv6) which is why the total size is 22
Sources/WireGuardKitGo/router.go line 117 at r1 (raw file):
result[0] = pi.protocol copy(result[4:], destAddrBytes[:]) binary.BigEndian.PutUint16(result[2:], pi.sourcePort)
Should we make sure that the padding is set correctly as well ?
result[1] = 0Sources/WireGuardKitGo/router.go line 296 at r1 (raw file):
_, err := device.Read(batch.packets, batch.sizes, maxOffset) if err != nil { return
We leak the batch we just got from the pool here
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, and @mvd-ows)
Sources/WireGuardKitGo/router.go line 155 at r1 (raw file):
var destAddress netip.Addr var srcPort, destPort uint16 nextHeader := packet[6]
Shouldn't this be protocol ? That's how it ends up being used, it makes it hard to follow that it has a name that has not much to do with the notion of protocol, especially given that the v4 version of this function names it protocol.
Sources/WireGuardKitGo/router.go line 195 at r1 (raw file):
return 0, fmt.Errorf("illegal offset %d > %d", offset, maxOffset) } var packets *PacketBatch
This should really be renamed packetsBatch, otherwise it makes the code down the line really hard to read
range packets.packets for example.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 62 at r1 (raw file):
Previously, buggmagnet wrote…
Let's add a comment explaining why we have the same line repeated twice, to make sure we don't think it's a bug in the future if it's intentional
Done.
Sources/WireGuardKitGo/router.go line 68 at r1 (raw file):
Previously, buggmagnet wrote…
We can remove this else statement here
Done.
Sources/WireGuardKitGo/router.go line 100 at r1 (raw file):
Previously, buggmagnet wrote…
This should be
protocolinstead oftype, or maybeprotocol type
Done.
Sources/WireGuardKitGo/router.go line 101 at r1 (raw file):
Previously, buggmagnet wrote…
We should specify that
dest addrscan either be 8 or 16 bytes (for IPv6) which is why the total size is 22
Done.
Sources/WireGuardKitGo/router.go line 103 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
This function isn't used anywhere anymore.
Done.
Sources/WireGuardKitGo/router.go line 117 at r1 (raw file):
Previously, buggmagnet wrote…
Should we make sure that the
paddingis set correctly as well ?result[1] = 0
Done.
Sources/WireGuardKitGo/router.go line 141 at r1 (raw file):
Previously, mvd-ows wrote…
Do you not need to validate
protocolto ensure that the parsing below is applicable?
No, source and destination IPs in an IPv4 header are protocol-independent
Sources/WireGuardKitGo/router.go line 168 at r1 (raw file):
Previously, mvd-ows wrote…
Can't you subslice the packet on the outside and send one less argument?
Done.
Sources/WireGuardKitGo/router.go line 296 at r1 (raw file):
Previously, buggmagnet wrote…
We leak the
batchwe just got from the pool here
Done.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 40 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 141 at r1 (raw file):
Previously, acb-mv wrote…
No, source and destination IPs in an IPv4 header are protocol-independent
Right, my bad. I forgot that you assume IPv4 in this function and figured you were reading the header type of the header currently being parsed. Which also doesn't make any sense because the Ethernet header is not exposed to WireGuard. Carry on :-)
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 39 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 16 at r1 (raw file):
Previously, buggmagnet wrote…
The name doesn't exactly say how it's used.
Can we clarify it ?
Done.
Sources/WireGuardKitGo/router.go line 26 at r1 (raw file):
Previously, mvd-ows wrote…
The name
make_batchis not idiomatic.makeBatchwould be preferred.Consider adding a simple comment that explains why it's needed. I assume it's so the uninitialized instance being returned can be tracked elsewhere.
Nit: Also consider if there is a better name to use than
truncate. I see two issues: First, in a truncate operation the waste it typically discarded. Second, at a quick glance, one might assume thatpbis immutable and that the returned instance is actually a truncated version of the data.
Mostly done.
I could call it split, though that's even more vague as to intention (and sounds like a list/string operation). More colloquially, the term haircut comes to mind (as used as a metaphor, for example in finance), but that also sounds silly and requires an explanation. truncate seems descriptive. Perhaps truncateReturningOverflow or similar if we need more disambiguation?
Sources/WireGuardKitGo/router.go line 56 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
Having thought about this long and hard, we should probably use a batch size of 128. Underruns just cost more static memory, overruns cost us more allocs since then we have to keep the overflow around.
That's all very well, except that wireGuardGo seems to feed us the packets one at a time.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 39 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 155 at r1 (raw file):
Previously, buggmagnet wrote…
Shouldn't this be
protocol? That's how it ends up being used, it makes it hard to follow that it has a name that has not much to do with the notion ofprotocol, especially given that the v4 version of this function names it protocol.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: 1 of 7 files reviewed, 42 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 261 at r1 (raw file):
} rw := 0
Can we rename those realWrites and virtualWrites ?
It's really hard to follow otherwise
Sources/WireGuardKitGo/router.go line 232 at r4 (raw file):
headerData := PacketHeaderData{} // realPackets := initializeWritePacketBuffer(len(bufs))
Let's remove those comments if we don't intend to use the code.
Sources/WireGuardKitGo/router.go line 263 at r4 (raw file):
vw, err = r.virtual.Write(r.writeVirtualPackets, offset) } return rw + vw, err
There's no chance that this should overflow, but let's add a comment that says that we know it cannot for safety's sake.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @mvd-ows)
Sources/WireGuardKitGo/api-apple.go line 152 at r1 (raw file):
Previously, buggmagnet wrote…
We should check that
tunFd > 0before callingdupon it
We also need to guarantee thattunFD < getdtablesize(), so we should fail in that case too.I just noticed we don't do those checks in the
wgTurnOnfunction either, we definitely should.
What is the point? unix.Dup returns an error already. In the case that it fails, we already check the error.
Moreover, if it fails, the only reason not to cause a panic would be to not leak. All bets are off if the process has ~5109 file descriptors.
After having run some experiments, even when at a file descriptor limit, dup just works, and works well. I can only imagine that the man page is outdated.
Sources/WireGuardKitGo/api-apple.go line 171 at r1 (raw file):
Previously, mvd-ows wrote…
That's a very low value for MTU. Is this adjusted after the device has been created?
We currently use 1280 everywhere on iOS, this is as per design, because iOS is slightly broken.
Sources/WireGuardKitGo/api-apple.go line 175 at r1 (raw file):
Previously, buggmagnet wrote…
missing
unix.Close(dupTunFd)
At this point, the file descriptor is already owned by the wireguard device, closing it would result in a double free, so this is a very dangerous suggestion.
Sources/WireGuardKitGo/api-apple.go line 190 at r1 (raw file):
Previously, mvd-ows wrote…
dupTunFdis probably owned bytunat this point. Or you could just unconditionally close it after the call toCreateTUNFromFile().
At this point, dev owns the Device anyway, so it really should call dev.Close().
Sources/WireGuardKitGo/router.go line 101 at r1 (raw file):
Previously, acb-mv wrote…
Done.
dest addr can be either 4 or 16 bytes, not 8. But we will always use a 16 byte representation. So I'd argue it's best to just say 16 bytes, end of.
Sources/WireGuardKitGo/router.go line 164 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
There is a very useful library that we already depend on here - https://pkg.go.dev/gvisor.dev/gvisor/pkg/tcpip/header
This will parse the IPv6 and IPv4 headers properly. Would allow us to remove this TODO.
lol
lmao
What that package does is practically the same as what we're doing anyway.
Sources/WireGuardKitGo/router.go line 185 at r1 (raw file):
Previously, mvd-ows wrote…
The standard map type is not safe for concurrent read/write. Not sure if applicable.
There are two distinct maps, one for the read side and one for the write side. They should only ever be read and modified by one go routine.
Sources/WireGuardKitGo/router.go line 263 at r4 (raw file):
Previously, buggmagnet wrote…
There's no chance that this should overflow, but let's add a comment that says that we know it cannot for safety's sake.
I really don't think we should be doing this.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 261 at r1 (raw file):
Previously, buggmagnet wrote…
Can we rename those
realWritesandvirtualWrites?
It's really hard to follow otherwise
Done.
Sources/WireGuardKitGo/router.go line 268 at r1 (raw file):
Previously, mvd-ows wrote…
Consider whether you should attempt to write to the virtual device as well, even when the primary write has failed.
The real and virtual devices are on the client side, and should be closed at the same time. Though if they're not for some reason and there is some incoming data to the virtual device, it is IMHO not the router's concern to cut that off.
Sources/WireGuardKitGo/router.go line 273 at r1 (raw file):
Previously, mvd-ows wrote…
Consider whether you should avoid relying on
rwandvwbeing0in case of error. By convention, they are undefined.
I'll zero virtualWritten in case of error so that it doesn't throw the count. In our case, it is possible that we successfully wrote some packets but got an error subsequently. Presumably the caller will ignore the count in case of error, but, all effort being equal, delivering the most accurate trivially obtainable count probably makes more sense than just zeroing the whole thing. (Given Go's lack of ternary operators, doing the latter would mean another 3-line if statement, further cluttering the code without particularly good reason.)
Sources/WireGuardKitGo/router.go line 232 at r4 (raw file):
Previously, buggmagnet wrote…
Let's remove those comments if we don't intend to use the code.
Done.
|
Previously, pinkisemils (Emīls Piņķis) wrote…
Done |
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/api-apple.go line 175 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
At this point, the file descriptor is already owned by the wireguard device, closing it would result in a double free, so this is a very dangerous suggestion.
Done.
Sources/WireGuardKitGo/api-apple.go line 178 at r1 (raw file):
Previously, mvd-ows wrote…
Why do you need to check
virtualNetbut notvtun?If in some scenarios
CreateNetTUN()can be said to have succeeded without returning a validvirtualNet, perhaps it would be more clear to instead return a *struct that holdsvtunandvirtualNet. The latter could then be pointer, and with applicable comments included about what to expect.
vtun and virtualNet are the same value, though being returned with different interfaces.
Sources/WireGuardKitGo/api-apple.go line 180 at r1 (raw file):
Previously, buggmagnet wrote…
missing
unix.Close(dupTunFd)
Done.
Sources/WireGuardKitGo/api-apple.go line 208 at r1 (raw file):
Previously, buggmagnet wrote…
We want to make sure we don't lose
virtualNethere
Done.
Sources/WireGuardKitGo/router.go line 17 at r1 (raw file):
Previously, buggmagnet wrote…
Should we use instead
conn.IdealBatchSizehere ?
Done.
Sources/WireGuardKitGo/router.go line 94 at r1 (raw file):
Previously, mvd-ows wrote…
Consider whether there should be a specific type to represent the protocol.
This has now been replaced with gvisor's tcpip.TransportProtocolNumber
Sources/WireGuardKitGo/router.go line 122 at r1 (raw file):
Previously, mvd-ows wrote…
These constants should be available elsewhere already. But perhaps this saves an import.
Good catch. We depend on GVisor (via WireGuardGo), so we have now imported its version of these constants.
Sources/WireGuardKitGo/router.go line 126 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
The return arguments could be named to indicate what they mean. Or this could have a comment explaining what they mean, currently, it's very obtuse.
Done.
Sources/WireGuardKitGo/router.go line 126 at r1 (raw file):
Previously, mvd-ows wrote…
Consider ensuring that the slice passed has enough data in it.
the getPacketHeaderData* functions have been refactored to accept a pointer to the structure to fill and return a boolean value, checking the input size, which takes care of this issue.
Sources/WireGuardKitGo/router.go line 128 at r1 (raw file):
Previously, mvd-ows wrote…
You can actually do
case ProtocolTCP, ProtocolUDP:to fold the cases. Consider usingbinary.BigEndian.Uint16().
Done.
Sources/WireGuardKitGo/router.go line 138 at r1 (raw file):
Previously, mvd-ows wrote…
Consider ensuring that the slice passed has enough data in it.
Done.
Sources/WireGuardKitGo/router.go line 140 at r1 (raw file):
Previously, mvd-ows wrote…
int(packet[0]) * 4may be more clear.
Done.
Sources/WireGuardKitGo/router.go line 195 at r1 (raw file):
Previously, buggmagnet wrote…
This should really be renamed
packetsBatch, otherwise it makes the code down the line really hard to read
range packets.packetsfor example.
I've renamed it to packetBatch; packetsBatch rolls off the tongue awkwardly, and doesn't add any clarity over packetBatch
Sources/WireGuardKitGo/router.go line 279 at r1 (raw file):
Previously, mvd-ows wrote…
Using
1500seems reasonable, but can be problematic. Most code that needs to store UDP packets allocate the maximum allowed size, which is 65K.
Done
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 46 at r1 (raw file):
Previously, mvd-ows wrote…
This is to ensure a single entity accesses the maps, right? Perhaps related data and logic could be placed in a separate private component (struct).
Done.
Sources/WireGuardKitGo/router.go line 63 at r1 (raw file):
Previously, mvd-ows wrote…
You should wait for the cancelled workers to complete.
Done.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 41 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 61 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
We should probably add a test that checks for active go routines, as it's done in
wireguard-godevice tests too. https://github.com/WireGuard/wireguard-go/blob/master/device/device_test.go#L395-L421Then we can be certain we are not leaking anything too.
Checks have been added, concluding that WireGuardGo's netstack.tun appears to be leaking goroutines. This has been documented as issue IOS-739, though its impact appears to be noncatastrophic if not manageable.
|
Previously, acb-mv wrote…
Same issue as this one? WireGuard/wireguard-go#101 |
|
Previously, acb-mv wrote…
Thanks for clarifying. |
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 23 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 61 at r1 (raw file):
Previously, mvd-ows wrote…
Same issue as this one? WireGuard/wireguard-go#101
Thanks. If we end up forking WireGuardGo, we may fix it ourselves, assuming the maintainers of the upstream branch don't do so.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, 1 of 3 files at r8, 1 of 2 files at r9.
Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/api-apple.go line 152 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
What is the point?
unix.Dupreturns an error already. In the case that it fails, we already check the error.Moreover, if it fails, the only reason not to cause a
panicwould be to not leak. All bets are off if the process has ~5109 file descriptors.
After having run some experiments, even when at a file descriptor limit,dupjust works, and works well. I can only imagine that the man page is outdated.
Actually, nevermind, it looks like getdtablesize is a very old and obsolete function at this point.
Thanks for running the experiment though.
Sources/WireGuardKitGo/router_test.go line 21 at r5 (raw file):
func TestRead(t *testing.T) { a, _, _ := netstack.CreateNetTUN([]netip.Addr{}, []netip.Addr{}, 1280)
This doesn't seem to be testing anything, we can safely remove it.
There was a problem hiding this comment.
Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router_test.go line 21 at r5 (raw file):
Previously, buggmagnet wrote…
This doesn't seem to be testing anything, we can safely remove it.
Done.
|
This is a slight change compared to before. It probably doesn't make a difference, unless a device's logger can be reassigned. Same with the other calls to |
There was a problem hiding this comment.
Reviewable status: 4 of 7 files reviewed, 12 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 113 at r10 (raw file):
} type PacketHeaderData struct {
Have you considered using remoteAddr etc instead of destAddr? I find it quite confusing when related code reads the source IP and puts it in destAddr.
Sources/WireGuardKitGo/router.go line 225 at r10 (raw file):
for packetIndex := range packetBatch.packets { copy(bufs[packetIndex][offset:], packetBatch.packets[packetIndex][maxOffset:])
What's the purpose of the maxOffset subslicing?
Sources/WireGuardKitGo/router.go line 229 at r10 (raw file):
if packetBatch.isVirtual && fillPacketHeaderData(bufs[packetIndex][offset:], &headerData, false) { r.read.setVirtualRoute(headerData)
The comment at the start of this function mentions parallel execution. But the function called here is not safe in that context, without using additional locking.
Sources/WireGuardKitGo/router.go line 241 at r10 (raw file):
case newVirtualRoute := <-r.virtualRouteChan: r.virtualRoutes[newVirtualRoute] = true continue
Superfluous. There is no fall-through unless explicitly added.
Sources/WireGuardKitGo/router.go line 309 at r10 (raw file):
if err != nil { r.batchPool.Put(batch) return
Consider whether continue is a better choice here. Currently, if there's a single failed read, the worker silently aborts and the tunnel becomes permanently broken?
There was a problem hiding this comment.
Reviewable status: 4 of 7 files reviewed, 11 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 113 at r10 (raw file):
Previously, mvd-ows wrote…
Have you considered using
remoteAddretc instead ofdestAddr? I find it quite confusing when related code reads the source IP and puts it indestAddr.
Good idea; I'll change source* to local* and dest* to remote*
Sources/WireGuardKitGo/router.go line 225 at r10 (raw file):
Previously, mvd-ows wrote…
What's the purpose of the
maxOffsetsubslicing?
It's a fossil from when we thought we may be passing the buffers back to WireGuardGo. We can get rid of it.
Sources/WireGuardKitGo/router.go line 309 at r10 (raw file):
Previously, mvd-ows wrote…
Consider whether
continueis a better choice here. Currently, if there's a single failed read, the worker silently aborts and the tunnel becomes permanently broken?
Good catch. @pinkisemils , would you agree?
|
Previously, mvd-ows wrote…
How likely is |
|
Previously, acb-mv wrote…
At the moment, wireguard-go only spawns a single read worker, which (while this assumption holds) makes it safe to eschew the overhead of additional locking. I have rephrased the comment to make this assumption explicit. |
|
Previously, acb-mv wrote…
maxOffset has now been removed. |
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 12 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router_test.go line 542 at r10 (raw file):
secondPayload := []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10} thirdPayload := make([]byte, 100) rand.Read(secondPayload[:])
Wrong instance used as argument here?
Sources/WireGuardKitGo/router_test.go line 597 at r10 (raw file):
t.Fatalf("Expected to receive %v, instead got %v", secondPayload, clientBuff[:bytesRead]) }
Isn't there supposed to be a clientConnection.Write(thirdPayload) here?
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router.go line 225 at r10 (raw file):
Previously, acb-mv wrote…
maxOffset has now been removed.
Nit: That same subslicing can be removed alltogether, since [0:] effectively produces the source slice unchanged.
Sources/WireGuardKitGo/router.go line 229 at r10 (raw file):
Previously, acb-mv wrote…
At the moment, wireguard-go only spawns a single read worker, which (while this assumption holds) makes it safe to eschew the overhead of additional locking. I have rephrased the comment to make this assumption explicit.
Updated comment looks good 👍
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/api-apple.go line 152 at r1 (raw file):
Previously, buggmagnet wrote…
Actually, nevermind, it looks like
getdtablesizeis a very old and obsolete function at this point.
Thanks for running the experiment though.
Done.
Sources/WireGuardKitGo/router.go line 225 at r10 (raw file):
Previously, mvd-ows wrote…
Nit: That same subslicing can be removed alltogether, since
[0:]effectively produces the source slice unchanged.
Done
Sources/WireGuardKitGo/router_test.go line 542 at r10 (raw file):
Previously, mvd-ows wrote…
Wrong instance used as argument here?
Fixed
Sources/WireGuardKitGo/router_test.go line 597 at r10 (raw file):
Previously, mvd-ows wrote…
Isn't there supposed to be a
clientConnection.Write(thirdPayload)here?
Fixed
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @mvd-ows, and @pinkisemils)
Sources/WireGuardKitGo/go.mod line 20 at r1 (raw file):
Previously, buggmagnet wrote…
This will require us moving to go 1.20, we should make sure to track that change in the mullvad codebase.
We do need to move to Go 1.20 because of dependencies we use here.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @mvd-ows)
Sources/WireGuardKitGo/router.go line 309 at r10 (raw file):
Previously, acb-mv wrote…
Good catch. @pinkisemils , would you agree?
A continue may well make this a busy loop. We could introduce yet another channel to relay the error back to the Read call to make it fail. Ideally, we would shut down both readers in that case.
|
Previously, pinkisemils (Emīls Piņķis) wrote…
Done. |
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @acb-mv, @buggmagnet, @MarkusPettersson98, and @pinkisemils)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router_test.go line 294 at r13 (raw file):
func TestUDP(t *testing.T) { aIp := netip.AddrFrom4([4]byte{1, 2, 3, 4})
nit
A lot of the setup in these tests is very similar, which makes the tests really verbose for no reason, for instance I count this line 8 times in the file.
Is there a chance we can do a bit of refactoring to either parametrize those setups, or have a function that does the init so that the tests are easier to read ?
Sources/WireGuardKitGo/router_test.go line 305 at r13 (raw file):
b, bNet, _ := netstack.CreateNetTUN([]netip.Addr{bIp}, []netip.Addr{}, 1280) // aDev := device.NewDevice(a, conn.NewStdNetBind(), device.NewLogger(device.LogLevelVerbose, ""))
Can we delete this and all the other commented code lines in this file ?
Sources/WireGuardKitGo/router_test.go line 531 at r13 (raw file):
} func testTcpTraffic(t *testing.T, serverNet, clientNet *netstack.Net, serverIP, clientIP netip.Addr) {
The clientIP parameter is not used
Sources/WireGuardKitGo/router_test.go line 621 at r13 (raw file):
t.Fatalf("Failed to parse a packet header") } assert.Equal(t, header.protocol, uint8(17))
assert.Equal(t, header.protocol, tcpip.TransportProtocolNumber(17))
Same for the other protocol check in this test.
Let's also delete those comments
|
Previously, buggmagnet wrote…
Done |
|
Previously, buggmagnet wrote…
Removed |
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router_test.go line 305 at r13 (raw file):
Previously, buggmagnet wrote…
Can we delete this and all the other commented code lines in this file ?
Done.
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)
Sources/WireGuardKitGo/router_test.go line 621 at r13 (raw file):
Previously, buggmagnet wrote…
assert.Equal(t, header.protocol, tcpip.TransportProtocolNumber(17))
Same for the other protocol check in this test.
Let's also delete those comments
Done.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv, @MarkusPettersson98, and @pinkisemils)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @MarkusPettersson98)
Sources/WireGuardKitGo/router.go line 306 at r15 (raw file):
if err != nil { r.batchPool.Put(batch) r.error = err
This is not thread safe, the other thread might not see this write until way later.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @MarkusPettersson98)
pinkisemils
force-pushed
the
ios-711-wgTurnOnIan-router
branch
from
8efb9dd to
3385ada
Compare
pinkisemils
force-pushed
the
ios-711-wgTurnOnIan-router
branch
from
3385ada to
cdd6038
Compare
This change is