From fb0ef4c19d169cbf7466ad871e866b954a27ffcf Mon Sep 17 00:00:00 2001 From: Varun Agarwal Date: Wed, 9 Sep 2026 22:32:39 +0530 Subject: [PATCH 1/3] fix(ipn/proxies): restore getproxytimeout for proxyFor lock-read guard proxyFor()'s internal timeout for its RLock'd map-read goroutine (intended purely as a deadlock-recovery guard, per the ProxyFor doc-comment: "if it takes longer than getproxytimeout, it returns an error") was inadvertently changed from getproxytimeout (5s) to minWaitPeriodSec/2 (1s), with no accompanying doc-comment update and no explanation in the commit message. Impact: ProxyFor() only retries/waits for a missing proxy when isWellknown(id) is true (WG/Orbot/pip/internal/global-h1 ids). For any other, app-registered custom proxy id, proxyFor() is the only lookup attempt - there is no fallback wait. On a loaded or low-RAM device, the paired px.Lock() in AddProxy/RemoveProxy can legitimately hold the mutex for longer than 1s while a proxy is being registered/torn down (especially proxies whose constructor performs real I/O). Previously this had up to 5s of slack before proxyFor() gave up; now it has only 1s, turning a previously-recoverable, momentary lock stall into a permanent, unretried "proxy not found" for that connection - observed downstream (celzero/rethink-app-derived fork) as an intermittently failing custom local HTTP proxy route on Android TV / Fire TV Stick hardware, causing affected app connections to fail and the calling app to retry indefinitely. Fix: restore timeout := getproxytimeout, matching the function's existing doc-comment and preserving the deadlock-recovery intent this guard was designed for, without touching the minWaitPeriodSec change (3s->2s) which only affects the separate wellknown-id retry/backoff path and is not implicated in this regression. No behavior change for the intended deadlock-recovery case (an actual hang still errors out, just with the originally-documented 5s grace period instead of 1s). --- intra/ipn/proxies.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/intra/ipn/proxies.go b/intra/ipn/proxies.go index 797941b4..f752fd3b 100644 --- a/intra/ipn/proxies.go +++ b/intra/ipn/proxies.go @@ -929,7 +929,26 @@ func (px *proxifier) proxyFor(id string) (Proxy, error) { // Ingress (dummy): no fast path, fall through to general lookup } - timeout := time.Duration(minWaitPeriodSec/2) * time.Second + // Regression fix: this used to be getproxytimeout (5s) and was + // inadvertently shortened to minWaitPeriodSec/2 (1s) in 8677a52c + // ("core/volatile: cr by muse spark" era commit chain). proxyFor is + // called for every proxy id, including non-wellknown, app-registered + // ids (see isWellknown/ProxyFor above) for which there is NO retry/ + // wait fallback -- ProxyFor returns immediately with errProxyNotFound + // for those ids, so this is the *only* window a caller gets to find + // a just-registered proxy. The lookup itself is a cheap RLock'd map + // read (see below), but on loaded/low-RAM devices the paired Lock() + // in AddProxy/RemoveProxy can legitimately hold the mutex for longer + // than 1s during proxy setup/teardown, especially for proxies that do + // real I/O in their constructor. Shortening this guard to 1s turns a + // rare, recoverable stall into a hard, unretried lookup failure for + // any non-wellknown proxy id registered right around this window -- + // observed in production as a permanently-failing custom local proxy + // route until the next reconnect. Restoring getproxytimeout (5s) + // keeps this a deadlock-recovery guard (its original documented + // purpose, see the ProxyFor doc-comment above) rather than a + // register-race timeout. + timeout := getproxytimeout // go.dev/play/p/xCug1W3OcMH p, completed := core.Grx("pxr.ProxyFor: "+id, func(_ context.Context) (Proxy, error) { px.RLock() From a25158483825c96f2cdc926c95399d4a045e692f Mon Sep 17 00:00:00 2001 From: Varun Agarwal Date: Thu, 10 Sep 2026 00:06:15 +0530 Subject: [PATCH 2/3] =?UTF-8?q?=EF=BB=BFRevert=20"ipn/proxies:=20ping=20if?= =?UTF-8?q?=20last=20good=20rx/tx=20is=20more=20than=202m"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f8fdaafd4d2d6ef9e2c656670404114853dfc866. StreamShield investigation into a Zee5 playback hang (video CDN TCP connections dying with "endpoint is closed for send" on the direct/ Exit proxy path) is bisecting this commit as one of two remaining suspects in the celzero/firestack 61894b7fdb..8677a52cbd range, after ruling out and fixing the proxyFor lock-read timeout regression (315d2335, see StreamShield PR celzero/firestack#248). This commit added a periodic core.Gx("proxy.health.TxRx."+pid, ...) health-ping trigger fired whenever a proxy's last-good rx or tx is older than tzzTimeout (2m). Reverting to test whether this newly introduced background Ping() call against long-idle-but-otherwise- healthy proxies (including the Exit passthrough proxy carrying real CDN traffic) is contributing to, or masking symptoms of, the observed TCP write failures. Not confirmed as root cause; reverted as part of a bisection A/B test. --- intra/ipn/proxy.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/intra/ipn/proxy.go b/intra/ipn/proxy.go index cc22cd88..86c4b918 100644 --- a/intra/ipn/proxy.go +++ b/intra/ipn/proxy.go @@ -766,9 +766,6 @@ func healthy(p Proxy) error { age := now - stat.LastOpen oldEnough := age > ageThreshold.Milliseconds() - - lastGoodRx := now - stat.LastGoodRx - lastGoodTx := now - stat.LastGoodTx lastOK := stat.LastOK lastOKNeverOK := lastOK <= 0 lastOKBeyondThres := lastOK > 0 && now-lastOK > lastOKThreshold.Milliseconds() @@ -778,10 +775,8 @@ func healthy(p Proxy) error { pid, core.FmtMillis(age), pxstatus(status), lastOKNeverOK, lastOKBeyondThres) } else if now-lastOK > tzzTimeout.Milliseconds() { core.Gx("proxy.health.TZZ."+pid, func() { p.Ping() }) - } else if lastGoodTx > tzzTimeout.Milliseconds() || lastGoodRx > tzzTimeout.Milliseconds() { - core.Gx("proxy.health.TxRx."+pid, func() { p.Ping() }) } else if status != TOK { - core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() }) + core.Gx("proxy.health.TOK."+pid, func() { p.Ping() }) } return nil // ok From a419e042f36c24bf659bebecde5d4d83481deb62 Mon Sep 17 00:00:00 2001 From: Varun Agarwal Date: Thu, 10 Sep 2026 00:06:34 +0530 Subject: [PATCH 3/3] =?UTF-8?q?=EF=BB=BFRevert=20"netstack/icmp:=20prevent?= =?UTF-8?q?=20icmp=20floods"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit b33dbb88e093782ca19169c4566f0b9a2f416423. StreamShield investigation into a Zee5 playback hang (video CDN TCP connections dying with "endpoint is closed for send" on the direct/ Exit proxy path) is bisecting this commit as the second of two remaining suspects in the celzero/firestack 61894b7fdb..8677a52cbd range, after ruling out and fixing the proxyFor lock-read timeout regression (315d2335, see StreamShield PR celzero/firestack#248). This commit added a stack-wide ICMP rate limiter (10/s, burst 7) plus a per-source ICMP echo tarpit/stall (sleeps up to 5s per ping once a source exceeds 10 pings in a 10s window) in the netstack ICMP path. Reverting to test whether this new rate limiting/stalling logic is interacting badly with concurrent TCP dialing/health-probing on the same netstack instance, or otherwise contributing to the observed TCP write failures on unrelated connections. Not confirmed as root cause; reverted as part of a bisection A/B test. --- intra/core/expiringmap.go | 7 ------- intra/icmp.go | 24 ------------------------ intra/netstack/icmp.go | 14 -------------- intra/netstack/icmpecho.go | 6 ------ intra/netstack/stackopts.go | 14 -------------- 5 files changed, 65 deletions(-) diff --git a/intra/core/expiringmap.go b/intra/core/expiringmap.go index c89ce3f6..eb77960e 100644 --- a/intra/core/expiringmap.go +++ b/intra/core/expiringmap.go @@ -107,13 +107,6 @@ func (m *ExpMap[P, Q]) Get(key P) uint32 { return v.hits } -func (m *ExpMap[P, Q]) SetMin(key P) uint32 { - if done(m.ctx) || m.minlife <= 0 { - return 0 - } - return m.Set(key, m.minlife) -} - // Set sets the expiry for the given key and returns the number of hits. // expiry is clamped to minlife. If the key was expired, its hit window // is reset to 0 before returning. Value is set to Q's zero value. diff --git a/intra/icmp.go b/intra/icmp.go index 844b08b1..e8e1c83b 100644 --- a/intra/icmp.go +++ b/intra/icmp.go @@ -24,23 +24,13 @@ import ( type icmpHandler struct { *baseHandler - staller *core.ExpMap[netip.AddrPort, string] // src(addr:port) -> stallSecs } var _ netstack.GICMPHandler = (*icmpHandler)(nil) -// pings allowed per source within icmpFloodTrackTTL, after -// which each ping is stalled for up to icmptarpitMaxSecs. -const ( - icmpFloodHits = 10 // pings allowed per source per window - icmptarpitMaxSecs = 5 // max stall per ping - icmpFloodTrackTTL = 10 * time.Second -) - func NewICMPHandler(pctx context.Context, resolver dnsx.Resolver, prox ipn.ProxyProvider, listener Listener) netstack.GICMPHandler { h := &icmpHandler{ baseHandler: newBaseHandler(pctx, "icmp", resolver, prox, listener), - staller: core.NewExpiringMapLifetime[netip.AddrPort, string](pctx, "icmp.staller", icmpFloodTrackTTL), } core.Gx("icmp.ps", h.processSummaries) @@ -49,14 +39,6 @@ func NewICMPHandler(pctx context.Context, resolver dnsx.Resolver, prox ipn.Proxy return h } -func (h *icmpHandler) maybeStall(src netip.AddrPort) (secs uint32) { - if n := h.staller.Get(src); n > icmpFloodHits { - secs = icmptarpitMaxSecs - } - h.staller.SetMin(src) // track for icmpFloodTrackTTL - return -} - // Ping implements netstack.GICMPHandler. Takes ownership of msg. // Nb: to send icmp pings, root access is required; and so, // send "unprivileged" icmp pings via udp reqs; which do @@ -112,12 +94,6 @@ func (h *icmpHandler) Ping(msg []byte, source, target netip.AddrPort) (echoed bo return false // denied } - // delay flooders; this fn is async, so stalling doesn't block dispatchers - if secs := h.maybeStall(source); secs > 0 { - log.I("t.icmp: flood: stalled %s => %s for %ds", source, target, secs) - time.Sleep(time.Duration(secs) * time.Second) - } - if px, err = h.prox.ProxyTo(cid, dst, "icmp", uid, pids); err != nil || px == nil { err = log.EE("t.icmp: egress: no proxy(%s); err %v", pids, err) return false // denied diff --git a/intra/netstack/icmp.go b/intra/netstack/icmp.go index 6670e36e..5abe7987 100644 --- a/intra/netstack/icmp.go +++ b/intra/netstack/icmp.go @@ -84,12 +84,6 @@ func (f *icmpForwarder) reply4(id stack.TransportEndpointID, pkt *stack.PacketBu log.D("icmp: v4: %s: type %v passthrough", f.o, hdr.Type()) return // not handled } - // consult the stack-wide ICMP rate limiter; see: stackopts.go:SetNetstackOpts - // github.com/google/gvisor/blob/738e1d995f/pkg/tcpip/network/ipv4/icmp.go - if !f.s.AllowICMPMessage() { - log.V("icmp: v4: %s: rate limited; dropping echo %s => %s", f.o, src, dst) - return true // handled (silently dropped) - } ipHdr := header.IPv4(l3hdr) replyData := stack.PayloadSince(pkt.TransportHeader()) localAddressBroadcast := pkt.NetworkPacketInfo.LocalAddressBroadcast @@ -203,14 +197,6 @@ func (f *icmpForwarder) reply6(id stack.TransportEndpointID, pkt *stack.PacketBu } l3 := pkt.Network() // l3.Dst == id.LocalAddr and l3.Src == id.RemoteAddr - - // consult the stack-wide ICMP rate limiter before; see: stackopts.go:SetNetstackOpts - // github.com/google/gvisor/blob/738e1d995f/pkg/tcpip/network/ipv6/icmp.go - if !f.s.AllowICMPMessage() { - log.V("icmp: v6: %s: rate limited; dropping echo %s => %s", f.o, l3.DestinationAddress(), l3.SourceAddress()) - return true // handled (silently dropped) - } - route, err := f.s.FindRoute(pkt.NICID, l3.DestinationAddress(), l3.SourceAddress(), pkt.NetworkProtocolNumber, false) if err != nil { log.W("icmp: v6: %s: no route on %v to %s <= %s", f.o, pkt.NICID, l3.DestinationAddress(), l3.SourceAddress()) diff --git a/intra/netstack/icmpecho.go b/intra/netstack/icmpecho.go index ba156c8a..4b2c28f4 100644 --- a/intra/netstack/icmpecho.go +++ b/intra/netstack/icmpecho.go @@ -217,12 +217,6 @@ func (r *icmpResponder) process(h *icmpForwarder, nic tcpip.NICID, pkt *wire.Par return } - // consult the stack-wide ICMP rate limiter before; see: stackopts.go:SetNetstackOpts - if h.s != nil && !h.s.AllowICMPMessage() { - logwv(true)("icmp: responder: rate limited; dropping ping %s => %s", src, dst) - return - } - pinged := h.h.Ping(icmpMsg, src, dst) resp, proto, l4proto, tag, err := r.echoReply(pkt, payload, pinged) diff --git a/intra/netstack/stackopts.go b/intra/netstack/stackopts.go index 9cad91f3..e301ec2d 100644 --- a/intra/netstack/stackopts.go +++ b/intra/netstack/stackopts.go @@ -7,7 +7,6 @@ package netstack import ( - "golang.org/x/time/rate" "gvisor.dev/gvisor/pkg/tcpip" "gvisor.dev/gvisor/pkg/tcpip/network/ipv4" "gvisor.dev/gvisor/pkg/tcpip/network/ipv6" @@ -15,20 +14,7 @@ import ( "gvisor.dev/gvisor/pkg/tcpip/transport/tcp" ) -const ( - // icmpPingLimit caps generated ICMP messages per second: - // Firestack client code must consult Stack.AllowICMPMessage() - // but it auto-applies to gVisor generated ICMP errors (ipv4.go:allowICMPReply) - icmpPingLimit = rate.Limit(10) - // icmpPingBurst caps the initial burst: - // Firestack client code must consult Stack.AllowICMPMessage() - icmpPingBurst = 7 -) - func SetNetstackOpts(s *stack.Stack) { - s.SetICMPLimit(icmpPingLimit) - s.SetICMPBurst(icmpPingBurst) - // TODO: other stack otps? // github.com/xjasonlyu/tun2socks/blob/31468620e/core/option/option.go#L69