ceamac/slocker-lite
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix -p/--port-forward unreachable on extern networks

Browse Source 
A server listening on an extern network wasn't reachable via -p at all,
from the host or from a real outside client -- confirmed by direct
testing: since extern's bridge moved into a private namespace (the
earlier connectivity fix), host root had no route to the container
subnet whatsoever (ip route get fell through to the LAN default gateway
instead), so -p's own DNAT rule, which targets the container's real IP
directly, had nowhere to route the rewritten packet.

Fixed with two pieces in ensure_uplink_provisioned(), both confirmed
necessary by direct testing -- the same "route alone isn't enough on
Android" lesson the uplink's own outbound/return-path ip rules already
learned: a host-root route to the container subnet through the uplink,
plus a matching ip rule routing traffic to that subnet into main (without
it, Android's own lower-priority-number policy routing -- a generic
fwmark 0/0x10000 catch-all among them -- intercepts the packet into an
unrelated table before rule evaluation ever reaches main, so the route
alone is silently never consulted).

Also fixes a related robustness bug found while testing this: a failed
ensure_uplink_provisioned() only stopped the tap relay, leaving every
already-added ip rule/iptables piece (deterministic, hash-derived names)
live on the host -- a first failed attempt then made every later attempt
for the same network name fail identically and permanently, until a
manual fix or a full device reboot. Fixed by recording the relay's pid to
the uplink state file as soon as it's known, so a failure can roll back
via the same teardown_uplink_state() a real --delete-network-full would
use, instead of a partial, drifting copy of its cleanup logic.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
Viorel Munteanu
2026-09-03 12:18:33 +00:00
parent 37ab2fcb1e
commit 6216e2bced
2 changed files with 89 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/network_bridge.cpp
+83 -12
View File
@@ -250,6 +250,12 @@ bool uplink_provisioned(const std::string& root_tap) {
return run_process({"ip", "link", "show", root_tap}).exit_code == 0;
}
// Forward declaration: ensure_uplink_provisioned() below calls this on its
// own failure path to fully roll back whatever it already added, and the
// two are defined in this order for readability (provision before
// teardown) despite the dependency running the other way.
void teardown_uplink_state(const NetworkEntry& network);
// Idempotent (see uplink_provisioned() above): a no-op if the host-root-side
// device already exists. Three distinct pieces are needed beyond the
// tap+relay link and its addresses/route/NAT to actually carry traffic to
@@ -299,6 +305,25 @@ bool ensure_uplink_provisioned(const NetworkEntry& network) {
}
relay->root_side_tap_name = root_tap;
// Recorded now, before any of the steps below that can fail -- so that
// a failure partway through can roll back via the exact same
// teardown_uplink_state() a later --delete-network-full would use,
// rather than needing a second, drifting copy of its cleanup logic
// here. Harmless on the success path too: uplink_state_path() only
// needs to name the relay's pid, already known at this point.
{
std::error_code ec;
auto path = uplink_state_path(network.name);
std::filesystem::create_directories(path.parent_path(), ec);
std::ofstream out(path);
if (out) {
out << relay->relay_pid << '\n';
} else {
spdlog::warn("failed to record uplink relay pid for network '{}' -- a later teardown won't find it",
network.name);
}
}
auto [netns_addr, root_addr] = uplink_transit_addresses(network.name);
std::string transit_subnet = uplink_transit_subnet(network.name);
@@ -328,24 +353,56 @@ bool ensure_uplink_provisioned(const NetworkEntry& network) {
}
ok = ok &&
run_process({"ip", "rule", "add", "priority", "100", "to", transit_subnet, "lookup", "main"}).exit_code == 0;
// Without these two, host root has no way to route to the container
// subnet at all -- needed for -p/--port-forward, whose DNAT rule (added
// in host root, targeting the container's real IP directly --
// port_forward.cpp) otherwise has nowhere to send the rewritten packet.
// Two distinct pieces, both confirmed necessary by direct testing, not
// assumed -- the same "route alone isn't enough on Android" lesson the
// uplink's own outbound/return-path ip rules above already learned:
// 1. The route itself, through the uplink's own netns-side address --
// the same way any next-hop router would reach it. From there, the
// private namespace already forwards arriving traffic to the
// bridge on its own (a fresh namespace's FORWARD policy is ACCEPT
// by default, and ip_forward is already enabled above), and the
// reply's return path is already covered by the private
// namespace's own default route out through this same uplink (set
// above) plus host root's own conntrack correctly reversing the
// original DNAT on the way back out -- no second DNAT stage
// needed.
// 2. An `ip rule` routing traffic *to* the container subnet into
// `main` (where the route above lives), the same shape as the
// return-path rule just above for the transit subnet. Without it,
// the route added in step 1 is simply never consulted: confirmed
// via `ip rule show` on the real device that Android's own
// lower-priority-number policy rules (a generic `fwmark
// 0/0x10000 lookup 99` catch-all among them, matching any
// untouched/forwarded packet) intercept the packet and route it
// into an unrelated table -- with no route to the container
// subnet there -- long before rule evaluation ever reaches `main`.
ok = ok &&
run_process({"ip", "route", "add", network.subnet, "via", strip_prefix(netns_addr), "dev", root_tap})
.exit_code == 0;
ok = ok &&
run_process({"ip", "rule", "add", "priority", "100", "to", network.subnet, "lookup", "main"}).exit_code == 0;
if (!ok) {
spdlog::error("failed to configure uplink for network '{}'", network.name);
stop_tap_relay(*relay);
// Real bug found by testing, not assumed: an earlier version only
// called stop_tap_relay() here, leaving every ip rule/iptables piece
// already added above (all deterministic, hash-derived names) live
// on the host -- a first failed attempt then made every later
// attempt for the *same* network name fail identically and
// permanently ("File exists" on an `ip rule add` that was never
// removed), since nothing ever cleaned it up short of a manual
// by-hand fix or a full device reboot. teardown_uplink_state()
// reverses every piece unconditionally and best-effort, so calling
// it here is exactly as safe as a real --delete-network-full would
// be against this same partially-provisioned state.
teardown_uplink_state(network);
return false;
}
std::error_code ec;
auto path = uplink_state_path(network.name);
std::filesystem::create_directories(path.parent_path(), ec);
std::ofstream out(path);
if (out) {
out << relay->relay_pid << '\n';
} else {
spdlog::warn("failed to record uplink relay pid for network '{}' -- a later teardown won't find it",
network.name);
}
return true;
}
@@ -362,7 +419,21 @@ bool ensure_uplink_provisioned(const NetworkEntry& network) {
void teardown_uplink_state(const NetworkEntry& network) {
std::string root_tap = uplink_root_tap_name(network.name);
std::string transit_subnet = uplink_transit_subnet(network.name);
auto [netns_addr, root_addr] = uplink_transit_addresses(network.name);
// Purely for tidiness -- the kernel already removes this route on its
// own once root_tap itself is deleted below, same as any route through a
// deleted device.
if (run_process({"ip", "route", "del", network.subnet, "via", strip_prefix(netns_addr), "dev", root_tap})
.exit_code != 0) {
spdlog::warn("failed to remove host-root route to network '{}' (already gone, or never existed)",
network.name);
}
if (run_process({"ip", "rule", "del", "priority", "100", "to", network.subnet, "lookup", "main"}).exit_code !=
0) {
spdlog::warn("failed to remove host-root container-subnet ip rule for network '{}' (already gone, or never existed)",
network.name);
}
if (run_process({"ip", "rule", "del", "priority", "100", "to", transit_subnet, "lookup", "main"}).exit_code !=
0) {
spdlog::warn("failed to remove uplink return-path ip rule for network '{}' (already gone, or never existed)",
src/network_bridge.h
+6 -1
View File
@@ -113,7 +113,12 @@ bool should_use_veth(const NetworkEntry& network);
// route alone is not sufficient on Android): an inserted-at-the-front
// iptables FORWARD accept rule, and two `ip rule`s (outbound and
// return-path) routing this traffic into whichever policy-routing table
// the host is actually using for its own real traffic right now.
// the host is actually using for its own real traffic right now. Also
// adds a host-root route to the container subnet itself (via the
// uplink) plus its own matching `ip rule` -- needed for
// -p/--port-forward, whose DNAT rule targets the container's real IP
// directly; without both pieces (route *and* rule, the same lesson as
// the two above) host root has no way to route to the container at all.
// Idempotent: a no-op (true) if the bridge already exists -- **except** the
// uplink step for extern, which still runs (with its own, separate
// idempotency check) even when the bridge already existed, so an