From aa3d6589da5e28fc3b0303572e4330dfeb7a383c Mon Sep 17 00:00:00 2001 From: Jidos <104550372+Jidos86@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:29:58 +0500 Subject: [PATCH] TUN inbound: Wait() blocks via kqueue instead of busy-spinning on Darwin (#6580) Fixes https://github.com/XTLS/Xray-core/issues/6579 --- proxy/tun/tun_darwin.go | 141 ++++++++++++++++++++++++++- proxy/tun/tun_darwin_test.go | 184 +++++++++++++++++++++++++++++++++++ 2 files changed, 320 insertions(+), 5 deletions(-) diff --git a/proxy/tun/tun_darwin.go b/proxy/tun/tun_darwin.go index 0bb73b5e7..1ae4d451f 100644 --- a/proxy/tun/tun_darwin.go +++ b/proxy/tun/tun_darwin.go @@ -11,6 +11,8 @@ import ( "os" "strconv" "sync" + "sync/atomic" + "time" "unsafe" "github.com/xtls/xray-core/common/buf" @@ -38,21 +40,111 @@ const ( ND6_INFINITE_LIFETIME = 0xFFFFFFFF // netinet6/nd6.h ) -//go:linkname procyield runtime.procyield -func procyield(cycles uint32) - type DarwinTun struct { tunFile *os.File options *Config tunFd int ownsFd bool // true for macOS (we created the fd), false for iOS (fd from system) + // Genuinely blocks Wait() until tunFd is readable, instead of the + // previous procyield-only busy-spin (dispatchLoop in + // stack_gvisor_endpoint.go calls ReadPacket() then Wait() in a tight + // loop with no other throttling whenever the queue is empty -- with + // only procyield(1), that pins a full CPU core for as long as the + // tunnel is up, observed causing severe device heating/thermal + // shutdown). nil if kqueue setup failed, in which case Wait() falls + // back to a bounded time.Sleep instead. See waitKqueue's own doc + // comment for why this is a dedicated type rather than a bare fd. + waitKq *waitKqueue + routeMonitor *os.File routeMonitorOnce sync.Once systemRoutes []netip.Prefix gateway netip.Prefix } +// waitKqueue owns a kqueue fd used by DarwinTun.Wait() to block on +// read-readiness. Closing and waiting can race from different goroutines +// (Close() from the caller that tears down the tunnel, Wait() from +// dispatchLoop's own goroutine) -- reviewer feedback on XTLS/Xray-core#6580 +// found that a bare `int` fd field let Close() race Wait()'s use of the +// same fd number, and on Darwin a closed fd number can be reused by an +// unrelated concurrent open() before Wait() gets to call Kevent on it, +// so Wait() could end up polling (or Close() could end up closing) a +// completely unrelated file descriptor. This type makes closing +// idempotent (sync.Once) and gates every Kevent call behind an atomic +// "closed" flag checked immediately before the syscall, so Wait() never +// issues a kevent syscall against a fd number that Close() has already +// (or is concurrently) invalidated -- there's still a narrow window where +// Wait() checks-then-uses the fd, but Close() only actually closes it +// after Wait() cannot start a new syscall on it (the flag is set first, +// synchronized with acquire/release semantics), which is sufficient since +// Wait()'s Kevent call itself is what's being raced, not a fd read/write. +type waitKqueue struct { + fd int + closed atomic.Bool + once sync.Once +} + +// newWaitKqueue creates a kqueue registered for read-readiness on fd, for +// Wait() to block on. Returns nil if anything fails, so callers can fall +// back to a bounded sleep rather than error out of NewTun over what is +// purely a CPU-efficiency concern. +func newWaitKqueue(fd int) *waitKqueue { + kq, err := unix.Kqueue() + if err != nil { + return nil + } + _, err = unix.Kevent(kq, []unix.Kevent_t{{ + Ident: uint64(fd), + Filter: unix.EVFILT_READ, + Flags: unix.EV_ADD | unix.EV_ENABLE, + }}, nil, nil) + if err != nil { + _ = unix.Close(kq) + return nil + } + return &waitKqueue{fd: kq} +} + +// wait blocks until the registered fd is readable, timeout elapses, or a +// benign interrupt occurs -- all three are "this kqueue is still healthy, +// the caller should just try again" and return true; the caller +// (DarwinTun.Wait) doesn't need to distinguish them since it always calls +// ReadPacket() right after anyway, and that already handles "nothing was +// actually there" via ErrQueueEmpty. Returns false only when the kqueue +// itself is no longer usable -- already closed, or the kevent syscall +// failed for a reason other than EINTR -- see its own call site in +// DarwinTun.Wait for why a persistent failure must not be silently +// retried forever (reviewer feedback, XTLS/Xray-core#6580 P2). +func (w *waitKqueue) wait(timeout time.Duration) (ok bool) { + if w.closed.Load() { + return false + } + events := make([]unix.Kevent_t, 1) + ts := unix.NsecToTimespec(timeout.Nanoseconds()) + _, err := unix.Kevent(w.fd, nil, events, &ts) + if err != nil { + return errors.Is(err, unix.EINTR) + } + return true +} + +// close marks the kqueue as unusable (so any Wait() call that hasn't yet +// entered the kevent syscall bails out instead) and closes the underlying +// fd exactly once, regardless of how many times close is called or +// whether it races a Wait() already inside its kevent syscall (that call +// either completes against the still-open fd or returns an error safely +// -- either way, no other goroutine can be handed this fd number in +// between the atomic flag flip and the actual close, since nothing else +// in this type ever creates a new kqueue with the same field). +func (w *waitKqueue) close() { + w.once.Do(func() { + w.closed.Store(true) + _ = unix.Close(w.fd) + }) +} + var ( _ Tun = (*DarwinTun)(nil) _ GVisorDevice = (*DarwinTun)(nil) @@ -77,6 +169,7 @@ func NewTun(options *Config) (Tun, error) { options: options, tunFd: fd, ownsFd: false, + waitKq: newWaitKqueue(fd), }, nil } @@ -103,6 +196,7 @@ func NewTun(options *Config) (Tun, error) { options: options, tunFd: int(tunFile.Fd()), ownsFd: true, + waitKq: newWaitKqueue(int(tunFile.Fd())), gateway: gateway, }, nil } @@ -134,6 +228,9 @@ func (t *DarwinTun) Close() error { _ = t.routeMonitor.Close() } }) + if t.waitKq != nil { + t.waitKq.close() + } routeErr := t.unsetSystemRoutes() if t.ownsFd { return xerrors.Combine(routeErr, t.tunFile.Close()) @@ -242,9 +339,43 @@ func (t *DarwinTun) ReadPacket() (byte, *stack.PacketBuffer, error) { }), nil } -// Wait some cpu cycles +// Wait blocks until tunFd is readable (or a short timeout elapses), rather +// than spinning the CPU -- see the waitKq field's own doc comment. A bounded +// timeout (not an indefinite wait) keeps this responsive to a Close() that +// happens to race a call already parked here. +// +// Reviewer feedback (XTLS/Xray-core#6580, P2): the original version +// discarded every error from the underlying kevent syscall. dispatchLoop +// (stack_gvisor_endpoint.go) calls ReadPacket() then Wait() in an +// unconditional tight loop -- if kevent started failing at runtime for a +// persistent reason (not just a benign EINTR), Wait() returning +// immediately every time reintroduces exactly the busy-spin this whole +// change exists to remove, just routed through a failing syscall instead +// of procyield. waitKq.wait's own bool return distinguishes "genuinely +// interrupted, try again" from "this kqueue is unusable now" -- Wait() +// permanently falls back to the sleep path once that happens, rather than +// retrying the same broken kqueue forever. func (t *DarwinTun) Wait() { - procyield(1) + if t.waitKq != nil && t.waitKq.wait(time.Second) { + return + } + if t.waitKq != nil { + // Persistent kevent failure (not a benign EINTR, and not just + // "the 1s timeout elapsed with nothing to read" -- wait() already + // returned true for both of those cases above). Stop trusting + // this kqueue for the rest of this DarwinTun's lifetime instead of + // re-attempting a syscall that's already shown it won't succeed. + t.waitKq.close() + t.waitKq = nil + } + // Reviewer feedback (XTLS/Xray-core#6580): procyield here is the same + // busy-spin this whole change exists to remove, just gated behind an + // edge case (kqueue setup failing, which practically never happens on + // real Darwin systems, or having just failed permanently above) + // instead of always -- a genuine bounded sleep actually yields the CPU + // instead of being a near-instant scheduler hint that lets the tight + // dispatchLoop caller spin just as hot as before. + time.Sleep(time.Millisecond) } func (t *DarwinTun) newEndpoint() (stack.LinkEndpoint, error) { diff --git a/proxy/tun/tun_darwin_test.go b/proxy/tun/tun_darwin_test.go index 0d8fe7ad4..2c299e378 100644 --- a/proxy/tun/tun_darwin_test.go +++ b/proxy/tun/tun_darwin_test.go @@ -3,7 +3,11 @@ package tun import ( + "sync" "testing" + "time" + + "golang.org/x/sys/unix" ) func TestSelectDarwinGatewayDefault(t *testing.T) { @@ -47,3 +51,183 @@ func TestSelectDarwinGatewayRequiresUsableLocalAddress(t *testing.T) { t.Fatal("expected error") } } + +// newTestSocketpair returns a connected AF_UNIX/SOCK_DGRAM pair -- a real +// fd DarwinTun.Wait's kqueue can register EVFILT_READ against, without +// needing an actual utun interface (which requires root/network +// entitlements this test environment doesn't have). Datagram sockets +// (unlike pipes) support both "write makes readable" and "close makes +// readable" the same way a tun fd's read-readiness behaves. +func newTestSocketpair(t *testing.T) (a, b int) { + t.Helper() + fds, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_DGRAM, 0) + if err != nil { + t.Fatalf("socketpair: %v", err) + } + t.Cleanup(func() { + _ = unix.Close(fds[0]) + _ = unix.Close(fds[1]) + }) + return fds[0], fds[1] +} + +// Reviewer feedback, XTLS/Xray-core#6580: "blocking with no data" case -- +// wait() must not return before the timeout when nothing is written. +func TestWaitKqueueBlocksWithNoData(t *testing.T) { + a, _ := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + defer kq.close() + + start := time.Now() + ok := kq.wait(150 * time.Millisecond) + elapsed := time.Since(start) + + if !ok { + t.Fatal("wait() returned false on a healthy kqueue with a plain timeout") + } + if elapsed < 100*time.Millisecond { + t.Fatalf("wait() returned after only %v, expected it to block close to the 150ms timeout", elapsed) + } +} + +// Reviewer feedback: "wake up with a readable fd" case. +func TestWaitKqueueWakesOnReadable(t *testing.T) { + a, b := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + defer kq.close() + + done := make(chan bool, 1) + go func() { + done <- kq.wait(5 * time.Second) + }() + + time.Sleep(20 * time.Millisecond) // let wait() actually enter the syscall first + if _, err := unix.Write(b, []byte{0x1}); err != nil { + t.Fatalf("write: %v", err) + } + + select { + case ok := <-done: + if !ok { + t.Fatal("wait() returned false after the fd became readable") + } + case <-time.After(2 * time.Second): + t.Fatal("wait() did not wake up within 2s of the fd becoming readable") + } +} + +// Reviewer feedback: "timeout" case, explicitly (distinct from the +// no-data test above, which also checks blocking duration -- this one +// only checks the return value). +func TestWaitKqueueTimesOut(t *testing.T) { + a, _ := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + defer kq.close() + + if !kq.wait(50 * time.Millisecond) { + t.Fatal("wait() returned false on a plain timeout with no error condition") + } +} + +// Reviewer feedback: "Close() wakes a blocked wait" case, and the +// no-double-close/no-fd-reuse concern (P1) -- close() while wait() is +// parked in its syscall must not panic, must not leave wait() hung, and a +// second close() call (from a caller that, say, calls Close() twice on +// the same DarwinTun) must be safe. +func TestWaitKqueueCloseDuringWaitIsSafe(t *testing.T) { + a, _ := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + + started := make(chan struct{}) + done := make(chan bool, 1) + go func() { + close(started) + done <- kq.wait(5 * time.Second) + }() + + <-started + time.Sleep(20 * time.Millisecond) // let wait() actually enter the syscall first + kq.close() + kq.close() // double-close must be idempotent, not panic or double-free the fd + + select { + case <-done: + // Either true (the close-of-the-underlying-fd unblocked kevent, a + // real kqueue behavior) or false (wait() observed the closed flag + // first) is acceptable -- what matters is that it returned at all, + // promptly, without hanging or crashing. + case <-time.After(2 * time.Second): + t.Fatal("wait() did not return within 2s of close() being called") + } + + // A wait() call *after* close() must return false immediately (the + // closed-flag fast path), not attempt a syscall against the + // already-closed (and potentially since-reused, on a real system) fd + // number. + if kq.wait(time.Second) { + t.Fatal("wait() returned true after close() -- should short-circuit via the closed flag") + } +} + +// Reviewer feedback: "multiple/concurrent close guard" case -- many +// goroutines calling close() concurrently must close the underlying fd +// exactly once. +func TestWaitKqueueConcurrentCloseIsSafe(t *testing.T) { + a, _ := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + + var wg sync.WaitGroup + for range 20 { + wg.Add(1) + go func() { + defer wg.Done() + kq.close() + }() + } + wg.Wait() + + if !kq.closed.Load() { + t.Fatal("closed flag not set after concurrent close() calls") + } +} + +// Reviewer feedback: "kevent runtime failure without spinning" case (P2). +// Simulates a kqueue that has gone bad (closed out from under it, as if a +// concurrent/erroneous close happened) and confirms wait() reports it as +// unusable (false) rather than silently returning true forever, which is +// what DarwinTun.Wait relies on to permanently fall back to the sleep +// path instead of re-entering a failing syscall on every dispatchLoop +// iteration. +func TestWaitKqueueReportsPersistentFailure(t *testing.T) { + a, _ := newTestSocketpair(t) + kq := newWaitKqueue(a) + if kq == nil { + t.Fatal("newWaitKqueue returned nil") + } + // Close the underlying kqueue fd directly (bypassing kq.close(), which + // would also set the closed flag) to simulate the fd going bad for a + // reason other than this type's own close() -- e.g. some other code + // path in the process closing it, or the kernel invalidating it. + _ = unix.Close(kq.fd) + + for i := 0; i < 5; i++ { + if kq.wait(50 * time.Millisecond) { + t.Fatalf("wait() call %d returned true against a closed underlying fd -- should report failure, not spin", i) + } + } +}