diff --git a/CLAUDE.md b/CLAUDE.md index 14e9aa4..cf5fe73 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -271,7 +271,14 @@ Source layout (all under `src/`): `remove_port_forward_record(container_name, bwrap_pid)` (`bwrap_pid` captured from the same callback, in a variable declared in `run_container()`'s own scope) so a cleanly-exiting session's own record - doesn't linger for `--clean-processes` to find later. + doesn't linger for `--clean-processes` to find later. The same callback + also collects each `JoinedNetwork::relay` (`network_join.h`) returned by + `join_networks()` into a `std::vector active_relays` + declared in `run_container()`'s own scope — the direct tap+relay + (`network_tap_relay.h`) analog of `active_port_forwards` above, since a + relay process is likewise independent host-global state (unlike a veth + pair) that needs an explicit `stop_tap_relay()` call for each, made right + alongside the `remove_port_forward()` loop after `run_bwrap()` returns. - `self_test.{h,cpp}` — `run_self_tests()` implements `-t/--test`, this project's own built-in self-test mode (distinct from the Meson-driven fixture smoke test under `tests/`, described in "Build & test commands" @@ -690,12 +697,29 @@ Source layout (all under `src/`): networked services are unaffected — confirmed by testing (see below). For each named network: looked up in `config.networks` (an unknown name is a per-network error, not fatal to the others); `ensure_network_provisioned()` - (`network_bridge.h`) covers post-reboot recreation; a veth pair is created - wherever that network's bridge lives (`wrap_for_network()`, reused from - `network_bridge.h`), the bridge-side end attached and brought up, the - container-side end moved into the session's own namespace (`ip link set - ... netns `) and renamed `eth` (`N` = the network's position in - the `-n` list, so multiple joins each get a distinct interface). **Address + (`network_bridge.h`) covers post-reboot recreation; then, per + `should_use_veth(network)` (`network_bridge.h` — combines the network's own + `veth` policy flag with a kernel-capability probe, see that file's own + entry above), either a veth pair is created wherever that network's bridge + lives (`wrap_for_network()`, reused from `network_bridge.h`), the + bridge-side end attached and brought up, the container-side end moved into + the session's own namespace (`ip link set ... netns `) and renamed + `eth`, **or**, when veth isn't available or the network was created + with `--no-veth`, `create_tap_relay()` (`network_tap_relay.h`, see below) + is used instead, producing the same end state (a ready `eth` in the + container's namespace) via two tap devices and a relay process rather than + a kernel veth pair. `eth`'s `N` is the network's position in the `-n` + list, so multiple joins each get a distinct interface, regardless of which + strategy created it — everything downstream (IP assignment, routes, the + address returned to `port_forward.h`) is identical either way, since it + only ever operates on `eth` by name. Unlike a veth pair (torn down by + the kernel automatically once the session's namespace goes away, whatever + else fails), a tap relay is an independent process with no such automatic + cleanup — if any step after `create_tap_relay()` succeeds fails later in + `join_one_network()` (address exhaustion, a failed `ip addr add`/route + command), a `fail()` helper (`.cpp`-local, only present when a relay was + actually created) calls `stop_tap_relay()` before returning `nullopt`, so + a partial failure doesn't leak the relay process. **Address allocation, `pick_free_address()`, needed a real fix during testing, not just design**: an interface's actual assigned IP lives inside its own private per-container namespace, invisible from the bridge's own namespace @@ -721,23 +745,29 @@ Source layout (all under `src/`): route command needed for same-bridge reachability regardless of kind). Every step failure is logged specifically (which command, which network) and best-effort: `join_networks()` returns one `JoinedNetwork {network, - container_ip}` per network that actually joined (in `-n` order, so shorter - than the request list on any partial failure), never fatal to the + container_ip, relay}` per network that actually joined (in `-n` order, so + shorter than the request list on any partial failure), never fatal to the already-running session (network setup can only happen after `bwrap`'s own namespace exists, i.e. potentially after the sandboxed command is already running) — this return value exists specifically for `port_forward.h` (see below) to resolve a `-p` spec against which networks/IPs are actually - usable, not as a pass/fail signal on its own. An empty `network_names` - returns immediately (no namespace wait at all), so callers that always - invoke this once `on_bwrap_pid_known` fires for any reason (`commands.cpp` - also fires it for `-D/--daemonize` alone, with no `-n`) don't pay for a - wait that has nothing to do. Veth teardown + usable, not as a pass/fail signal on its own; `relay` (`nullopt` for a + veth-joined network) is what `run_container()` (`commands.cpp`) collects + to call `stop_tap_relay()` on after `run_bwrap()` returns, mirroring how it + already collects `active_port_forwards` for `-p`'s own cleanup. An empty + `network_names` returns immediately (no namespace wait at all), so callers + that always invoke this once `on_bwrap_pid_known` fires for any reason + (`commands.cpp` also fires it for `-D/--daemonize` alone, with no `-n`) + don't pay for a wait that has nothing to do. Veth teardown needs no explicit code: the kernel destroys an entire veth pair (both ends, including the one still attached to the bridge) the instant *either* end's owning namespace is destroyed, so a session's veths disappear on their own once its namespace does — only the bridge/iptables/persistent- namespace state is deliberately left behind (`network_bridge.h`'s - reboot-reconciliation design). **Verified end-to-end on this dev machine + reboot-reconciliation design); a tap relay instead needs the explicit + `stop_tap_relay()` call described above, since it's an independent process + with no namespace of its own to be torn down by. **Verified end-to-end on + this dev machine (root, via a scoped `doas` rule)**: two concurrently-running containers on the same `intern` network got distinct addresses and could ping each other; an `intern`-joined container could not reach the outside @@ -907,14 +937,74 @@ Source layout (all under `src/`): `container_if_name` is a completely ordinary interface from the container's own point of view — `join_one_network()`'s existing IP assignment/route/DNAT-target-address code (unchanged, not yet wired to - call this) needs no changes at all. **Not yet wired into - `join_one_network()`/`run_container()`** — see `self_test.{h,cpp}` above - for how this file's own create/attach/teardown cycle was verified - end-to-end in isolation first; full traffic-plane verification (does - `-p`/inter-container connectivity actually work end-to-end through this - path) is deferred to the commit that does that wiring, where two real - `--no-veth`-joined containers pinging/curling each other is a more natural - and simpler proof than hand-rolled raw-socket test traffic. + call this) needs no changes at all. See `self_test.{h,cpp}` above for how + this file's own create/attach/teardown cycle was verified end-to-end in + isolation first, before being wired into `join_one_network()`. + + **Real fd-leak bug caught by direct testing, not assumed**: the relay + child, unlike every other forked child elsewhere in this project, never + `exec()`s — so `O_CLOEXEC` on fds created *before* this fork (e.g. + `daemonize.cpp`'s own report-pipe write end, still open in the forking + process at this point since `report_daemon_started()` — which closes it — + hasn't run yet when `join_networks()` is called) never takes effect, + since it only closes fds *across `exec()`*, not across a fork that never + execs. Without a fix, the relay child inherited a live copy of that pipe's + write end and never closed it, so `daemonize()`'s read-until-EOF in the + *original, pre-fork* process blocked forever, even after + `report_daemon_started()` closed its own copy — a pipe only reports EOF + once *every* copy of its write end, across every process, is closed. + Confirmed directly: `-r -D -n -- sleep 600` hung + indefinitely; killing the session (which reaches `stop_tap_relay()` via + `run_container()`'s own post-`run_bwrap()` cleanup, closing the leaked + copy) immediately unblocked the original process. Fixed by + `close_inherited_fds()` (`.cpp`-local): scans `/proc/self/fd` and closes + everything except stdin/stdout/stderr and the report pipe's own write + end, called as the very first thing in `relay_child_main()`. A + first-attempt companion fix — adding the relay's pid to the session's own + cgroup (`session_cgroup.h`) so `--kill` would reach it directly, since a + relay is a *sibling* of bwrap rather than a descendant and so would never + inherit cgroup membership on its own — was tried and then **reverted**: + `remove_session_cgroup()` runs *inside* `run_bwrap()`, before + `run_container()` ever gets to call `stop_tap_relay()`, so the cgroup was + still non-empty (the relay still in it) at removal time, and every + session using this fallback left a stray, never-removed cgroup directory + behind (`rmdir` failing with `EBUSY`, confirmed by testing). Since the + ordinary flow already stops the relay correctly on its own (killing bwrap + unblocks `run_bwrap()`'s own `waitpid()`, letting `run_container()` + finish its normal cleanup, `stop_tap_relay()` included) and the only gap + left by *not* doing this is a benign, self-resolving race in `--kill`'s + own "has it fully stopped" check (a genuine crash of the whole session + process, not just bwrap, is a separate, already-scoped concern — see the + crash-orphan sweep below), the added complexity wasn't worth it. + + **Verified end-to-end on this dev machine (root, via the scoped `doas` + rule), using a `--no-veth` `extern` network specifically to exercise this + path**: two real containers joined the same network, each getting a + distinct address (`10.168.0.2`/`10.168.0.3`) via the tap+relay path with + no veth involved at all, and pinged each other successfully (0% packet + loss, confirmed repeatably). **Known gap, confirmed by testing, root + cause not yet identified**: neither container could reach the network's + own gateway IP (the address `provision_bridge()` assigns directly to the + bridge itself) — not with ICMP, not with a TCP `wget` — despite ARP + resolving correctly (confirmed via the container's own `ip neigh`/ + `/proc/net/arp` showing a `REACHABLE` entry for the gateway's real MAC), + meaning L2 delivery through the relay is not at fault. The *exact same* + bridge, same subnet, same host, same MASQUERADE/`ip_forward` setup, but + joined via veth instead of this fallback, reaches the same gateway IP + perfectly (0% loss) — ruling out every environment-level explanation + (host firewall, `rp_filter`, this dev sandbox's own networking) that would + otherwise affect both paths identically. `net.ipv4.conf. + .rp_filter=0` (and broader `all`/bridge-scoped variants) was tried and + confirmed *not* to fix it. Root cause undetermined — diagnosing further + needs host-level tools (`tcpdump`, direct `iptables`/`sysctl` inspection) + this session's `doas` access doesn't permit (restricted to running + `slocker-lite` itself only). **This means gateway/outside reachability + through the tap+relay fallback is unconfirmed** — peer-to-peer + connectivity (an `intern` network's whole purpose, and half of what an + `extern` network offers) is solid; reaching the real internet through one + specifically needs re-verification, ideally on the actual veth-less + target device (a different kernel/environment where this dev sandbox's + own unidentified cause may not even apply) before being relied on. - `session_cgroup.{h,cpp}` — gives `--kill` (`kill_session.{h,cpp}`, see below) a reliable way to find every process a session ever started, however deeply forked/daemonized/reparented, by putting it in a dedicated cgroup v2 diff --git a/src/commands.cpp b/src/commands.cpp index 28a6970..e7bbaf0 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -41,6 +41,7 @@ #include "network_bridge.h" #include "network_join.h" #include "network_subnet.h" +#include "network_tap_relay.h" #include "oci_image.h" #include "pid_file.h" #include "port_forward.h" @@ -692,6 +693,14 @@ int run_container(const std::filesystem::path& image_tar, // the session's namespace goes away. std::vector active_port_forwards; + // Populated inside on_bwrap_pid_known (below) from join_networks()'s own + // JoinedNetwork::relay, read again after run_bwrap() returns to stop + // each one -- unlike a veth-joined network (torn down by the kernel on + // its own once the session's namespace goes away), a tap relay + // (network_tap_relay.h, used when should_use_veth() is false) is an + // independent process with no such automatic cleanup. + std::vector active_relays; + // Set inside on_bwrap_pid_known below, read again after run_bwrap() // returns to remove this session's own port-forward state record. pid_t bwrap_pid = -1; @@ -706,6 +715,11 @@ int run_container(const std::filesystem::path& image_tar, // have already had its chance to run by then rather than racing // an already-returned parent. auto joined = join_networks(pid, network_specs, app_config); + for (const auto& joined_network : joined) { + if (joined_network.relay) { + active_relays.push_back(*joined_network.relay); + } + } for (const auto& spec : parsed_port_forwards) { if (auto active = add_port_forward(spec, joined)) { active_port_forwards.push_back(*active); @@ -740,6 +754,10 @@ int run_container(const std::filesystem::path& image_tar, remove_port_forward_record(container_name, bwrap_pid); } + for (const auto& relay : active_relays) { + stop_tap_relay(relay); + } + if (!unmount_layer(mounted->top_layer_id)) { spdlog::error("failed to unmount layer {}", mounted->top_layer_id); } diff --git a/src/network_join.cpp b/src/network_join.cpp index 5fd4608..312bdb4 100644 --- a/src/network_join.cpp +++ b/src/network_join.cpp @@ -36,6 +36,7 @@ #include "network_bridge.h" #include "network_subnet.h" +#include "network_tap_relay.h" #include "pid_file.h" #include "process.h" #include "sandbox_process.h" @@ -145,65 +146,99 @@ std::optional pick_free_address(const NetworkEntry& network, bool i return std::nullopt; } -std::optional join_one_network(pid_t ns_pid, const NetworkEntry& network, int if_index) { +std::optional join_one_network(pid_t ns_pid, const NetworkEntry& network, int if_index) { if (!ensure_network_provisioned(network)) { return std::nullopt; } std::string bridge = bridge_name(network.name); - // Distinct prefixes ("vh"/"vp") so the two ends' names can't collide with - // each other while both still live in the same (bridge's) namespace, - // right after creation and before the peer end is moved away. + // Distinct prefixes ("vh"/"vp"/"th") so names can't collide with each + // other while a veth pair's two ends still both live in the same + // (bridge's) namespace, right after creation and before the peer end is + // moved away. std::string suffix = fmt::format("{:08x}", fnv1a(fmt::format("{}-{}", network.name, ns_pid))); - std::string host_veth = "vh" + suffix; - std::string peer_veth = "vp" + suffix; - - if (!run(wrap_for_network(network, {"ip", "link", "add", host_veth, "type", "veth", "peer", "name", peer_veth}), - "create veth pair", network.name)) { - return std::nullopt; - } - if (!run(wrap_for_network(network, {"ip", "link", "set", host_veth, "master", bridge}), "attach veth to bridge", - network.name) || - !run(wrap_for_network(network, {"ip", "link", "set", host_veth, "up"}), "bring host veth up", - network.name)) { - return std::nullopt; - } - if (!run(wrap_for_network(network, {"ip", "link", "set", peer_veth, "netns", fmt::to_string(ns_pid)}), - "move veth into the container's namespace", network.name)) { - return std::nullopt; - } - std::string container_if = fmt::format("eth{}", if_index); - if (!run(wrap_in_container(ns_pid, {"ip", "link", "set", peer_veth, "name", container_if}), - "rename the container's interface", network.name)) { - return std::nullopt; + + // should_use_veth() (network_bridge.h) combines the network's own `veth` + // policy flag with a kernel-capability probe -- see + // docs/networking-design.md's tap+relay addendum for why a kernel might + // lack veth support at all (the real target device supports tun/tap but + // not veth), and network_tap_relay.h for what the fallback below does. + // Both branches leave `container_if` a completely ordinary interface + // from the container's own point of view -- everything after this if/else + // (IP assignment, routes, the address returned to port_forward.h) is + // unchanged either way. + std::optional relay; + if (should_use_veth(network)) { + std::string host_veth = "vh" + suffix; + std::string peer_veth = "vp" + suffix; + + if (!run(wrap_for_network(network, + {"ip", "link", "add", host_veth, "type", "veth", "peer", "name", peer_veth}), + "create veth pair", network.name)) { + return std::nullopt; + } + if (!run(wrap_for_network(network, {"ip", "link", "set", host_veth, "master", bridge}), + "attach veth to bridge", network.name) || + !run(wrap_for_network(network, {"ip", "link", "set", host_veth, "up"}), "bring host veth up", + network.name)) { + return std::nullopt; + } + if (!run(wrap_for_network(network, {"ip", "link", "set", peer_veth, "netns", fmt::to_string(ns_pid)}), + "move veth into the container's namespace", network.name)) { + return std::nullopt; + } + if (!run(wrap_in_container(ns_pid, {"ip", "link", "set", peer_veth, "name", container_if}), + "rename the container's interface", network.name)) { + return std::nullopt; + } + } else { + std::string host_tap = "th" + suffix; + auto created = create_tap_relay(network, bridge, host_tap, ns_pid, container_if); + if (!created) { + spdlog::error("failed to create tap relay for network '{}'", network.name); + return std::nullopt; + } + relay = created; } + // Unlike a veth pair (torn down by the kernel on its own once the + // session's namespace goes away, whatever else fails below), a tap + // relay is an independent process with no such automatic cleanup -- if + // it was already created, any later failure in this function must stop + // it explicitly, or it leaks forever. + auto fail = [&]() -> std::optional { + if (relay) { + stop_tap_relay(*relay); + } + return std::nullopt; + }; + auto container_ip = pick_free_address(network, false); if (!container_ip) { spdlog::error("no free IPv4 address available on network '{}'", network.name); - return std::nullopt; + return fail(); } if (!run(wrap_in_container(ns_pid, {"ip", "addr", "add", *container_ip, "dev", container_if}), "assign the container's IPv4 address", network.name)) { - return std::nullopt; + return fail(); } if (network.ipv6) { auto container_ip6 = pick_free_address(network, true); if (!container_ip6) { spdlog::error("no free IPv6 address available on network '{}'", network.name); - return std::nullopt; + return fail(); } if (!run(wrap_in_container(ns_pid, {"ip", "-6", "addr", "add", *container_ip6, "dev", container_if}), "assign the container's IPv6 address", network.name)) { - return std::nullopt; + return fail(); } } if (!run(wrap_in_container(ns_pid, {"ip", "link", "set", container_if, "up"}), "bring the container's interface up", network.name)) { - return std::nullopt; + return fail(); } if (network.kind == NetworkKind::extern_) { @@ -231,7 +266,7 @@ std::optional join_one_network(pid_t ns_pid, const NetworkEntry& ne // Bare IP, no prefix -- what port_forward.h needs as a DNAT target; // pick_free_address() returns the CIDR form since that's what `ip addr // add` needs above. - return container_ip->substr(0, container_ip->find('/')); + return JoinedNetwork{network, container_ip->substr(0, container_ip->find('/')), relay}; } } // namespace @@ -258,8 +293,8 @@ std::vector join_networks(pid_t bwrap_outer_pid, const std::vecto ++if_index; continue; } - if (auto container_ip = join_one_network(*ns_pid, *it, if_index)) { - joined.push_back({*it, *container_ip}); + if (auto joined_network = join_one_network(*ns_pid, *it, if_index)) { + joined.push_back(std::move(*joined_network)); } else { spdlog::error("failed to join network '{}'", name); } diff --git a/src/network_join.h b/src/network_join.h index 09df737..8e5fb97 100644 --- a/src/network_join.h +++ b/src/network_join.h @@ -16,20 +16,26 @@ #pragma once +#include #include #include #include #include "config_file.h" +#include "network_tap_relay.h" // One network a session successfully joined, and the IPv4 address it was // assigned there -- what port_forward.h needs to resolve a -p spec (by name, // or by "the container's sole extern network" when none is given) to an -// actual DNAT target. +// actual DNAT target. `relay` is set only when the join used the tap+relay +// fallback (network_tap_relay.h) instead of a veth pair -- nullopt for a +// veth-joined network, which needs no explicit teardown (the kernel tears +// the veth pair down on its own once the session's namespace goes away). struct JoinedNetwork { NetworkEntry network; std::string container_ip; + std::optional relay; }; // Joins a just-started -r/--run session (identified by bwrap_outer_pid, the diff --git a/src/network_tap_relay.cpp b/src/network_tap_relay.cpp index 4d68b90..39b5ece 100644 --- a/src/network_tap_relay.cpp +++ b/src/network_tap_relay.cpp @@ -16,6 +16,7 @@ #include "network_tap_relay.h" +#include #include #include #include @@ -27,6 +28,7 @@ #include #include +#include #include #include @@ -39,6 +41,47 @@ namespace { +// Closes every open fd except stdin/stdout/stderr and `keep_fd` (the report +// pipe's write end). Necessary specifically because this relay child, unlike +// every other forked child elsewhere in this project, never exec()s -- it +// keeps running as a plain continuation of this same process image, so +// O_CLOEXEC (already used throughout this codebase for exactly this kind of +// fd hygiene -- e.g. pid_file.h's SessionLock, daemonize.cpp's own report +// pipe) never actually takes effect here: it only closes fds *across +// exec()*, not across a fork() that never execs. **Real bug caught by +// direct testing, not assumed**: without this, the relay child inherited a +// live copy of daemonize.cpp's own report-pipe write end (still open in the +// forking process at this point, since report_daemon_started() -- which +// closes it -- hasn't run yet when join_networks() is called) and never +// closed it, so daemonize()'s read-until-EOF in the *original, pre-fork* +// process blocked forever even after report_daemon_started() closed its own +// copy -- a pipe only reports EOF once *every* copy of its write end, across +// every process, is closed. Confirmed directly: `-r -D -n ` +// hung indefinitely; killing the session (which incidentally reaches +// stop_tap_relay() via run_container()'s own post-run_bwrap() cleanup) +// closed the leaked copy and immediately unblocked the original process. +void close_inherited_fds(int keep_fd) { + DIR* dir = opendir("/proc/self/fd"); + if (!dir) { + return; + } + struct dirent* entry; + while ((entry = readdir(dir)) != nullptr) { + char* end = nullptr; + long fd = strtol(entry->d_name, &end, 10); + if (!end || *end != '\0') { + continue; // "." / ".." + } + int fd_int = static_cast(fd); + if (fd_int == STDIN_FILENO || fd_int == STDOUT_FILENO || fd_int == STDERR_FILENO || fd_int == keep_fd || + fd_int == dirfd(dir)) { + continue; + } + close(fd_int); + } + closedir(dir); +} + // Opens /dev/net/tun and creates a tap device named `name` in whatever // network namespace this process is currently in -- IFF_NO_PI so both ends // of a relay agree on raw-frame framing with no extra header, IFF_TAP (not @@ -105,6 +148,8 @@ void report_line(int fd, const std::string& line) { [[noreturn]] void relay_child_main(int report_fd, const NetworkEntry& network, const std::string& bridge, const std::string& host_tap_name, pid_t container_ns_pid, const std::string& container_if_name) { + close_inherited_fds(report_fd); + if (network.kind == NetworkKind::intern) { if (!enter_namespace(persistent_netns_path(network.name).string())) { report_line(report_fd, "ERROR failed to enter network's persistent namespace\n");