fix(restart): serialise stop->start and flush stale DNS conntrack (B3)
`/etc/init.d/shater restart` left DNS to the router's own LAN address dead
and never recovering, while `stop` + pause + `start` was fine — with the
status still reporting plane=full / engine_running=true and `shaterd
reconcile` fixing nothing.
Cause: `restart` is not synchronised end to end.
* procd's `stop` is ASYNCHRONOUS. rc.common's `restart` is literally
`stop; start`, and the `service delete` ubus call returns as soon as
SIGTERM has been SENT. `start_service` therefore re-adds the instance
(and runs `shaterd migrate`) while the outgoing `shaterd run` is still
executing its honest teardown.
* The successor's only defence was `daemonAlive()` -> exit(1), leaning on
procd's `respawn 3600 5 0` to try again five seconds later. That is a
blind retry, not synchronisation: it neither knows nor waits for the
teardown, and it turns every restart into a logged crash plus a
five-second hole with no data plane.
* `term_timeout 10` SIGKILLs a predecessor whose teardown outlives it —
engine.Close of a several-hundred-outbound box flushes cache.db to
flash before the netplane teardown even starts — aborting the teardown
at an arbitrary point and leaving the plane HALF removed.
* Nothing in the tree ever touched conntrack, so flows that crossed one
of those windows kept entries formed against a plane that no longer
exists. For UDP there is no handshake to resynchronise on and every
retry merely refreshes the entry, so the flow stays wedged for as long
as the client keeps asking — a flow-scoped, permanent failure that no
ruleset rebuild can reach.
* RoutingPresent() reported "plane intact" from the ip RULE alone, while
ApplyRouting installs a rule AND a `local default dev lo` route removed
by two independent commands. A teardown interrupted between them was
therefore invisible, applyLocked's fast-path skipped ApplyRouting
forever, and no reconcile could repair it.
Fix (fail-closed posture unchanged — no new window in which LAN traffic can
reach the WAN; teardown still removes the table LAST and the forward-chain
drop is untouched):
* init: `start_service` waits for a live predecessor pidfile to clear
before opening the instance, so restart == stop + pause + start. Zero
cost at boot. term_timeout 10 -> 30 so an honest teardown is never
killed halfway.
* daemon: the single-owner guard WAITS for the predecessor (bounded,
60s) instead of exiting 1; it still refuses if the budget expires.
* netplane: new FlushDNSConntrack() (ctnetlink, UDP orig-dport 53 only —
a blanket flush would drop the admin's own SSH/LuCI sessions) called
on every plane transition: after a ruleset loads, after the table is
removed, and once more in applyLocked when the whole plane (table +
policy routing + sysctls) is assembled.
* netplane: RoutingPresent() now verifies both halves it installs.
Regression tests fail on the pre-fix code (verified by reverting each fix):
TestApplyNftFlushesDNSConntrack, TestTeardownNftFlushesDNSConntrack,
TestRoutingPresentRequiresLocalDefaultRoute, TestWaitForPredecessor*.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -47,6 +47,13 @@ PROG=/usr/bin/shaterd
|
||||
# hotplug/shater-cron touch the data plane. tmpfs => cleared by reboot, so
|
||||
# nothing reconciles before this init has run at boot.
|
||||
ACTIVE_FLAG=/var/run/shater.active
|
||||
# Written by `shaterd run`; the single-owner token this init waits on so a
|
||||
# restart never overlaps a new data plane with the previous one's teardown.
|
||||
PIDFILE=/var/run/shaterd.pid
|
||||
# Seconds `start` will wait for a predecessor to finish its teardown. Must be
|
||||
# >= term_timeout below (procd's hard cap on a predecessor's life after SIGTERM)
|
||||
# so we never give up while procd is still letting it shut down cleanly.
|
||||
STOP_WAIT_SECS=40
|
||||
|
||||
# --- helpers ---------------------------------------------------------------
|
||||
|
||||
@@ -66,6 +73,54 @@ _slog() {
|
||||
[ "$(uci -q get shater.globals.log_syslog)" = "0" ] || logger -t shater "$@"
|
||||
}
|
||||
|
||||
# Echo the pid of a LIVE `shaterd run`, or fail. The pidfile is written by the
|
||||
# daemon itself and removed only by the daemon that owns it, AFTER its teardown
|
||||
# has completed — so "pidfile names a live process" is precisely "the previous
|
||||
# data plane has not been dismantled yet".
|
||||
shater_daemon_pid() {
|
||||
local pid
|
||||
pid=$(cat "$PIDFILE" 2>/dev/null) || return 1
|
||||
[ -n "$pid" ] || return 1
|
||||
kill -0 "$pid" 2>/dev/null || return 1
|
||||
echo "$pid"
|
||||
}
|
||||
|
||||
# Block until no predecessor daemon is left, bounded by STOP_WAIT_SECS.
|
||||
#
|
||||
# WHY THIS EXISTS. procd's `stop` is ASYNCHRONOUS: rc.common's `restart` is
|
||||
# literally `stop; start`, and the `service delete` ubus call returns the moment
|
||||
# procd has SENT SIGTERM — not when the instance is gone. `start` therefore
|
||||
# re-adds the instance while the outgoing `shaterd run` is still executing its
|
||||
# honest teardown (engine close, then `nft delete table`, `ip rule`/`ip route`
|
||||
# removal and the per-iface sysctl restore). The result is that `restart` is NOT
|
||||
# equivalent to `stop` + pause + `start`: the new plane is stood up on top of
|
||||
# kernel state the old one has not finished removing, which is what B3 (DNS to
|
||||
# the router's own LAN address dead after a restart, and never recovering) came
|
||||
# out of. Waiting here restores the equivalence, and costs literally nothing when
|
||||
# there is no predecessor — the check runs before the first sleep.
|
||||
#
|
||||
# Returning non-zero does NOT abort the start: the daemon carries its own
|
||||
# single-owner guard and will refuse (or wait) on its side. Better to hand the
|
||||
# decision to the process that can actually see the plane than to leave the box
|
||||
# with no service at all.
|
||||
shater_wait_stopped() {
|
||||
local i=0 pid
|
||||
pid=$(shater_daemon_pid) || return 0
|
||||
_slog -p daemon.info \
|
||||
"restart: waiting for the previous shaterd (pid $pid) to finish tearing the data plane down"
|
||||
while [ "$i" -lt "$STOP_WAIT_SECS" ]; do
|
||||
sleep 1
|
||||
i=$((i + 1))
|
||||
shater_daemon_pid >/dev/null || {
|
||||
_slog -p daemon.info "restart: previous shaterd exited after ${i}s; starting a fresh one"
|
||||
return 0
|
||||
}
|
||||
done
|
||||
_slog -p daemon.warn \
|
||||
"restart: previous shaterd (pid $pid) still alive after ${STOP_WAIT_SECS}s — starting anyway"
|
||||
return 1
|
||||
}
|
||||
|
||||
# --- procd lifecycle -------------------------------------------------------
|
||||
|
||||
start_service() {
|
||||
@@ -87,6 +142,14 @@ start_service() {
|
||||
return 0
|
||||
fi
|
||||
|
||||
# Do not stand a new data plane up on top of one that is still being taken
|
||||
# down. On `restart` procd has only just SIGTERMed the previous instance and
|
||||
# returned; this is the handshake that makes `restart` == `stop` + pause +
|
||||
# `start`. It also keeps `migrate` below from rewriting UCI underneath a
|
||||
# daemon that is still reading it. No-op (and no delay) when nothing is
|
||||
# running, which is the boot case.
|
||||
shater_wait_stopped
|
||||
|
||||
# Bring the UCI schema forward before the daemon reads it (idempotent;
|
||||
# refuses a newer schema) so an upgraded package never applies a stale config.
|
||||
"$PROG" migrate >/dev/null 2>&1
|
||||
@@ -111,7 +174,16 @@ start_service() {
|
||||
procd_set_param stderr 1
|
||||
# Give the daemon room to run its honest teardown (engine.Close + netplane
|
||||
# restore) before procd SIGKILLs it.
|
||||
procd_set_param term_timeout 10
|
||||
#
|
||||
# 30s, not 10s: an engine holding a few hundred outbounds closes its
|
||||
# urltest/observatory goroutines and flushes experimental.cache_file to FLASH
|
||||
# before the netplane teardown even starts, and on eMMC/NAND that alone can
|
||||
# outlast 10s. A SIGKILL there aborts the teardown at an arbitrary point and
|
||||
# leaves the plane HALF removed — the nft table gone but the policy routing
|
||||
# still installed, or vice versa — which is precisely the class of leftover
|
||||
# state the successor's idempotent fast-path cannot see and never repairs.
|
||||
# Shutdown is bounded by procd either way; we are only choosing where.
|
||||
procd_set_param term_timeout 30
|
||||
procd_close_instance
|
||||
|
||||
# Mark the stack live for hotplug/cron — but ONLY when interception is
|
||||
@@ -141,10 +213,12 @@ stop_service() {
|
||||
reload_service() {
|
||||
# Fired by the `shater` config.change reload-trigger (LuCI Save & Apply /
|
||||
# reload_config). Simplest correct behaviour: stop + start. `stop` clears the
|
||||
# flag and SIGTERMs the daemon (honest teardown); `start` re-guards on
|
||||
# enabled and, if still enabled, launches a fresh `shaterd run` that reads
|
||||
# the new UCI and applies it. When the stack is disabled, `start` is a no-op,
|
||||
# so a disable+apply cleanly tears everything down.
|
||||
# flag and SIGTERMs the daemon (honest teardown); `start` WAITS for that
|
||||
# teardown to actually finish (shater_wait_stopped) and then launches a fresh
|
||||
# `shaterd run` that reads the new UCI and applies it. When the stack is
|
||||
# disabled, `start` is a no-op, so a disable+apply cleanly tears everything
|
||||
# down. Because the wait lives in start_service, this path gets the same
|
||||
# stop-then-start ordering guarantee as `restart`.
|
||||
stop
|
||||
start
|
||||
}
|
||||
|
||||
@@ -488,6 +488,23 @@ func (a *Applier) applyLocked(m *model.Model) (bool, error) {
|
||||
return changed, err
|
||||
}
|
||||
|
||||
// The plane is COMPLETE only here: table + policy routing + sysctls. ApplyNft
|
||||
// already flushed the DNS conntrack when it loaded the ruleset, but that is
|
||||
// one step too early — ApplyRouting is idempotent BY del-then-add, so it opens
|
||||
// a window in which the fwmark rule is momentarily absent, and any DNS flow
|
||||
// that crosses that window is tracked against a plane that is still being
|
||||
// assembled. Flushing once more now that every piece is in place is what makes
|
||||
// "no entry survives the transition" actually true. Only on a real change (the
|
||||
// fast path assembled nothing), best-effort, and cheap: the :53 entry count is
|
||||
// bounded by the number of clients.
|
||||
if !nftCurrent {
|
||||
if n, ferr := netplane.FlushDNSConntrack(); ferr != nil {
|
||||
a.log.Debug("flush DNS conntrack after plane change: ", ferr)
|
||||
} else if n > 0 {
|
||||
a.log.Debug("plane changed: dropped ", n, " stale DNS conntrack entries")
|
||||
}
|
||||
}
|
||||
|
||||
// (4) success. Bump the effective-state generation ONLY when something really
|
||||
// moved: a no-op reconcile must not invalidate an armed commit-confirm window
|
||||
// (cron reconciles every minute — counting those would cancel every rollback
|
||||
|
||||
@@ -175,10 +175,23 @@ func cmdRun() int {
|
||||
logFactory.SetLevel(controlLogLevel(g.LogLevel))
|
||||
}
|
||||
|
||||
// Refuse to start a second daemon (race-safe single-owner guard). procd's
|
||||
// term_timeout ensures the previous instance exits before reload restarts us.
|
||||
if pid, ok := daemonAlive(); ok && pid != os.Getpid() {
|
||||
logger.Error("another shaterd is already running (pid ", pid, ") — refusing to start")
|
||||
// Single-owner guard: never run two daemons at once. On a `restart` procd's
|
||||
// `service delete` is ASYNCHRONOUS — the ubus call returns immediately while
|
||||
// the outgoing instance is still running its honest teardown — and the
|
||||
// following `service add` starts us straight away, so a predecessor being
|
||||
// alive here is the NORMAL restart case, not an error.
|
||||
//
|
||||
// This used to exit(1) on the spot and lean on procd's `respawn ... 5 ...` to
|
||||
// try again five seconds later. That is a blind retry, not synchronisation:
|
||||
// it neither knows nor waits for the predecessor's teardown to finish, and it
|
||||
// turns every restart into at least one logged crash plus a five-second hole
|
||||
// in which the LAN has no plane at all. Waiting for the predecessor to exit
|
||||
// makes `restart` behave exactly like `stop` + pause + `start`: our apply is
|
||||
// then strictly ordered AFTER the previous teardown, which is the whole point
|
||||
// of the guard.
|
||||
if pid, waiting := waitForPredecessor(predecessorBudget, predecessorPoll, logger); waiting {
|
||||
logger.Error("another shaterd is still running (pid ", pid, ") after waiting ",
|
||||
predecessorBudget, " for it to exit — refusing to start")
|
||||
return 1
|
||||
}
|
||||
if err := writePidfile(); err != nil {
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
package main
|
||||
|
||||
// The startup handshake that stops a restarting daemon from standing a new data
|
||||
// plane up on top of the previous one's teardown.
|
||||
//
|
||||
// Deliberately free of build tags: the logic is pure timing and is exercised by
|
||||
// the tests on any host, while the probe it polls (pidfile + kill(pid, 0)) is
|
||||
// linux-only and is installed by predecessor_linux.go.
|
||||
|
||||
import (
|
||||
"os"
|
||||
"time"
|
||||
|
||||
"github.com/sagernet/sing-box/log"
|
||||
)
|
||||
|
||||
// predecessorBudget / predecessorPoll bound the wait for an outgoing daemon.
|
||||
//
|
||||
// The budget must comfortably exceed the slowest honest teardown (engine.Close
|
||||
// of a box with a few hundred outbounds plus the cache-file flush to flash, then
|
||||
// the nft/ip/sysctl removal) AND the init script's procd term_timeout, which is
|
||||
// the hard cap on how long a predecessor can live after its SIGTERM. Overshooting
|
||||
// costs nothing on a healthy box — the wait ends the instant the predecessor is
|
||||
// gone — while undershooting reintroduces the very overlap this exists to remove.
|
||||
//
|
||||
// Variables, not constants, so the tests can drive the loop without sleeping.
|
||||
var (
|
||||
predecessorBudget = 60 * time.Second
|
||||
predecessorPoll = 100 * time.Millisecond
|
||||
)
|
||||
|
||||
// aliveProbe is the seam waitForPredecessor polls. The portable default reports
|
||||
// "no predecessor", so a non-linux build (where there is no daemon at all) never
|
||||
// waits; predecessor_linux.go replaces it with the real pidfile probe.
|
||||
var aliveProbe = func() (int, bool) { return 0, false }
|
||||
|
||||
// waitForPredecessor blocks until no OTHER shaterd owns the pidfile, or until the
|
||||
// budget runs out.
|
||||
//
|
||||
// Returns (pid, true) only when a predecessor is STILL alive once the budget
|
||||
// expires — the genuine "two daemons" error the caller refuses on. A predecessor
|
||||
// that exits within the budget (the restart case) returns (0, false) and startup
|
||||
// continues, now guaranteed to be sequenced after its teardown, exactly as it is
|
||||
// after a manual `stop` + pause + `start`.
|
||||
func waitForPredecessor(budget, poll time.Duration, logger log.ContextLogger) (int, bool) {
|
||||
pid, ok := aliveProbe()
|
||||
if !ok || pid == os.Getpid() {
|
||||
return 0, false
|
||||
}
|
||||
if logger != nil {
|
||||
logger.Info("a previous shaterd (pid ", pid, ") is still shutting down — ",
|
||||
"waiting for its teardown to finish before applying a new data plane")
|
||||
}
|
||||
deadline := time.Now().Add(budget)
|
||||
for time.Now().Before(deadline) {
|
||||
time.Sleep(poll)
|
||||
pid, ok = aliveProbe()
|
||||
if !ok || pid == os.Getpid() {
|
||||
return 0, false
|
||||
}
|
||||
}
|
||||
return pid, true
|
||||
}
|
||||
@@ -0,0 +1,9 @@
|
||||
//go:build linux
|
||||
|
||||
package main
|
||||
|
||||
// Bind the portable predecessor wait to the real single-owner probe. Split out
|
||||
// of main.go so waitForPredecessor itself stays build-tag-free and testable on
|
||||
// any developer host.
|
||||
|
||||
func init() { aliveProbe = daemonAlive }
|
||||
@@ -0,0 +1,106 @@
|
||||
package main
|
||||
|
||||
// B3 regression, daemon half.
|
||||
//
|
||||
// `/etc/init.d/shater restart` is `stop; start`, and procd's `stop` is
|
||||
// asynchronous: the ubus `service delete` returns as soon as SIGTERM has been
|
||||
// SENT, so `start` re-adds the instance while the outgoing `shaterd run` is
|
||||
// still executing its honest teardown. The startup guard used to exit(1) the
|
||||
// moment it saw a live predecessor and rely on procd's `respawn ... 5 ...` to
|
||||
// try again later — a blind retry that neither knows nor waits for the teardown
|
||||
// to finish, and that turns every restart into a logged crash plus a five-second
|
||||
// hole with no data plane.
|
||||
//
|
||||
// The contract these pin: startup BLOCKS until the predecessor is gone (so our
|
||||
// apply is strictly ordered after its teardown, exactly as it is after a manual
|
||||
// `stop` + pause + `start`), and only refuses when the predecessor outlives the
|
||||
// whole budget.
|
||||
|
||||
import (
|
||||
"os"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// scriptAlive installs an aliveProbe that reports a live predecessor for the
|
||||
// first n calls and "gone" afterwards, and restores the real probe.
|
||||
func scriptAlive(t *testing.T, pid, n int) *int {
|
||||
t.Helper()
|
||||
orig := aliveProbe
|
||||
t.Cleanup(func() { aliveProbe = orig })
|
||||
calls := 0
|
||||
aliveProbe = func() (int, bool) {
|
||||
calls++
|
||||
if calls <= n {
|
||||
return pid, true
|
||||
}
|
||||
return 0, false
|
||||
}
|
||||
return &calls
|
||||
}
|
||||
|
||||
// TestWaitForPredecessorWaitsForTeardown: a predecessor that is still tearing
|
||||
// the plane down must be WAITED for, not refused. Before the fix this returned
|
||||
// "still alive" on the first probe and the daemon exited 1.
|
||||
func TestWaitForPredecessorWaitsForTeardown(t *testing.T) {
|
||||
calls := scriptAlive(t, 4242, 3)
|
||||
|
||||
pid, stillAlive := waitForPredecessor(2*time.Second, time.Millisecond, nil)
|
||||
if stillAlive {
|
||||
t.Fatalf("a predecessor that exits within the budget must not be refused (pid %d)", pid)
|
||||
}
|
||||
if *calls < 4 {
|
||||
t.Fatalf("expected the guard to keep probing until the predecessor was gone, got %d probes", *calls)
|
||||
}
|
||||
}
|
||||
|
||||
// TestWaitForPredecessorRefusesAfterBudget: the single-owner invariant is kept —
|
||||
// a predecessor that never dies still ends in a refusal, it is just no longer
|
||||
// the FIRST answer.
|
||||
func TestWaitForPredecessorRefusesAfterBudget(t *testing.T) {
|
||||
orig := aliveProbe
|
||||
defer func() { aliveProbe = orig }()
|
||||
aliveProbe = func() (int, bool) { return 4242, true }
|
||||
|
||||
start := time.Now()
|
||||
pid, stillAlive := waitForPredecessor(30*time.Millisecond, time.Millisecond, nil)
|
||||
if !stillAlive || pid != 4242 {
|
||||
t.Fatalf("an immortal predecessor must still be refused, got pid=%d alive=%v", pid, stillAlive)
|
||||
}
|
||||
if time.Since(start) < 30*time.Millisecond {
|
||||
t.Fatalf("the guard must exhaust its budget before refusing")
|
||||
}
|
||||
}
|
||||
|
||||
// TestWaitForPredecessorNoPredecessorIsFree: the boot path must pay nothing —
|
||||
// one probe, no sleep. A wait that cost a tick on every clean start would show up
|
||||
// as a slower boot for no reason.
|
||||
func TestWaitForPredecessorNoPredecessorIsFree(t *testing.T) {
|
||||
orig := aliveProbe
|
||||
defer func() { aliveProbe = orig }()
|
||||
calls := 0
|
||||
aliveProbe = func() (int, bool) { calls++; return 0, false }
|
||||
|
||||
start := time.Now()
|
||||
if _, stillAlive := waitForPredecessor(time.Minute, time.Second, nil); stillAlive {
|
||||
t.Fatalf("no predecessor must not be reported as alive")
|
||||
}
|
||||
if calls != 1 {
|
||||
t.Fatalf("expected exactly one probe when nothing is running, got %d", calls)
|
||||
}
|
||||
if time.Since(start) > 500*time.Millisecond {
|
||||
t.Fatalf("the no-predecessor path must not sleep")
|
||||
}
|
||||
}
|
||||
|
||||
// TestWaitForPredecessorIgnoresOwnPid: a pidfile naming THIS process (a crashed
|
||||
// predecessor whose pid we were handed, or a re-exec) is not a predecessor.
|
||||
func TestWaitForPredecessorIgnoresOwnPid(t *testing.T) {
|
||||
orig := aliveProbe
|
||||
defer func() { aliveProbe = orig }()
|
||||
aliveProbe = func() (int, bool) { return os.Getpid(), true }
|
||||
|
||||
if _, stillAlive := waitForPredecessor(time.Minute, time.Second, nil); stillAlive {
|
||||
t.Fatalf("our own pid must never count as a predecessor")
|
||||
}
|
||||
}
|
||||
@@ -31,6 +31,12 @@ func ApplyNft(ruleset string) error {
|
||||
if err := runNftStdin(ruleset, "-f", "-"); err != nil {
|
||||
return fmt.Errorf("nft -f (load) failed: %w", err)
|
||||
}
|
||||
// The divert plane just changed underneath every flow that is currently
|
||||
// tracked. For UDP :53 that is not self-correcting — see conntrack.go — so
|
||||
// drop those entries and let the clients re-derive their path through the
|
||||
// ruleset that is now loaded. Best-effort: a failed flush leaves exactly the
|
||||
// behaviour we had before and must never fail an otherwise-good load.
|
||||
_, _ = FlushDNSConntrack()
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -84,6 +90,11 @@ func TeardownNft() error {
|
||||
if out, err := execCommand("nft", "delete", "table", "inet", "shater").CombinedOutput(); err != nil {
|
||||
return fmt.Errorf("nft delete table inet shater: %v\n%s", err, out)
|
||||
}
|
||||
// Same reasoning as ApplyNft, mirrored: every DNS flow that was being
|
||||
// delivered through the tproxy socket has just lost the rule that put it
|
||||
// there. Without this, those entries survive into whatever plane comes next
|
||||
// (including "no plane at all") and keep pointing at a socket that is gone.
|
||||
_, _ = FlushDNSConntrack()
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -366,25 +377,50 @@ func isPointToPoint(dev string) bool {
|
||||
return strings.Contains(string(out), "POINTOPOINT")
|
||||
}
|
||||
|
||||
// RoutingPresent reports whether our fwmark ip rule is currently installed, for
|
||||
// RoutingPresent reports whether our policy routing is currently installed, for
|
||||
// EVERY family the model asks for. With Globals.IPv6 on, ApplyRouting installs
|
||||
// both a -4 and a -6 rule, so checking only -4 was a half-truth: `ip -6 rule` is
|
||||
// both a -4 and a -6 half, so checking only -4 was a half-truth: `ip -6 rule` is
|
||||
// flushed independently (a `network reload` / `ifup` can drop one family and not
|
||||
// the other), and the caller's idempotent fast-path would then conclude the plane
|
||||
// was intact and never restore the missing v6 rule — leaving v6 clients diverted
|
||||
// by nft but with nowhere to be delivered locally.
|
||||
//
|
||||
// With Globals.IPv6 off no v6 rule is installed BY DESIGN, so its absence must
|
||||
// not be read as a missing plane; only the -4 rule is required then.
|
||||
// not be read as a missing plane; only the -4 half is required then.
|
||||
//
|
||||
// # Both halves are checked, not just the rule
|
||||
//
|
||||
// ApplyRouting installs TWO things per family — the `fwmark -> table` rule AND
|
||||
// the `local default dev lo` route inside that table — and they are removed by
|
||||
// two INDEPENDENT commands (`ip rule del`, `ip route flush table`). Checking only
|
||||
// the rule made the second half invisible: a table that had been flushed while
|
||||
// its rule survived (a teardown interrupted part-way, an `ip route flush` from
|
||||
// any other actor) read as "plane intact", so applyLocked's fast-path skipped
|
||||
// ApplyRouting forever and NOTHING re-created the route. The visible result is a
|
||||
// box that reports plane=full / engine_running=true while diverted packets are
|
||||
// marked, find an empty table, fall through to the main table and are handed to
|
||||
// the fail-closed forward drop — a permanent, healthy-looking outage that a
|
||||
// reconcile cannot repair, because a reconcile is exactly what consults this
|
||||
// function. A presence check must cover everything its Apply counterpart
|
||||
// installs, or the idempotent fast-path becomes a trap.
|
||||
func RoutingPresent(g model.Globals) bool {
|
||||
want := fmt.Sprintf("fwmark 0x%x", effFwmark(g))
|
||||
wantRule := fmt.Sprintf("fwmark 0x%x", effFwmark(g))
|
||||
table := fmt.Sprintf("%d", effTable(g))
|
||||
fams := []string{"-4"}
|
||||
if g.IPv6 {
|
||||
fams = append(fams, "-6")
|
||||
}
|
||||
for _, fam := range fams {
|
||||
out, err := execCommand("ip", fam, "rule", "show").Output()
|
||||
if err != nil || !strings.Contains(string(out), want) {
|
||||
if err != nil || !strings.Contains(string(out), wantRule) {
|
||||
return false
|
||||
}
|
||||
// `ip -4 route show table N` prints "local default dev lo scope host";
|
||||
// the v6 form is "local default dev lo metric 1024 pref medium". Matching
|
||||
// the route TYPE + destination covers both without pinning the trailing
|
||||
// attributes, which differ by family and iproute2 version.
|
||||
rout, rerr := execCommand("ip", fam, "route", "show", "table", table).Output()
|
||||
if rerr != nil || !strings.Contains(string(rout), "local default") {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
package netplane
|
||||
|
||||
// Conntrack maintenance for plane transitions.
|
||||
//
|
||||
// # Why the data plane has to touch conntrack at all
|
||||
//
|
||||
// Everything else in this package is *stateless* from the kernel's point of view:
|
||||
// an nft ruleset, a couple of `ip rule`/`ip route` entries and a handful of
|
||||
// sysctls. Rebuilding them is atomic and idempotent, so a rebuilt plane is
|
||||
// indistinguishable from a freshly-installed one — EXCEPT for one thing the
|
||||
// rebuild cannot reach: the connection-tracking entries that were established
|
||||
// while the previous plane (or a half-removed one) was in force.
|
||||
//
|
||||
// That matters here because our divert is a TPROXY divert. A `tproxy` statement
|
||||
// hands the packet to a LOCAL TRANSPARENT SOCKET; when that socket is gone the
|
||||
// statement evaluates to NFT_BREAK and the packet takes a completely different
|
||||
// path through the ruleset. A restart necessarily walks through such a window:
|
||||
// the outgoing daemon closes its engine BEFORE it removes the table (Teardown
|
||||
// order — deliberately, because the reverse order would open a plaintext leak),
|
||||
// and the incoming daemon starts its engine BEFORE it loads the new table. Any
|
||||
// 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.
|
||||
//
|
||||
// # Scope: :53/UDP only, deliberately
|
||||
//
|
||||
// A blanket `conntrack -F` would also delete the entries behind the operator's
|
||||
// SSH session, the LuCI session and the admin panel — fw4's input chain accepts
|
||||
// them via `ct state established,related`, so dropping their conntrack entries
|
||||
// drops the sessions. Locking the admin out of the box while "fixing" DNS is not
|
||||
// a trade we get to make. UDP/:53 is the narrowest cut that covers the observed
|
||||
// failure class: DNS is retried by every client within a second, so deleting its
|
||||
// entries costs nothing and is invisible.
|
||||
//
|
||||
// Best-effort by contract: a kernel without conntrack, a netlink permission
|
||||
// error or a non-Linux build all report zero deletions and no error path that can
|
||||
// fail an apply. Losing the flush degrades to the old behaviour; it must never
|
||||
// take a working plane down.
|
||||
|
||||
// dnsPort is the only port whose conntrack entries we touch. See the package
|
||||
// comment for why this is deliberately not "everything".
|
||||
const dnsPort uint16 = 53
|
||||
|
||||
// flushUDPPortConntrack is the platform seam. The default is a no-op so the
|
||||
// package builds (and `go vet`s) on non-Linux dev hosts; conntrack_linux.go's
|
||||
// init() replaces it with the real ctnetlink delete on the router target. Tests
|
||||
// substitute a recorder.
|
||||
var flushUDPPortConntrack = func(port uint16) (int, error) { return 0, nil }
|
||||
|
||||
// FlushDNSConntrack deletes every UDP connection-tracking entry whose ORIGINAL
|
||||
// destination port is 53, for both address families, and returns how many were
|
||||
// removed.
|
||||
//
|
||||
// Called on every plane transition that can change where a DNS packet is
|
||||
// delivered: after a ruleset is loaded (ApplyNft — the full plane, the holding
|
||||
// plane and a rollback all go through it) and after the table is removed
|
||||
// (TeardownNft). Idempotent and cheap: on an idle box the DNS entry count is a
|
||||
// handful, and on a busy one it is bounded by the number of clients.
|
||||
func FlushDNSConntrack() (int, error) { return flushUDPPortConntrack(dnsPort) }
|
||||
@@ -0,0 +1,62 @@
|
||||
//go:build linux
|
||||
|
||||
package netplane
|
||||
|
||||
// The real ctnetlink implementation of the conntrack seam declared in
|
||||
// conntrack.go. It lives behind a build tag for the same reason apply's flock
|
||||
// does: the netlink conntrack API only exists on Linux, and the control plane
|
||||
// must still build and test on a developer's Windows/macOS host.
|
||||
//
|
||||
// Deliberately netlink and NOT the `conntrack` CLI: conntrack-tools is not a
|
||||
// dependency of shater-core (and pulling it in for one call would add ~100 KiB
|
||||
// of userland to a flash-constrained router), so a shell-out would silently
|
||||
// no-op on every real box — the worst possible outcome for a fix whose entire
|
||||
// job is to remove stale state.
|
||||
|
||||
import (
|
||||
"github.com/sagernet/netlink"
|
||||
"golang.org/x/sys/unix"
|
||||
)
|
||||
|
||||
func init() { flushUDPPortConntrack = ctnetlinkFlushUDPPort }
|
||||
|
||||
// ctnetlinkFlushUDPPort deletes the UDP conntrack entries whose ORIGINAL
|
||||
// destination port is `port`, in both families, and returns the total deleted.
|
||||
//
|
||||
// Errors are aggregated rather than short-circuited: v4 and v6 are independent
|
||||
// tables and a failure on one must not hide a successful cleanup of the other.
|
||||
// The first error is returned for logging; the count is still accurate for the
|
||||
// families that succeeded.
|
||||
func ctnetlinkFlushUDPPort(port uint16) (int, error) {
|
||||
var (
|
||||
total int
|
||||
firstErr error
|
||||
)
|
||||
for _, family := range []netlink.InetFamily{
|
||||
netlink.InetFamily(unix.AF_INET),
|
||||
netlink.InetFamily(unix.AF_INET6),
|
||||
} {
|
||||
filter := &netlink.ConntrackFilter{}
|
||||
// Protocol MUST be set before the port: AddPort refuses to add a port
|
||||
// filter while the layer-4 protocol is unknown (a port means nothing
|
||||
// without one), so the order here is load-bearing.
|
||||
if err := filter.AddProtocol(unix.IPPROTO_UDP); err != nil {
|
||||
if firstErr == nil {
|
||||
firstErr = err
|
||||
}
|
||||
continue
|
||||
}
|
||||
if err := filter.AddPort(netlink.ConntrackOrigDstPort, port); err != nil {
|
||||
if firstErr == nil {
|
||||
firstErr = err
|
||||
}
|
||||
continue
|
||||
}
|
||||
n, err := netlink.ConntrackDeleteFilter(netlink.ConntrackTable, family, filter)
|
||||
total += int(n)
|
||||
if err != nil && firstErr == nil {
|
||||
firstErr = err
|
||||
}
|
||||
}
|
||||
return total, firstErr
|
||||
}
|
||||
@@ -449,6 +449,13 @@ func TestRoutingPresentBothFamilies(t *testing.T) {
|
||||
out = c.v6
|
||||
}
|
||||
}
|
||||
// RoutingPresent also verifies the `local default dev lo` route
|
||||
// that ApplyRouting installs beside the rule (see
|
||||
// TestRoutingPresentRequiresLocalDefaultRoute). This case set is
|
||||
// about the RULE half, so the route half is always healthy here.
|
||||
if name == "ip" && len(arg) >= 3 && arg[1] == "route" {
|
||||
out = "local default dev lo scope host"
|
||||
}
|
||||
cs := append([]string{"-test.run=TestSysctlRevertHelperProcess", "--", name}, arg...)
|
||||
cmd := exec.Command(os.Args[0], cs...)
|
||||
cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "GO_HELPER_STDOUT="+out)
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
package netplane
|
||||
|
||||
// B3 regressions: what a restart leaves behind.
|
||||
//
|
||||
// The bug these pin: `/etc/init.d/shater restart` (stop immediately followed by
|
||||
// start) left DNS to the router's own LAN address permanently dead, while
|
||||
// `stop` + pause + `start` was fine and the status kept reporting plane=full /
|
||||
// engine_running=true. Two independent defects fed it, and both live here:
|
||||
//
|
||||
// 1. A plane transition (load or teardown of the tproxy divert) left the
|
||||
// CONNTRACK entries of flows that had crossed the transition pointing at a
|
||||
// plane that no longer exists. Nothing in the tree touched conntrack, so a
|
||||
// UDP flow — which has no handshake to resynchronise on and whose entry is
|
||||
// refreshed by every retry — stayed wedged indefinitely.
|
||||
// 2. RoutingPresent() reported "plane intact" from the ip RULE alone, ignoring
|
||||
// the `local default dev lo` ROUTE that ApplyRouting installs alongside it.
|
||||
// A teardown interrupted between the two (procd SIGKILL at term_timeout) or
|
||||
// any other `ip route flush` therefore became invisible: applyLocked's
|
||||
// idempotent fast-path skipped ApplyRouting forever and no reconcile could
|
||||
// repair it.
|
||||
|
||||
import (
|
||||
"os"
|
||||
"os/exec"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/sagernet/sing-box/shater/model"
|
||||
)
|
||||
|
||||
// recordFlush swaps the conntrack seam for a counter and restores it after the
|
||||
// test. Returns a pointer to the number of calls and the ports asked for.
|
||||
func recordFlush(t *testing.T) *[]uint16 {
|
||||
t.Helper()
|
||||
orig := flushUDPPortConntrack
|
||||
t.Cleanup(func() { flushUDPPortConntrack = orig })
|
||||
var seen []uint16
|
||||
flushUDPPortConntrack = func(port uint16) (int, error) {
|
||||
seen = append(seen, port)
|
||||
return 0, nil
|
||||
}
|
||||
return &seen
|
||||
}
|
||||
|
||||
// TestApplyNftFlushesDNSConntrack: loading a ruleset must drop the DNS conntrack
|
||||
// entries formed against the previous plane. Without this, a flow that crossed
|
||||
// the restart window keeps being delivered by a rule set that is gone.
|
||||
func TestApplyNftFlushesDNSConntrack(t *testing.T) {
|
||||
seen := recordFlush(t)
|
||||
var rec []string
|
||||
orig := execCommand
|
||||
execCommand = fakeExec(&rec)
|
||||
defer func() { execCommand = orig }()
|
||||
|
||||
if err := ApplyNft("table inet shater {}\n"); err != nil {
|
||||
t.Fatalf("ApplyNft: %v", err)
|
||||
}
|
||||
if len(*seen) != 1 || (*seen)[0] != dnsPort {
|
||||
t.Fatalf("ApplyNft must flush UDP :%d conntrack exactly once, got %v", dnsPort, *seen)
|
||||
}
|
||||
// Ordering matters: the flush is only meaningful once the NEW ruleset is in
|
||||
// the kernel, otherwise the very next packet re-creates the entry against the
|
||||
// old plane. The load is the last nft invocation before it.
|
||||
if len(rec) != 2 || !strings.Contains(rec[0], "-c") || strings.Contains(rec[1], "-c") {
|
||||
t.Fatalf("expected validate-then-load, got %v", rec)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplyNftDoesNotFlushOnFailure: a ruleset that does not load leaves the
|
||||
// PREVIOUS plane in charge (nft -f is one netlink transaction). Flushing then
|
||||
// would tear down live flows for nothing.
|
||||
func TestApplyNftDoesNotFlushOnFailure(t *testing.T) {
|
||||
seen := recordFlush(t)
|
||||
orig := execCommand
|
||||
execCommand = failingExec()
|
||||
defer func() { execCommand = orig }()
|
||||
|
||||
if err := ApplyNft("table inet shater {}\n"); err == nil {
|
||||
t.Fatalf("ApplyNft must report the nft failure")
|
||||
}
|
||||
if len(*seen) != 0 {
|
||||
t.Fatalf("a failed load must not flush conntrack, got %v", *seen)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTeardownNftFlushesDNSConntrack is the mirror: removing the table strands
|
||||
// every DNS flow that was being delivered through the tproxy socket, so those
|
||||
// entries must go with it.
|
||||
func TestTeardownNftFlushesDNSConntrack(t *testing.T) {
|
||||
seen := recordFlush(t)
|
||||
var rec []string
|
||||
orig := execCommand
|
||||
execCommand = fakeExec(&rec)
|
||||
defer func() { execCommand = orig }()
|
||||
|
||||
if err := TeardownNft(); err != nil {
|
||||
t.Fatalf("TeardownNft: %v", err)
|
||||
}
|
||||
// fakeExec makes every command succeed, so TableExists() is true and the
|
||||
// delete runs.
|
||||
if len(*seen) != 1 || (*seen)[0] != dnsPort {
|
||||
t.Fatalf("TeardownNft must flush UDP :%d conntrack exactly once, got %v", dnsPort, *seen)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTeardownNftAbsentTableDoesNotFlush: nothing was diverting, so nothing is
|
||||
// stranded. Keeps the flush out of the hot path of an idle box.
|
||||
func TestTeardownNftAbsentTableDoesNotFlush(t *testing.T) {
|
||||
seen := recordFlush(t)
|
||||
orig := execCommand
|
||||
execCommand = failingExec()
|
||||
defer func() { execCommand = orig }()
|
||||
|
||||
if err := TeardownNft(); err != nil {
|
||||
t.Fatalf("TeardownNft with no table must be a no-op, got %v", err)
|
||||
}
|
||||
if len(*seen) != 0 {
|
||||
t.Fatalf("absent table must not flush conntrack, got %v", *seen)
|
||||
}
|
||||
}
|
||||
|
||||
// failingExec returns an execCommand replacement whose every command exits 1.
|
||||
func failingExec() func(string, ...string) *exec.Cmd {
|
||||
return func(name string, arg ...string) *exec.Cmd {
|
||||
cs := append([]string{"-test.run=TestRestartHelperProcess", "--", name}, arg...)
|
||||
cmd := exec.Command(os.Args[0], cs...)
|
||||
cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "GO_HELPER_FAIL=1")
|
||||
return cmd
|
||||
}
|
||||
}
|
||||
|
||||
// TestRestartHelperProcess is the exec helper for failingExec.
|
||||
func TestRestartHelperProcess(t *testing.T) {
|
||||
if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
|
||||
return
|
||||
}
|
||||
if os.Getenv("GO_HELPER_FAIL") == "1" {
|
||||
os.Exit(1)
|
||||
}
|
||||
os.Exit(0)
|
||||
}
|
||||
|
||||
// TestRoutingPresentRequiresLocalDefaultRoute is the second half of B3.
|
||||
//
|
||||
// ApplyRouting installs TWO things per family and they are removed by two
|
||||
// independent commands. A presence check that only looks at the rule declares a
|
||||
// half-removed plane healthy — and because applyLocked consults exactly this
|
||||
// function to decide whether to re-run ApplyRouting, the missing route is then
|
||||
// never restored: marked packets find an empty table, fall through to main and
|
||||
// are eaten by the fail-closed forward drop, permanently, with the status still
|
||||
// saying plane=full.
|
||||
//
|
||||
// On the pre-fix implementation the first two cases below return true.
|
||||
func TestRoutingPresentRequiresLocalDefaultRoute(t *testing.T) {
|
||||
const (
|
||||
rule = "32765:\tfrom all fwmark 0x2000 lookup shater"
|
||||
route = "local default dev lo scope host"
|
||||
)
|
||||
cases := []struct {
|
||||
name string
|
||||
ipv6 bool
|
||||
v4rule, v4rte string
|
||||
v6rule, v6rte string
|
||||
want bool
|
||||
}{
|
||||
// THE REGRESSION: rule survived, table was flushed.
|
||||
{"v4 rule present, route flushed", false, rule, "", "", "", false},
|
||||
{"ipv6 on, v6 route flushed", true, rule, route, rule, "", false},
|
||||
// Sanity: a complete plane is still reported as present.
|
||||
{"v4 complete", false, rule, route, "", "", true},
|
||||
{"ipv6 on, both complete", true, rule, route, rule, route, true},
|
||||
// The pre-existing rule-level contract must not regress.
|
||||
{"v4 rule missing", false, "", route, "", "", false},
|
||||
{"ipv6 on, v6 rule missing", true, rule, route, "", route, false},
|
||||
}
|
||||
|
||||
orig := execCommand
|
||||
defer func() { execCommand = orig }()
|
||||
|
||||
for _, c := range cases {
|
||||
t.Run(c.name, func(t *testing.T) {
|
||||
execCommand = func(name string, arg ...string) *exec.Cmd {
|
||||
out := ""
|
||||
if name == "ip" && len(arg) >= 2 {
|
||||
v6 := arg[0] == "-6"
|
||||
switch arg[1] {
|
||||
case "rule":
|
||||
out = c.v4rule
|
||||
if v6 {
|
||||
out = c.v6rule
|
||||
}
|
||||
case "route":
|
||||
out = c.v4rte
|
||||
if v6 {
|
||||
out = c.v6rte
|
||||
}
|
||||
}
|
||||
}
|
||||
cs := append([]string{"-test.run=TestSysctlRevertHelperProcess", "--", name}, arg...)
|
||||
cmd := exec.Command(os.Args[0], cs...)
|
||||
cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1", "GO_HELPER_STDOUT="+out)
|
||||
return cmd
|
||||
}
|
||||
g := model.Globals{FwmarkBase: 0x2000, TableBase: 0x2000, IPv6: c.ipv6}
|
||||
if got := RoutingPresent(g); got != c.want {
|
||||
t.Errorf("RoutingPresent = %v, want %v", got, c.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user