Compare commits

..
Author SHA1 Message Date
omarandClaude Opus 5 8fd5c52488 fix(tproxy): connect the UDP write-back socket + release NAT sessions on close
release / aarch64_cortex-a53 (push) Successful in 3m29s
release / x86_64 (push) Successful in 3m21s
release / apk aarch64_cortex-a53 (push) Successful in 2m38s
release / apk x86_64 (push) Successful in 2m35s
release / release (push) Successful in 8s
release / release apk (push) Successful in 6s
B3, real root cause. On the live BPi-R3 Mini `netstat -lnup` showed shaterd
holding 33 sockets on the router's own LAN address 10.67.0.1:53, next to
dnsmasq's single socket, several with a growing Recv-Q. Reproduced read-only on
the box: 5 host queries to 10.67.0.1 -> 0 answers and total Recv-Q on those
sockets 0 -> 19200 (5 x 3840, one datagram parked in each, never read); 3
control queries to 127.0.0.1 -> all answered.

Where they come from: protocol/redirect/tproxy.go, tproxyPacketWriter.
WritePacket. The TPROXY UDP write-back socket must carry the ORIGINAL
DESTINATION as its source address, so upstream binds it there — but leaves it
UNCONNECTED (net.ListenPacket + WriteToUDPAddrPort) and sets SO_REUSEADDR AND
SO_REUSEPORT (sing's control.ReuseAddr sets both). An unconnected bound socket
is a RECEIVER as far as the kernel is concerned, so each one silently joins the
UDP demultiplex/reuseport set for that address:port. Nothing ever reads them —
this writer only sends.

With dns_intercept the original destination IS the router's LAN address, so
every intercepted DNS session parks another silent receiver on <lan-ip>:53. The
host's own queries to that address take the loopback path, are never diverted by
the nft plane (iifname is scoped to LAN devices), and are therefore spread across
that set by the reuseport 4-tuple hash: they land in a silent socket at random
and time out. Hence "2 restarts of 3 fine, the third dead", and hence a failure
that no ruleset rebuild or reconcile can touch. The stale [UNREPLIED] conntrack
entry seen alongside is a CONSEQUENCE of the unanswered query, not the cause.

Fix (upstream file, lx:tproxy_writeback_connect):
  * CONNECT the write-back socket to the one peer it ever talks to. The kernel's
    compute_score() rejects a connected socket for any other peer, and a
    connected UDP socket (sk_state == TCP_ESTABLISHED) is excluded from
    reuseport selection outright — so it can no longer be handed a datagram it
    will not read. Nothing about the reply changes: same spoofed source, same
    single peer, Write instead of WriteTo. The unconnected path is kept verbatim
    for a destination that cannot be bound (domain socksaddr).
  * A failed cached write now CLOSES the socket instead of only dropping the
    reference (upstream left the fd to the GC finalizer).
  * TProxy.Close() purges the UDP NAT cache. Closing the listener stops ingress
    but the cache evicts lazily, so after the inbound is gone nothing wakes the
    live sessions and each strands its write-back socket. Invisible upstream
    (one close at shutdown); on this fork the engine is rebuilt on every apply,
    so it was one stranded generation per apply.

Measured on the live box: the socket count is steady-state (22-40, fds 55-66),
i.e. bounded by the udpnat session lifetime rather than an unbounded leak — the
count itself is inherent to per-session write-back sockets and is harmless once
they are connected. The Close() purge removes the per-apply generations on top
of it.

The netplane UDP:53 conntrack flush from 32e8f8ff0 is KEPT, with its comment
corrected: it is hygiene on plane transitions, not the cure for B3.

Regression tests fail on the pre-fix code (verified by reverting each half):
TestWriteBackUsesConnectedSocket / TestWriteBackReusesOneSocket /
TestWriteBackClosesSocketOnWriteFailure ("use of WriteTo with pre-connected
connection") and TestTProxyCloseReleasesNatSessions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-07-25 13:41:02 +03:00
3 changed files with 349 additions and 7 deletions
+96 -2
View File
@@ -16,6 +16,7 @@ import (
"github.com/sagernet/sing/common"
"github.com/sagernet/sing/common/buf"
"github.com/sagernet/sing/common/control"
E "github.com/sagernet/sing/common/exceptions" // lx:tproxy_writeback_connect
M "github.com/sagernet/sing/common/metadata"
N "github.com/sagernet/sing/common/network"
"github.com/sagernet/sing/common/udpnat2"
@@ -68,7 +69,26 @@ func (t *TProxy) Start(stage adapter.StartStage) error {
}
func (t *TProxy) Close() error {
return t.listener.Close()
err := t.listener.Close()
// lx:begin tproxy_writeback_connect
// Closing the listener stops INGRESS but leaves every live UDP NAT session in
// the cache, and each session holds a write-back socket bound to its original
// destination. Nothing else ever wakes those sessions: the cache evicts
// lazily (on the next Get/Add), and after this inbound is gone there is no
// next Get. For a DNS session answered in-engine by hijack-dns there is not
// even an outbound connection whose failure could unwind it, so its socket
// survives until a GC finalizer happens to reach it.
//
// That is invisible upstream, where an inbound is closed once at shutdown. On
// this fork the engine is REBUILT on every config apply, so each apply would
// strand another generation of sockets on the router's own LAN :53. Purge
// evicts every session; the cache's OnEvict closes the conn, which unblocks
// the session's routing goroutine and runs its onClose — the one place the
// write-back socket is actually closed. Purge AFTER the listener so a packet
// arriving mid-teardown cannot re-create a session behind us.
t.udpNat.Purge()
// lx:end tproxy_writeback_connect
return err
}
func (t *TProxy) NewConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) {
@@ -121,18 +141,92 @@ type tproxyPacketWriter struct {
conn *net.UDPConn
}
// lx:begin tproxy_writeback_connect
//
// The TPROXY UDP write-back socket is bound to the ORIGINAL DESTINATION, so the
// client sees the reply coming from the address it addressed. Upstream leaves
// that socket UNCONNECTED (net.ListenPacket + WriteToUDPAddrPort), and that is
// the bug this block exists for.
//
// An unconnected socket bound to <addr>:<port> is, as far as the kernel is
// concerned, a RECEIVER for that address:port — and because the socket also
// carries SO_REUSEADDR it silently joins the UDP demultiplex set of whatever
// else is bound there. Nothing ever reads from it: this writer only sends. So
// every datagram the kernel happens to hand it is lost.
//
// On a router that transparently intercepts LAN DNS towards its OWN address
// (`nft ... udp dport 53 tproxy ...`) the original destination IS the router's
// LAN address, so each intercepted DNS session parks another silent receiver on
// <router-lan-ip>:53 right next to dnsmasq's socket — observed on the stand:
// 33 such sockets against dnsmasq's one, several with a growing Recv-Q. The
// host's own queries to that address take the loopback path, are never diverted,
// and are therefore demultiplexed among all of them: they land in one of the
// silent sockets at random and time out, permanently and unpredictably, while
// every other DNS path on the box keeps working.
//
// Connecting the socket fixes it at the root. compute_score() in the kernel's
// UDP lookup REJECTS a connected socket for any peer other than the connected
// one, so a write-back socket can no longer be handed a datagram it will not
// read. Nothing about the reply changes — same spoofed source address, same
// single peer, one Write instead of one WriteTo.
//
// The unconnected path is kept verbatim for a destination that cannot be bound
// (a domain socksaddr), so no existing case regresses.
// newTProxyWriteBack creates the connected write-back socket: local address =
// the original destination (transparent bind), peer = the client. A package
// variable so the behaviour can be tested without CAP_NET_ADMIN.
var newTProxyWriteBack = func(w *tproxyPacketWriter, destination M.Socksaddr) (*net.UDPConn, error) {
var dialer net.Dialer
dialer.LocalAddr = destination.UDPAddr()
dialer.Control = control.Append(dialer.Control, control.ReuseAddr())
dialer.Control = control.Append(dialer.Control, redir.TProxyWriteBack())
conn, err := w.listener.DialContext(dialer, w.ctx, "udp", w.source.String())
if err != nil {
return nil, err
}
udpConn, loaded := conn.(*net.UDPConn)
if !loaded {
conn.Close()
return nil, E.New("tproxy write back: unexpected connection type ", conn)
}
return udpConn, nil
}
// lx:end tproxy_writeback_connect
func (w *tproxyPacketWriter) WritePacket(buffer *buf.Buffer, destination M.Socksaddr) error {
defer buffer.Release()
if w.listener.ListenOptions().NetNs == "" {
conn := w.conn
if w.destination == destination && conn != nil {
_, err := conn.WriteToUDPAddrPort(buffer.Bytes(), w.source)
// lx:begin tproxy_writeback_connect
// The cached socket is CONNECTED to the client, so this is a plain
// Write. A failed write also closes it: upstream only dropped the
// reference, leaving the fd to the GC finalizer.
_, err := conn.Write(buffer.Bytes())
if err != nil {
conn.Close()
w.conn = nil
}
return err
// lx:end tproxy_writeback_connect
}
}
// lx:begin tproxy_writeback_connect
if destination.IsIP() {
udpConn, err := newTProxyWriteBack(w, destination)
if err != nil {
return err
}
if w.listener.ListenOptions().NetNs == "" && w.destination == destination {
w.conn = udpConn
} else {
defer udpConn.Close()
}
return common.Error(udpConn.Write(buffer.Bytes()))
}
// lx:end tproxy_writeback_connect
var listenConfig net.ListenConfig
listenConfig.Control = control.Append(listenConfig.Control, control.ReuseAddr())
listenConfig.Control = control.Append(listenConfig.Control, redir.TProxyWriteBack())
@@ -0,0 +1,234 @@
package redirect
// Regression cover for the lx:tproxy_writeback_connect block in tproxy.go.
//
// The failure it guards against, seen on a live BPi-R3 Mini: `shaterd` held 33
// UNCONNECTED UDP sockets on the router's own LAN address :53 — the write-back
// sockets of intercepted DNS sessions — alongside dnsmasq's single socket on the
// same address:port. Nothing reads a write-back socket, so every host-originated
// query that the kernel demultiplexed into one of them was silently dropped
// (Recv-Q climbing, sender timing out). Connecting the socket to the one peer it
// ever talks to removes it from the demultiplex set for everybody else.
//
// The real socket needs CAP_NET_ADMIN (IP_TRANSPARENT) and a foreign bind, so
// these tests drive the seam (newTProxyWriteBack) with an ordinary connected
// loopback socket. That is enough to pin both properties that actually broke:
//
// - the write path uses the CONNECTED form. If WritePacket ever goes back to
// WriteToUDPAddrPort, Go returns ErrWriteToConnected on a connected socket
// and these tests fail — i.e. the test cannot pass with an unconnected
// write-back socket, which is exactly the regression.
// - repeated writes to the same destination REUSE one socket instead of
// accumulating a new one per packet.
import (
"context"
"net"
"net/netip"
"testing"
"time"
"github.com/sagernet/sing-box/common/listener"
"github.com/sagernet/sing-box/option"
"github.com/sagernet/sing/common/buf"
M "github.com/sagernet/sing/common/metadata"
N "github.com/sagernet/sing/common/network"
"github.com/sagernet/sing/common/udpnat2"
)
// writeBackHarness stands up a loopback "client" socket and a tproxyPacketWriter
// whose socket factory returns a plain connected UDP socket aimed at it. It
// returns the writer, the client socket and a pointer to the factory call count.
func writeBackHarness(t *testing.T) (*tproxyPacketWriter, *net.UDPConn, *int) {
t.Helper()
client, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1)})
if err != nil {
t.Fatalf("client socket: %v", err)
}
t.Cleanup(func() { client.Close() })
source := netip.MustParseAddrPort(client.LocalAddr().String())
w := &tproxyPacketWriter{
ctx: context.Background(),
source: source,
// A listener with empty options is enough: WritePacket only reads
// ListenOptions().NetNs, and the factory is stubbed below.
listener: listener.New(listener.Options{
Context: context.Background(),
Listen: option.ListenOptions{},
}),
destination: M.SocksaddrFrom(netip.MustParseAddr("127.0.0.1"), 53),
}
calls := 0
orig := newTProxyWriteBack
t.Cleanup(func() { newTProxyWriteBack = orig })
newTProxyWriteBack = func(w *tproxyPacketWriter, destination M.Socksaddr) (*net.UDPConn, error) {
calls++
// The real implementation binds the ORIGINAL DESTINATION transparently
// and connects to w.source; here we only reproduce the connect half,
// which is the property under test.
conn, err := net.DialUDP("udp", nil, net.UDPAddrFromAddrPort(w.source))
if err != nil {
return nil, err
}
return conn, nil
}
return w, client, &calls
}
// readOne reads one datagram from the client socket with a short deadline.
func readOne(t *testing.T, client *net.UDPConn) string {
t.Helper()
_ = client.SetReadDeadline(time.Now().Add(2 * time.Second))
b := make([]byte, 512)
n, _, err := client.ReadFrom(b)
if err != nil {
t.Fatalf("client read: %v", err)
}
return string(b[:n])
}
// TestWriteBackUsesConnectedSocket: the reply must go out over a CONNECTED
// socket. On the pre-fix code the write is WriteToUDPAddrPort, which Go refuses
// on a connected socket ("use of WriteTo with pre-connected connection"), so
// this test is red for exactly the shape that caused the outage.
func TestWriteBackUsesConnectedSocket(t *testing.T) {
w, client, calls := writeBackHarness(t)
if err := w.WritePacket(buf.As([]byte("first")).ToOwned(), w.destination); err != nil {
t.Fatalf("WritePacket: %v", err)
}
if got := readOne(t, client); got != "first" {
t.Fatalf("client got %q, want %q", got, "first")
}
if *calls != 1 {
t.Fatalf("expected one write-back socket, got %d", *calls)
}
}
// TestWriteBackReusesOneSocket: a session that keeps answering the same client
// must keep ONE socket, not open a fresh one per datagram. The live box carried
// one socket per intercepted DNS session already; one per PACKET would turn a
// nuisance into an fd exhaustion.
func TestWriteBackReusesOneSocket(t *testing.T) {
w, client, calls := writeBackHarness(t)
for i, payload := range []string{"a", "b", "c", "d"} {
if err := w.WritePacket(buf.As([]byte(payload)).ToOwned(), w.destination); err != nil {
t.Fatalf("WritePacket %d: %v", i, err)
}
if got := readOne(t, client); got != payload {
t.Fatalf("packet %d: client got %q, want %q", i, got, payload)
}
}
if *calls != 1 {
t.Fatalf("four packets to one destination must share one socket, got %d sockets", *calls)
}
if w.conn == nil {
t.Fatalf("the write-back socket must be cached on the writer for reuse")
}
}
// TestWriteBackClosesSocketOnWriteFailure: upstream dropped the reference to a
// failed socket without closing it, leaving the fd to the GC finalizer. On a box
// that already parks one socket per DNS session that is the wrong direction.
func TestWriteBackClosesSocketOnWriteFailure(t *testing.T) {
w, client, _ := writeBackHarness(t)
if err := w.WritePacket(buf.As([]byte("warm")).ToOwned(), w.destination); err != nil {
t.Fatalf("WritePacket: %v", err)
}
readOne(t, client)
cached := w.conn
if cached == nil {
t.Fatalf("expected a cached socket after the first write")
}
// Close it behind WritePacket's back so the next write fails, exactly as a
// dead peer or a torn-down plane would make it fail.
cached.Close()
if err := w.WritePacket(buf.As([]byte("boom")).ToOwned(), w.destination); err == nil {
t.Fatalf("a write on a closed socket must report the failure")
}
if w.conn != nil {
t.Fatalf("a failed write must drop the cached socket")
}
// A second Close on an already-closed conn is an error, which is how we know
// WritePacket closed it rather than merely forgetting it.
if err := cached.Close(); err == nil {
t.Fatalf("WritePacket must CLOSE the failed socket, not just nil the field")
}
}
// --- Close() must release the UDP NAT sessions --------------------------------
// natSessionHandler stands in for the router: it drains the session conn until
// it errors (which is what Close does to it) and then reports onClose, exactly
// as the real routing goroutine does. onClose is where the write-back socket is
// closed, so "onClose fired" is the observable proof the socket was released.
type natSessionHandler struct {
released chan error
}
func (h *natSessionHandler) NewPacketConnectionEx(ctx context.Context, conn N.PacketConn, source M.Socksaddr, destination M.Socksaddr, onClose N.CloseHandlerFunc) {
go func() {
for {
buffer := buf.NewSize(1024)
_, err := conn.ReadPacket(buffer)
buffer.Release()
if err != nil {
if onClose != nil {
onClose(err)
}
h.released <- err
return
}
}
}()
}
// TestTProxyCloseReleasesNatSessions pins the apply-level half of the leak.
//
// Closing the inbound used to close the listener only. The NAT cache evicts
// lazily, so after the inbound is gone nothing ever touches it again and every
// live session — with the write-back socket it holds on the router's own
// LAN :53 — was left to a GC finalizer. On this fork the engine is rebuilt on
// every config apply, so that is one stranded generation of sockets per apply.
func TestTProxyCloseReleasesNatSessions(t *testing.T) {
handler := &natSessionHandler{released: make(chan error, 1)}
tp := &TProxy{
ctx: context.Background(),
listener: listener.New(listener.Options{
Context: context.Background(),
Listen: option.ListenOptions{},
}),
}
prepared := 0
tp.udpNat = udpnat.New(handler, func(source M.Socksaddr, destination M.Socksaddr, userData any) (bool, context.Context, N.PacketWriter, N.CloseHandlerFunc) {
prepared++
return true, context.Background(), nil, func(error) {}
}, time.Minute, false)
tp.udpNat.NewPacket(
[][]byte{{0x00}},
M.SocksaddrFrom(netip.MustParseAddr("10.67.0.2"), 40000),
M.SocksaddrFrom(netip.MustParseAddr("10.67.0.1"), 53),
nil,
)
if prepared != 1 {
t.Fatalf("expected one NAT session, got %d", prepared)
}
if err := tp.Close(); err != nil {
t.Fatalf("Close: %v", err)
}
select {
case <-handler.released:
case <-time.After(5 * time.Second):
t.Fatal("Close must release every live NAT session (and with it the write-back socket it holds)")
}
}
+19 -5
View File
@@ -21,11 +21,25 @@ package netplane
// flow that crosses one of those windows keeps a conntrack entry that was formed
// against a plane that no longer exists, and — for UDP, which has no handshake to
// resynchronise on — every retry merely refreshes that entry instead of
// re-deriving the path. The flow stays wedged for as long as the client keeps
// asking, which is exactly the shape of the "DNS to the router's own LAN address
// never comes back after `/etc/init.d/shater restart`" report: one flow family
// dead, everything else healthy, `plane=full`, and a reconcile that fixes nothing
// because there is nothing in the RULESET left to fix.
// re-deriving the path.
//
// # What this is NOT
//
// This flush is HYGIENE, not the cure for the "DNS to the router's own LAN
// address never comes back after a restart" report (B3). That turned out to be a
// socket-level collision: the engine's TPROXY UDP write-back sockets were bound
// UNCONNECTED to the original destination — the router's own LAN address :53 —
// and so joined the kernel's demultiplex set next to dnsmasq's socket, silently
// swallowing the host's own queries. The fix for that lives in the engine
// (protocol/redirect/tproxy.go, lx:tproxy_writeback_connect); the stale
// `[UNREPLIED]` conntrack entry seen on the live box was a CONSEQUENCE of the
// unanswered query, not its cause.
//
// It is kept because it is independently correct and costs one netlink
// round-trip per plane change: a DNS flow that was mid-flight across a plane
// rebuild has a conntrack entry describing a delivery path that no longer
// exists, and dropping it makes the first query after a restart re-derive its
// path immediately instead of waiting out a retry.
//
// # Scope: :53/UDP only, deliberately
//