Add sunos support #39
Conversation
|
Pinging @crawshaw and @danderson Per tailscale/tailscale#697 (comment) |
|
No response from David per usual, so I'll review this one. As a preliminary, it sounds like the current shortcoming is the requirement of I'll need to get a Illumos VM running to test this out in. Also, while we're doing this, we'll also need a wg-quick(8) implementation, like we have for FreeBSD and OpenBSD and such: https://git.zx2c4.com/wireguard-tools/tree/src/wg-quick . Interested in doing that? This can be a lot more fiddly than wireguard-go, depending on what kinds of routing schemes are supported by the Illumos kernel. |
ipc/uapi_illumos.go
Outdated
| @@ -0,0 +1,146 @@ | |||
| // +build illumos | |||
There was a problem hiding this comment.
I'm wondering -- is there a reason you couldn't add the illumos build tag to the uapi_bsd.go file? What's different about Illumos here?
There was a problem hiding this comment.
There is a diff, unfortunately. The BSD implementation uses kqueue to monitor for deletion of the unix socket (which is how wg-quick signals wireguard-go to shut down, afaict). A quick search tells me that Illumos doesn't have kqueue, but does have its own eventport API we could use.
Or if we're using SMF for lifetime management, we can skip all that and greatly simplify the UAPI listener, because wg-quick can make SMF shut stuff down reliably.
Reviewing the rest of this file on the assumption that we do indeed need a separate illumos implementation.
There was a problem hiding this comment.
Ack. So we'll keep it as a separate thing and monitor socket file deletion using an eventport. That's fine with me.
tun/tun_illumos.go
Outdated
| * descriptor number as the fd number is passed (with no other | ||
| * information) through the environment to the child. For now, use the | ||
| * "-f" flag. | ||
| */ |
There was a problem hiding this comment.
Seems like you already have everything, except for the name of the tun device, right? The tun%d part? Different Unices have various IOCTLs for grabbing this. Or some will expose the PID of the owning application and then you can enumerate. Unless the man pages tell some obvious way, you might be best off just poking at the kernel sources. If you can't find anything in a few days, let me know and I'll trawl around too. Every OS seems to have a different version of this same exercise.
There was a problem hiding this comment.
Did some quick rooting around in illumos-gate, but couldn't find net/if_tun.h (which seems to be the expected include file for both Solaris and Illumos). I also couldn't find it in SmartOS or OmniOS's forks, though I did find TritonDataCenter/smartos-live#330 .
This might be a minor issue for adding tuntap things to x/sys/unix, in that you'll have to manually define the constant blocks and whatnot, rather than being able to autogenerate them from the C headers.
Presumably that missing include also has the answer to "how do I get the interface name for this fd".
There was a problem hiding this comment.
The original source for the if_tun.h is probably here: https://github.com/kaizawa/tuntap/blob/master/if_tun.h
There was a problem hiding this comment.
Any updates on these investigations?
There was a problem hiding this comment.
Per the thread below on "cheating", I have thus far been unable to find an IOCTL for getting back the PPA.
There was a problem hiding this comment.
It's a little disheartening to encourage people to try and upstream useful things to wireguard-go (rather than keep them tailscale-specific), and then get chided for not doing code review on the weekend. I was taking my time because this is a fairly large change, and I'm unfamiliar with illumos's conventions and system facilities. Sending this very incomplete draft now, but I plan to do the bulk of reviewing during business hours.
Thanks @nshalman for sending this!
The comment was directed at David, not you Dave, who, by his own repeated admission, is often too busy for this stuff.
If you consider that a remarkable thing to do, you've made it clear where you stand with regards to this ecosystem. Thanks for making that clear to me. |
I don't consider it remarkable, I don't see how that follows from what I said. Regardless, I'll resume reading this code on Monday, since I'd certainly like Illumos support in wireguard-go, and it sounds like we agree on that. |
|
Oh, and regarding wg-quick: we did some preliminary digging on the facilities available in illumos. The networking facilities are similar to the BSDs (no policy routing, IP_BOUND_IF socket option), so that part of the wg-quick implementation looks almost identical to the openbsd userspace mode: overlay routes for endpoint IPs, and /etc/resolv.conf replacement for DNS configuration. For running and monitoring the daemon, SMF obviously springs to mind, but would lead to different semantics than the other wg-quick implementations (notably, Happy to send the patches for wg-quick. |
|
Thank you both, @zx2c4 and @danderson, for the feedback. I'll get started on cleaning up the stuff that I already understand, and work on figuring out the bits I don't understand just yet. :) In the meantime, replying to some non-code threads:
No others that I noticed, though the only exercise of this code on my end was that it worked connecting to a raspberry pi running the kernel module based version running on a rasberry pi a while back, and seems to mostly work when used by my hacked-up fork of tailscale.
SMF's |
There was a problem hiding this comment.
Thanks for sending the patch! As Jason already pointed out, I think the big thing to adjust is to get a lot of the low-level OS interaction stuff merged into golang.org/x/sys/unix instead of here, at which point this code can be significantly simplified.
I'm happy to take care of the x/sys/unix bits if you'd like, I don't know what your appetite is for learning "deep Go" (the various autogenerators that power x/sys/unix - useful knowledge if you plan to write more Go near the OS interface layer). Lemme know if I should take the lead, or just review the PR when you post it to https://github.com/golang/sys .
The other question we should resolve soon-ish is whether we're going to try for SMF integration in wg-quick, or go down the less-managed route that the BSDs took. My bias would be to go with SMF, since it looks like transient services gives us the semantics we want (wg-quick up not persisting across reboots), but surfaces wireguard-go's existence in system management tooling. That said, I'm not an illumos expert, so I defer to the approach you think is most idiomatic for the system, as long as the behavior of wg-quick matches other OSes.
ipc/uapi_illumos.go
Outdated
| @@ -0,0 +1,146 @@ | |||
| // +build illumos | |||
There was a problem hiding this comment.
There is a diff, unfortunately. The BSD implementation uses kqueue to monitor for deletion of the unix socket (which is how wg-quick signals wireguard-go to shut down, afaict). A quick search tells me that Illumos doesn't have kqueue, but does have its own eventport API we could use.
Or if we're using SMF for lifetime management, we can skip all that and greatly simplify the UAPI listener, because wg-quick can make SMF shut stuff down reliably.
Reviewing the rest of this file on the assumption that we do indeed need a separate illumos implementation.
tun/tun_illumos.go
Outdated
|
|
||
| /* | ||
| * Open another temporary file descriptor to the TUN driver. | ||
| * XXX It's not clear if this is actually needed, or if we could |
There was a problem hiding this comment.
Unless there's surprising semantics in the Illumos API surface, seems like it should be fine to reuse the device FD you extracted from the os.File.
tun/tun_illumos.go
Outdated
|
|
||
| var flags int32 = 0 | ||
|
|
||
| _, err := getmsg(int(tun.tunFile.Fd()), uintptr(0), |
There was a problem hiding this comment.
Do STREAMS offer any kind of batched read/write API? Or does getmsg/putmsg handle batched delivery under the hood? I ask because "one syscall per packet" ends up limiting tuntap performance on many platforms, and I'm wondering if illumos' use of STREAMS would let it sidestep that from the start.
There was a problem hiding this comment.
@gco pointed me at bufmod(4M)
I haven't explored what implementing that would look like here.
There was a problem hiding this comment.
Sounds like a nice optimization. Let's get things working right the vanilla way before we do things the wild way, though. It's already been a few years and this still hasn't shipped.
There was a problem hiding this comment.
Agreed. I just wanted to leave the breadcrumb for that here rather than leaving it sitting in my Twitter DMs. :)
There was a problem hiding this comment.
It will add latency, though. I suspect that putting the STREAM in RMSGD mode and using readv with sufficiently large buffers will work to read multiple packets with one syscall.
https://docs.oracle.com/cd/E19683-01/806-6546/appmech3-5/index.html
Thanks for offering to work on this. If you're big into Illumos, taking the lead on the wg-quick integration would be much appreciated. I'd prefer to keep the same semantics as all the other wg-quick implementations. If we want to later add deeper Illumos integration with SMF, that sounds fine, but would be a separate endeavor. For now, let's get everything all lined up. Usually there are a few problems to solve in wg-quick porting, beyond obvious things:
I would suggest reading all of the current wg-quick implementations, except perhaps the Android one, before starting, and then maybe start with one of the BSD implementations and tweak it. If you have additional questions about |
Sounds good. In that case, the Illumos implementation of wg-quick is likely to be nearly identical to the OpenBSD implementation:
Illumos has no facilities for this that I could find (no resolvconf, and no Illumos-specific facility for managing DNS). It also lacks the piecemeal namespacing support that dns hatchet relies on, so we're stuck with replacing /etc/resolv.conf.
There's no policy routing facilities. Similar to OpenBSD, we'll have to add /32 or /128 routes to the endpoints.
Again, similar situation to the BSDs, no good solution. AFAICT, the current FreeBSD and OpenBSD scripts just add/delete routes per the wg-quick configuration and don't handle conflict at all. The route monitor updates endpoint routes to avoid loops, but does nothing to handle repairing the routing table on exit.
Illumos's route(1M) supports -interface, so again should be the same as OpenBSD. Based on this, once we've merged the initial Illumos support, I'll probably start with a copy of the OpenBSD implementation, then circulate that in the Illumos community to see if there's something we can do better. |
There was a problem hiding this comment.
What is the etiquette for resolving conversations? Safe for me to mark nits and things as resolved? Preference for the original commenter to resolve?
I've replied to a few threads, and am about to push a commit with some initial nits fixed.
I just realized that I should have run gofmt over this code before opening the PR. My current plan is to delay the noise from making that change until the substantive fixes are in place.
tun/tun_illumos.go
Outdated
| * descriptor number as the fd number is passed (with no other | ||
| * information) through the environment to the child. For now, use the | ||
| * "-f" flag. | ||
| */ |
There was a problem hiding this comment.
The original source for the if_tun.h is probably here: https://github.com/kaizawa/tuntap/blob/master/if_tun.h
tun/tun_illumos.go
Outdated
|
|
||
| var flags int32 = 0 | ||
|
|
||
| _, err := getmsg(int(tun.tunFile.Fd()), uintptr(0), |
zx2c4-bot
force-pushed
the
master
branch
2 times, most recently
from
e467e07
to
2a607d1
Compare
zx2c4-bot
force-pushed
the
master
branch
2 times, most recently
from
c597c63
to
3b3de75
Compare
zx2c4-bot
force-pushed
the
master
branch
9 times, most recently
from
18aa925
to
ae5a948
Compare
nshalman
force-pushed
the
illumos-to-upstream
branch
from
ffbba2c
to
c6ce31a
Compare
|
Rebased to clear merge conflicts. Still ready for review by @zx2c4. |
tun/tun_solaris.go
Outdated
| return tun.events | ||
| } | ||
|
|
||
| func (tun *NativeTun) Read(bufs [][]byte, sizes []int, offset int) (int, error) { |
There was a problem hiding this comment.
buffs is used consistently elsewhere
There was a problem hiding this comment.
Fixed. Thank you for the close read!
There was a problem hiding this comment.
Darn. I was rushing home to beat you to it but...
https://git.zx2c4.com/wireguard-go/commit/?id=0ad14a89f5f9da577dae6a63ad196015e51a0381
bufs seems more correct to me, so I fixed it everywhere.
nshalman
force-pushed
the
illumos-to-upstream
branch
from
c6ce31a
to
15692aa
Compare
| l.connErr <- err | ||
| return | ||
| } | ||
| _, perr := l.evPort.GetOne(nil) |
| } | ||
|
|
||
| go func(l *UAPIListener) { | ||
| for { |
There was a problem hiding this comment.
Can you confirm that you've tested that this loop works as expected in a variety of circumstances?
| }(uapi) | ||
|
|
||
| // watch for new connections | ||
|
|
| defer unix.Close(routeSocket) | ||
|
|
||
| data := make([]byte, os.Getpagesize()) | ||
| for { |
There was a problem hiding this comment.
Can you confirm that this loop works in a variety of circumstances? Did you test it?
| // a chance to notice | ||
| // 2. It doesn't really respect 'ifconfig tunN down' properly so this code | ||
| // won't notice, and it will be broken and refuse to come back up | ||
| // if it is set down. |
There was a problem hiding this comment.
Not sure I understand what you mean. It'll get set down but won't notice coming back up? It won't notice going down but it'll come back up just fine?
| } | ||
| defer unix.Close(fd) | ||
|
|
||
| // get the STREAMS muxid for the tun device stream |
There was a problem hiding this comment.
"get" -- what are you getting? You're putting a constant into a variable, casting it to an int. And then later...
|
|
||
| // solaris/illumos defines the ioctl number as a signed int, but the | ||
| // common x/sys/unix functions use uint. | ||
| if err := unix.IoctlLifreq(fd, uint(reqnum), &l); err != nil { |
There was a problem hiding this comment.
...and then later you make it into a uint. Why?
Just write IoctlLifreq(fd, uint(unix.SIOG...), &l)
|
|
||
| // The "tun" device is a STREAMS module. We need to make a STREAMS ioctl | ||
| // request to that module using the TUNNEWPPA command. Returns the newly | ||
| // allocated PPA number. Passing -1 will request a dynamically assinged |
There was a problem hiding this comment.
I like that this comment uses capital letters at the beginning of the sentence and periods at the end. All comments should do that unless they really aren't sentences.
But then you mix two spaces between sentences and one space. Just do one space.
| assignedName := fmt.Sprintf("tun%d", ppa) | ||
|
|
||
| // Clean up anything possibly left behind by a crash | ||
| tunDestroy(assignedName) |
There was a problem hiding this comment.
Interesting...
What have you experienced being left behind? What are the circumstances here?
| tunDestroy(assignedName) | ||
|
|
||
| // The solaris/illumos tun driver doesn't have an ioctl for reading back the PPA. | ||
| // Just pass it through to the daemon process via environment variable. |
There was a problem hiding this comment.
Darn. Still? Weren't you talking to them about adding this at some point?
There was a problem hiding this comment.
Where's the source code for the tun driver? Is it not part of the base illumos?
There was a problem hiding this comment.
Weren't you talking to them about adding this at some point?
No, that was other functionality.
Also, I'm told that the code as it is today works on Solaris as well, an operating system that is closed source and over which I have even less influence. As gross as this is, I think it's a decent tradeoff to make to be able to support Solaris too.
If anything, some day illumos might get a newer better TUN driver that doesn't even use STREAMS and the solaris and illumos implementations in here would diverge. But that might not happen until I learn enough C to write one (and find the time and motivation to do so), so that could be (even more) years away...
There was a problem hiding this comment.
Can you link to the source code of the tun driver in illumos?
There was a problem hiding this comment.
As linked in a much older thread, the header file is here:
https://github.com/kaizawa/tuntap/blob/v1.3.3/if_tun.h
(Linking the version string referenced in e.g. the OmniOS build recipe:
https://github.com/omniosorg/omnios-build/blob/master/build/tuntap/build.sh#L21)
@gco or @alanc might be able to confirm that Solaris is using the same version...
There was a problem hiding this comment.
Solaris does not include the tuntap driver AFAIK, end-users need to install it if they need it. @danmcd would probably know the history (and merged tuntap into illumos!).
It's not in-gate in illumos. The distros ship it separately.
I linked to the OmniOS build recipe above. The SmartOS one is https://github.com/TritonDataCenter/illumos-extra/tree/master/tun and also references 1.3.
The OpenIndiana one is at https://github.com/OpenIndiana/oi-userland/tree/oi/hipster/components/network/tuntap
I don't see the Solaris build recipe in https://github.com/oracle/solaris-userland/tree/master/components so I'm guessing that it's in a closed repo. I was just wondering what version string you folks have and whether you've made any interesting changes to it.
There was a problem hiding this comment.
I'll emphasize "does not include". Solaris users need to get it from the 10 year old github repository.
There was a problem hiding this comment.
I'll emphasize "does not include". Solaris users need to get it from the 10 year old github repository.
Got it. So to be very explicit, in order to use my wireguard-go and tailscale ports you installed the driver directly from the https://github.com/kaizawa/tuntap repo. Is that correct?
There was a problem hiding this comment.
I got mine from @danmcd many years ago as part of a private tool called punchin. It's built from the same sources.
There was a problem hiding this comment.
Okay so it sounds like there's a single (old & crusty) codebase for tuntap shared across Solaris, Illumos, and derivatives. So,
Also, I'm told that the code as it is today works on Solaris as well, an operating system that is closed source and over which I have even less influence. As gross as this is, I think it's a decent tradeoff to make to be able to support Solaris too.
This is not correct then, right?
| var ppa int | ||
| _, err := fmt.Sscanf(tun.name, "tun%d", &ppa) | ||
| if err != nil { | ||
| return fmt.Errorf("received a bogus tun name: %s", tun.name) |
| return fmt.Errorf("received a bogus tun name: %s", tun.name) | ||
| } | ||
|
|
||
| // Unclear why, but we need to open a second handle on the tun device for |
|
|
||
| // Unclear why, but we need to open a second handle on the tun device for | ||
| // the ioctls that follow. Reusing the existing one does not work. | ||
| tunFD, err := unix.Open(tunNode, unix.O_RDWR, 0) |
There was a problem hiding this comment.
Everywhere else in the codebase, the D in Fd is lowercase.
|
|
||
| // select which ppa we're using | ||
| req := int(unix.IF_UNITSEL) | ||
| err = unix.IoctlSetPointerInt(tunFD, uint(req), ppa) |
There was a problem hiding this comment.
No need to make req a separate variable.
| return fmt.Errorf("unable to IF_UNITSEL: %v", err) | ||
| } | ||
|
|
||
| // link the tun stream to the IP multiplexor |
There was a problem hiding this comment.
Multiplexer with an e is the more common spelling. Is "o" a illumos thing?
No capital letter and period on this sentence, but it is elsewhere. I'm going to stop commenting on these things throughout the patchset. For next review just make sure they're all fixed per above.
| } | ||
|
|
||
| // By holding open this file descriptor on /dev/ip for the life of the connection | ||
| // and using I_LINK, STREAMS will tear everything down cleanly when we close these |
There was a problem hiding this comment.
But then I_LINK isn't actually set until deep down in a separate function. Maybe configureSTREAMS should be inlined into this function instead of being standalone, so that it's more clear.
|
|
||
| var l unix.Lifreq | ||
| if err := l.SetName(tun.name); err != nil { | ||
| return fmt.Errorf("failed to set name on Lifreq :%v", err) |
There was a problem hiding this comment.
failed to here, but unable to elsewhere. Look at what wireguard-go does elsewhere and copy that.
| } | ||
|
|
||
| // set the IP muxid | ||
| reqnum := int(unix.SIOCSLIFMUXID) |
There was a problem hiding this comment.
reqnum doesn't need to be a variable.
| return err | ||
| } | ||
|
|
||
| reqnum := int(unix.SIOCSLIFMTU) |
There was a problem hiding this comment.
reqnum doesn't need to be a separate variable.
There was a problem hiding this comment.
It looks like I've tried to remove all these reqnum assignment before, but to no avail.
We'll use this thread for you to help me satisfy the compiler.
The code as written compiles.
For each of the following variations I get the corresponding failure:
Just move it:
- reqnum := int(unix.SIOCGLIFMTU)
- if err := unix.IoctlLifreq(tun.ipfd, uint(reqnum), &l); err != nil {
+ if err := unix.IoctlLifreq(tun.ipfd, uint(int(unix.SIOCGLIFMTU)), &l); err != nil {Error:
tun/tun_solaris.go:389:44: cannot convert int(unix.SIOCGLIFMTU) (constant -1065850502 of type int) to type uint
Skip the intermediate int conversion:
- reqnum := int(unix.SIOCGLIFMTU)
- if err := unix.IoctlLifreq(tun.ipfd, uint(reqnum), &l); err != nil {
+ if err := unix.IoctlLifreq(tun.ipfd, uint(unix.SIOCGLIFMTU), &l); err != nil {Error:
tun/tun_solaris.go:389:44: cannot convert unix.SIOCGLIFMTU (untyped int constant -1065850502) to type uint
No conversions:
- reqnum := int(unix.SIOCGLIFMTU)
- if err := unix.IoctlLifreq(tun.ipfd, uint(reqnum), &l); err != nil {
+ if err := unix.IoctlLifreq(tun.ipfd, unix.SIOCGLIFMTU, &l); err != nil {Error:
tun/tun_solaris.go:389:39: cannot use unix.SIOCGLIFMTU (untyped int constant -1065850502) as uint value in argument to unix.IoctlLifreq (overflows)
I'm using go 1.20.1.
So. How should I proceed to clean these up?
There was a problem hiding this comment.
There was a problem hiding this comment.
See also: https://go.googlesource.com/sys/+/master/unix/syscall_solaris_test.go#370
Dunno if that counts; I wrote it. lolsob
There was a problem hiding this comment.
The compiler isn't confused; you are. Your comment in that test isn't accurate and does a disservice.
It looks like ioctl has the wrong signature in Go. According to https://illumos.org/man/2/ioctl req is an int. In Go, that's int32, not int or uint as the Go code has now. Presumably it's too late to fix that, which is unfortunate.
What you can do, though, is just avoid the negative ioctl constant for the ones you added, which probably have no users yet anyway. This way you don't run into ambiguity with sign extension. The easiest way to do this is probably just by adding & 0xffffffff onto the end of the constant definition. While you're at it, you can fix your bogus comment in the test too.
Be sure to submit this to x/sys with a compelling argument that you understand well; don't just link to this comment or something and call it a day.
(The lazy version, by the way, is just unix.IoctlLifreq(tun.ipfd, unix.SIOCGLIFMTU & 0xffffffff, &l)).
|
From your top message:
Testing with third-party software is not sufficient. Test |
Agreed. I have done testing with wireguard-go itself alongside WireGuard/wireguard-tools#17 too. They are packaged for OmniOS here: I have also gotten feedback from other users using both (some of which resulted in the recent bug fix in that PR.) Do you have a list of specific tests you would want to see done? |
I'm primarily concerned about all the weird cases with event listers on the unix socket, tun interface, and all that. So it'd be nice to know that you've printf'd up those code paths at some point to make sure its the behavior you're expecting. The lengthy development here and unsatisfactory earlier results make me slightly more wary of code that looks fine but might not be. So please just make sure that all parts of this PR are intentional and understood, and test that they're doing what that understanding expects them to do. |
|
I just spent a while reading through all the review over the last 3 years and the various revisions and such. I think I'm uncomfortable with this landing through Github and don't have enough trust that you've dotted all your is and crossed your ts. So I think we need to move to a more rigid and less haphazard review process. Please submit this to the wireguard mailing list when it's ready, as "PATCH v1", with a proper, detailed, and well-written commit message that explains the whole patch, the testing you've done, the provenance of the tun driver this is written for, rudimentary performance measurements, and so on. Be sure to add to the Be sure to read the documentation on submitting patches to mailing lists in the Linux kernel repository. Yes, this is annoying and probably feels like paperwork. But there's a lot of inconsistency here, and I don't feel alright moving forward with the usual "it's probably fine" stance I ordinarily have. |
Wireguard (wireguard-go) for sunos (illumos/Solaris)
Original Description
A currently working cleanup of the code by @jclulow from https://github.com/jclulow/wireguard-go-illumos-wip squashed down to a single commit.
This code works with the caveats noted in the commit message, and with additional modifications, can be added to the https://github.comtailscale/wireguard-go tree and be used to build mostly working tailscale binaries.
I barely know what I'm doing here, so any help carrying this across the finish line would be greatly appreciated.