Create tap devices persistently via ip tuntap add, drop retry logic

Real-device testing showed the previous retry-based fix (ad30f93) was
insufficient: a bare ioctl(TUNSETIFF)-created tap device (no IFF_PERSIST)
could work for one command and then vanish for the next on that kernel,
not just be slow to appear. Both tap devices are now created ahead of
time via an external `ip tuntap add dev <name> mode tap` before being
attached to via open_tap(), making them genuine persistent netdevices
with no tie to any fd/process lifetime -- the same technique QEMU/libvirt
use. stop_tap_relay() now explicitly `ip link del`s the host-side device
since it no longer disappears on its own; the crash-orphan sweep records
each relay's network kind/name too so it can do the same for orphans.
All retry logic (network_join.cpp's run_with_retry(), self_test.cpp's
wait_for_container_device_visible()) is removed as no longer needed.

self_test.cpp's post-teardown assertions updated to match: the host-side
device is now expected gone after stop_tap_relay(), while the
container-side device is expected to persist (it only goes away once its
own namespace is torn down, not merely because the relay stopped).

Verified end-to-end on the dev machine with --no-veth forcing the
fallback: eth0 stayed visible and usable across repeated commands with
no disappearance, and both gateway and outside (8.8.8.8) ping succeeded
at 0% loss.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
2026-08-30 19:59:14 +00:00
parent ad30f93f77
commit bebf559039
6 changed files with 363 additions and 181 deletions
+118 -47
View File
@@ -333,21 +333,25 @@ Source layout (all under `src/`):
trusting its pid. Confirms the host-side tap gets created and attached to
the bridge (`ip link show` output contains `master <bridge>`), the
container-side tap gets created with the requested name inside the target
namespace (checked via `nsenter --net=/proc/<pid>/ns/net -- ip link show`,
wrapped in `wait_for_container_device_visible()` — a `.cpp`-local bounded
retry, same shape as `network_join.cpp`'s own `run_with_retry()`, added
for the same real device-reported reason — see that function's own doc
comment for the full story, including the tempting-but-harmful internal-
self-check "fix" that was tried and ruled out first, confirmed by this
exact self-test: it started reliably failing, 100% of the time, the moment
that harmful check was added — despite having passed reliably many times
earlier the same session before that change — and immediately went back to
passing reliably once the harmful check was removed again), and — the
biggest previously-unverified assumption from `docs/networking-design.md`'s
tap+relay addendum — **both devices actually disappear on their own once
`stop_tap_relay()` stops the relay process, with no explicit `ip link del`
needed** (neither was created with `IFF_PERSIST`) — confirmed directly,
many times over, on this dev machine (root, via the scoped `doas` rule).
namespace (checked via a plain, single-shot
`nsenter --net=/proc/<pid>/ns/net -- ip link show`; an earlier version
wrapped this in a bounded retry, added when tap devices were still created
via bare `ioctl(TUNSETIFF)` and intermittently weren't immediately
visible — see `network_tap_relay.{h,cpp}`'s own entry below for why that
retry, and the whole class of symptom it was compensating for, is gone
now that devices are created persistently instead), and — **updated once
the original "both devices disappear on their own once `stop_tap_relay()`
stops the relay, no explicit `ip link del` needed" assumption turned out
to be wrong on the real target device** (`network_tap_relay.{h,cpp}`'s own
entry below has the full story) — now confirms the *host*-side device is
explicitly gone after `stop_tap_relay()` (the new `ip link del` step)
while the *container*-side device deliberately still exists (correctly
persistent — only the relay stopped, not the container's own network
namespace); the container-side device's actual disappearance, once that
namespace itself is torn down, isn't separately re-checked (`nsenter` has
nothing left to target once the namespace's only holding process has
already exited) — it's destroyed moments later anyway, at the very end of
this test, when its throwaway namespace-holder process is killed.
Deliberately its own small file since more real tests are expected here as
more of the networking feature lands.
- `env_spec.{h,cpp}``resolve_env_specs()` turns an ordered list of
@@ -869,12 +873,10 @@ Source layout (all under `src/`):
/ns/net -- ip addr add 10.168.0.2/24 dev eth0` failing with `"Cannot find
device \"eth0\""` right after `network_tap_relay.h`'s relay had already
created it — and confirmed by hand that simply retrying the whole session a
few times eventually worked. `run_with_retry()` (`.cpp`-local, same
shape/spirit as `wait_for_isolated_net_namespace()` below) replaces `run()`
for the three steps that touch the just-created `container_if` (IPv4
address, IPv6 address, bringing it up) — retries quietly (no per-attempt
log spam) for up to 500ms before giving up loudly, same as `run()`'s own
single-attempt error.
few times eventually worked. A first fix added a bounded (~500ms) retry
around the steps that touch the just-created `container_if` — later found
insufficient (see below) and removed again; `join_one_network()` now uses a
plain, single-attempt `run()` for every step, same as before any of this.
**A tempting "fix" investigated and ruled out by direct A/B testing on this
dev machine, not just reasoned about**: the obvious first instinct — have
@@ -892,17 +894,27 @@ Source layout (all under `src/`):
still holding it open, immediately after device creation, appears to
corrupt the device's *external* visibility specifically on this kernel;
the *same* process's own view of the device it just created stayed correct
throughout). Confirmed via direct A/B testing, not just correlation:
removing that internal check immediately restored 100%-reliable, zero-retry
first-try success on three separate real sessions; re-adding it reliably
broke it again, including for a from-scratch self-test reproduction (see
`self_test.{h,cpp}` below) that had passed reliably many times earlier in
the same session before this specific change. The fix that shipped is
unambiguous: never add an internal, same-process/fd-holding self-check to
the relay; only the *external*, separate-process retry above
(`run_with_retry()`) is safe, and is sufficient on its own — every retried
real-device call this session succeeded on the very first attempt once the
harmful internal check was gone.
throughout). The lesson that survived into the final fix: never add an
internal, same-process/fd-holding self-check to the relay.
**The retry fix above turned out to be insufficient**: a further round of
real-device testing showed a *different* failure — `ip addr add` against
the container-side device would sometimes succeed, only for the very next
command against that same device (`ip link set eth0 up`) to fail with
"Cannot find device", exhausting every retry. The device wasn't merely
slow to become visible after creation; it was **disappearing on its own**,
consistent with the underlying `ioctl(TUNSETIFF)`-created device (no
`IFF_PERSIST`) having a more fragile lifetime on that kernel than "stays
alive as long as the one fd that created it stays open." Per the user's
own suggested direction, the fix was structural, not another retry: both
tap devices are now created ahead of time via an external `ip tuntap add
dev <name> mode tap` (`create_persistent_tap()`, `network_tap_relay.cpp`
see that file's own entry below for the full detail), which sidesteps the
whole class of symptom by making the device a genuinely persistent
netdevice with no tie to any fd or process. All retry logic (`run()` is
used unconditionally, everywhere) was removed as part of this — the
earlier retry was compensating for a problem this fix removes outright,
not one it makes more likely to need retrying.
- `port_forward.{h,cpp}` — implements `-p`. `parse_port_forward_spec()`
splits `"[<network>:]<host-port>:<container-port>"` on `':'` (2 or 3
fields; the network name is deliberately restricted to excluding `':'` --
@@ -1021,13 +1033,21 @@ Source layout (all under `src/`):
caller-provided (not derived here) specifically so a future caller
(`join_one_network()`, once this is wired in) can reuse its own existing
fnv1a-based veth-naming scheme rather than this file growing a second,
drifting copy of that six-line hash. `open_tap()` (`.cpp`-local) creates
each device via a direct `open("/dev/net/tun")` + `ioctl(TUNSETIFF,
IFF_TAP | IFF_NO_PI)``IFF_NO_PI` so both ends agree on raw-frame
framing with no extra header, deliberately **not** `IFF_PERSIST`, so each
device is expected to disappear on its own once its one-and-only fd
closes, the same "no explicit teardown" property veth already has (see
`self_test.{h,cpp}` above for where this got verified). The relay child's
drifting copy of that six-line hash. Each device is now created two-step:
`create_persistent_tap(name)` (`.cpp`-local) first runs an external `ip
tuntap add dev <name> mode tap`, then `open_tap()` (`.cpp`-local, mostly
unchanged) `open("/dev/net/tun")` + `ioctl(TUNSETIFF, IFF_TAP |
IFF_NO_PI)`s onto that already-existing device (`IFF_NO_PI` so both ends
agree on raw-frame framing with no extra header) — `open_tap()` now only
*attaches* an fd to a device, it no longer *creates* one. **This split
replaced an earlier, simpler design** where `open_tap()` alone both
created (via the same `ioctl`, with no `IFF_PERSIST`) and attached, on the
assumption the device would then simply disappear on its own once its
one-and-only fd closed, the same "no explicit teardown" property veth
already has — see this entry's own "tap devices need to be created
persistently" paragraph further down for why that assumption turned out
to be wrong on the real target device, and `docs/networking-design.md`'s
matching section for the full incident writeup. The relay child's
entire setup sequence — enter the network's own namespace first if
`intern` (`persistent_netns_path()`, `persistent_netns.h`), create+attach
the host-side tap, `setns()` into the container's namespace (the
@@ -1041,9 +1061,17 @@ Source layout (all under `src/`):
between the two fds — this loop *is* the actual "veth wire," just
implemented once in userspace instead of by the kernel. No `SIGTERM`
handler is installed in the relay: default disposition (terminate) already
closes both fds on the way out, which is all `stop_tap_relay()`'s
"no explicit teardown" contract needs. `stop_tap_relay()` sends `SIGTERM`
and reaps the process. Once `create_tap_relay()` returns successfully,
closes both fds on the way out. `stop_tap_relay()` sends `SIGTERM`, reaps
the process, then explicitly `ip link del`s the host-side device
(`wrap_for_network(handle.network, ...)`, reaching wherever it lives —
host root for `extern`, the network's own persistent namespace for
`intern` — hence `TapRelayHandle` carrying its own `NetworkEntry`) — now
required since the device is persistent and no longer disappears just
because the relay's fd closed (see below). The container-side device
needs no matching step: it lives inside the container's own network
namespace, which the kernel already tears down (every interface inside
it, persistent or not, along with it) once the session itself ends.
Once `create_tap_relay()` returns successfully,
`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
@@ -1087,6 +1115,27 @@ Source layout (all under `src/`):
process, not just bwrap, is a separate, already-scoped concern — see the
crash-orphan sweep below), the added complexity wasn't worth it.
**Real bug reported from the real target device: tap devices need to be
created persistently, not tied to the relay's own fd lifetime.** Two
rounds of real-device testing (`network_join.cpp`'s own entry above has
the full incident writeup) found the container-side device intermittently
either not immediately visible after creation, or — worse, found on the
second round — visible and usable for one command (e.g. `ip addr add`
succeeding) and then gone for the very next one (`ip link set ... up`
failing with "Cannot find device"), on a kernel where the bare
`ioctl(TUNSETIFF)`-created (no `IFF_PERSIST`) device evidently has a more
fragile lifetime than "stays alive as long as its one creating fd stays
open." Per the user's own suggested direction, the fix (see
`create_persistent_tap()` above) creates both tap devices ahead of time
via an external `ip tuntap add dev <name> mode tap` — the same technique
QEMU/libvirt use to let an unprivileged process attach to a tap device set
up ahead of time — turning each into a genuinely persistent netdevice with
no tie to any fd or process at all, the same as a veth pair already is.
All retry logic from the first round's fix (`network_join.cpp`'s
`run_with_retry()`, `self_test.cpp`'s `wait_for_container_device_visible()`)
was removed once this structural fix made it unnecessary — see both
files' own entries.
**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
@@ -1115,6 +1164,18 @@ Source layout (all under `src/`):
`--delete-network-full` exists specifically so this class of
stale-state-masking-as-a-bug can't recur.
**Re-verified end-to-end on this dev machine after the persistent-device
redesign above**, again with `--no-veth` forcing the fallback: a single
container repeatedly used its tap-relay-backed `eth0` across several
commands in a row (`ip link show`, `ip addr show`, two rounds of `ping`)
with no disappearance between commands — the exact symptom the real
device hit — and both gateway ping and outside/internet ping (`8.8.8.8`)
succeeded at 0% loss. Session cleanup left no leftover host-side tap
device behind (only the bridge itself, deliberately left standing per
this project's reboot-reconciliation design); `-t/--test`'s own
`tap-relay create/attach/teardown` case (updated per `self_test.{h,cpp}`'s
own entry above) passes reliably across repeated runs.
**Crash-orphan sweep**, the direct tap+relay analog of `port_forward.h`'s
own (see its own entry below): unlike a veth pair or a session's own
bridge/persistent-namespace state, a relay process is host-global state
@@ -1127,8 +1188,14 @@ Source layout (all under `src/`):
`session_pid_file_path()`/`port_forward_state_path()` already use, so
`clean_stale_tap_relays()` can cross-reference filenames directly against
`list_sessions()`'s own `SessionInfo::path`. `record_tap_relays()` writes
one line per relay (`"<relay_pid> <host_tap_name>"`) to that path — a
no-op if there's nothing to record. `clean_stale_tap_relays()`
one line per relay (`"<relay_pid> <host_tap_name> <kind> <network_name>"`,
`<kind>` = `"extern"`/`"intern"`, `<network_name>` last since it's the one
field that can contain whitespace) to that path — a no-op if there's
nothing to record; the `<kind>`/`<network_name>` fields were added
alongside the persistent-tap-device redesign above, so a later sweep can
reconstruct a `NetworkEntry` and reach the right namespace to remove the
now-persistent host-side device too, not just kill the relay process.
`clean_stale_tap_relays()`
(`commands.cpp`'s `clean_processes_command()`, alongside
`clean_stale_sessions()`/`clean_stale_port_forwards()`) scans that
directory: a record whose filename doesn't match any currently-*running*
@@ -1136,9 +1203,13 @@ Source layout (all under `src/`):
already-dead pid, or one this process was never the parent of, isn't
treated as an error, since this sweep runs from a *separate* later
invocation that can't `waitpid()` an orphan it didn't fork — its true
parent's own exit, or `init` after reparenting, reaps it) before the
record file itself is deleted; a record whose session is still running is
left completely untouched. **Verified via a controlled scratch test**,
parent's own exit, or `init` after reparenting, reaps it), the host-side
tap device it named is removed (`ip link del`, via
`wrap_for_network()` against a `NetworkEntry` reconstructed from the
record's own `<kind>`/`<network_name>` fields — best-effort, same as
`stop_tap_relay()`'s own removal) before the record file itself is
deleted; a record whose session is still running is left completely
untouched. **Verified via a controlled scratch test**,
the same shape `port_forward.h`'s own sweep test used: root wasn't needed
for the sweep *logic* itself (only real tap/bridge creation needs it),
so this ran as a plain rootless daemonized session (`-D`, no `-n`) to get
+92 -11
View File
@@ -407,9 +407,13 @@ Landed as four commits (a fifth, this doc update, closes it out) — see
Verified via a new `-t/--test` case: a throwaway bridge + throwaway
network namespace, confirming the host-side tap attaches to the bridge,
the container-side tap appears inside the target namespace with the
requested name, and — the biggest assumption going in — both devices
disappear on their own once the relay is stopped, no explicit
`ip link del` needed (neither is created with `IFF_PERSIST`).
requested name. **The original assumption here — that both devices
disappear on their own once the relay is stopped, since neither was
created with `IFF_PERSIST` — turned out to be wrong on the real target
device** (see "Resolved: tap devices need to be created persistently,
not tied to the relay's own fd lifetime" below); devices are now created
persistently via an external `ip tuntap add`, and the host-side one is
explicitly removed (`ip link del`) when the relay stops.
3. **Wire into `join_one_network()`/`join_networks()`.** `JoinedNetwork`
gains an optional `relay` handle; `run_container()` collects and stops
them after `run_bwrap()` returns, mirroring `-p`'s own
@@ -532,11 +536,11 @@ failing with `"Cannot find device \"eth0\""` immediately after the relay had
already created it — confirmed by hand that retrying the whole session a few
times eventually let it succeed.
**Fix**: `network_join.cpp`'s `join_one_network()` now retries (bounded,
~500ms, quiet until final success/give-up) the three steps that touch the
just-created container interface — IPv4 address, IPv6 address, bringing it
up — via a new `run_with_retry()` instead of the plain `run()` used
elsewhere.
**First fix tried, later superseded (see next section)**: `network_join.cpp`'s
`join_one_network()` retried (bounded, ~500ms, quiet until final
success/give-up) the three steps that touch the just-created container
interface — IPv4 address, IPv6 address, bringing it up — via a
`run_with_retry()` instead of the plain `run()` used elsewhere.
**A tempting "fix" investigated and ruled out by direct A/B testing on the
dev machine, not just reasoned about**: the obvious first instinct — have
@@ -554,9 +558,86 @@ change was reverted. Root cause not fully understood (something about
forking a subprocess that inherits the tap fd — deliberately not
`O_CLOEXEC` — while still holding it open, immediately after device
creation, appears to corrupt the device's *external* visibility on this
kernel specifically), but the fix that shipped is unambiguous: never add an
internal, same-process/fd-holding self-check to the relay; the external,
separate-process retry above is safe and sufficient on its own.
kernel specifically), but the lesson that survived into the final fix is
unambiguous: never add an internal, same-process/fd-holding self-check to
the relay.
## Resolved: tap devices need to be created persistently, not tied to the relay's own fd lifetime
**Trigger**: the retry fix above turned out to be insufficient — a further
round of real-device testing (a second `run.log`) showed a *different*
failure pattern: `ip addr add` against the container-side device would
sometimes *succeed*, only for the very next command against that same
device (`ip link set eth0 up`) to fail with "Cannot find device", exhausting
every retry. The user's own diagnosis, confirmed correct: the device wasn't
merely slow to become visible after creation (the earlier theory) — it was
actually **disappearing on its own**, on this kernel, independent of
anything this project's own code was doing to it. This is consistent with
the underlying device having been created via a plain `ioctl(fd,
TUNSETIFF, &ifr)` with no `IFF_PERSIST` flag: on that kernel, its lifetime
seems to be tied to something more fragile than "the one fd that created
it stays open" (the relay process never closes or re-opens its own fds
around any of this) — never fully root-caused, and not worth chasing
further once a structurally different approach removed the whole class of
symptom.
**Fix, per the user's own suggestion**: stop relying on `ioctl(TUNSETIFF)`
alone to *create* the device at all. Both the host-side and container-side
tap devices are now created ahead of time by an external `ip tuntap add dev
<name> mode tap` command (`create_persistent_tap()`,
`network_tap_relay.cpp`) — the same technique QEMU/libvirt use to let an
unprivileged process attach to a tap device someone else set up — and only
*attached to* afterward via the existing `open_tap()`'s `open("/dev/net/tun")`
+ `ioctl(TUNSETIFF)` call (unchanged; it no longer creates, only opens an
fd onto an already-existing device). A device created this way is a first-
class, persistent netdevice from the kernel's point of view, with no tie to
any single fd or process at all — the same reason `ip tuntap add`/`ip
link add ... type veth` never need an owning process to stay alive either.
**Consequence: teardown is no longer automatic.** Since neither device
disappears on its own once the relay stops, `stop_tap_relay()` now
explicitly `ip link del`s the host-side device after reaping the relay
process (wrapped via `wrap_for_network()` to reach wherever it lives — host
root for `extern`, the network's own persistent namespace for `intern`).
The container-side device needs no equivalent step: it lives inside the
container's own network namespace, which the kernel already tears down
(taking every interface inside it along, persistent or not) once the
session itself ends — nothing new required there. The crash-orphan sweep
(`record_tap_relays()`/`clean_stale_tap_relays()`) was extended the same
way: its state file now also records each relay's network kind/name (not
just its pid and host-side device name), so a sweep for a crashed session
can reconstruct a `NetworkEntry` and reach the right namespace to remove
the orphaned host-side device, not just kill the orphaned relay process.
**All retry logic from the previous fix was dropped**, per the user's own
explicit instruction, once persistent creation removed the underlying
disappearing-device problem it was compensating for: `network_join.cpp`'s
`run_with_retry()` is gone (reverted to the plain `run()` used everywhere
else), and `self_test.cpp`'s matching `wait_for_container_device_visible()`
retry helper is gone too.
**`self_test.cpp`'s own expectations updated accordingly**: the test used to
assert that *both* the host-side and container-side devices vanish on their
own once `stop_tap_relay()` stops the relay — exactly the assumption this
fix disproves. It now asserts the host-side device is gone (the new
explicit `ip link del` step) while the container-side device is still
present (correctly persistent, since only the relay stopped, not the
container's own namespace) — the namespace itself is destroyed moments
later, at the very end of the test, when its throwaway holder process is
killed.
**Verified end-to-end on this dev machine, `--no-veth` forcing the fallback
path** (root, via the scoped `doas` rule): a fresh `extern` network's
container repeatedly used its tap-relay-backed `eth0` across several
commands in a row (`ip link show`, `ip addr show`, ping) with no
disappearance; gateway ping (0% loss) and outside/internet ping to
`8.8.8.8` (0% loss) both worked; a second round of `ip link
show`/`ip addr show` after the pings still saw the same device correctly.
Session cleanup left no leftover host-side tap/veth devices behind (only
the bridge itself, which is deliberately left standing per this project's
reboot-reconciliation design). `-t/--test`'s own `tap-relay
create/attach/teardown` case, updated as above, passes reliably across
repeated runs.
## Explicitly out of scope for now
+6 -50
View File
@@ -92,50 +92,6 @@ bool run(const std::vector<std::string>& argv, std::string_view what, std::strin
return true;
}
// Same as run() above, but retries (nanosleep, EINTR-retry -- the same
// shape wait_for_isolated_net_namespace() below already uses) on failure
// for up to timeout_ms before giving up, quietly (no per-attempt error
// spam) -- only the final give-up is logged as an error, same as run()'s
// own single-attempt message. **Real bug reported from the real target
// device**: right after network_tap_relay.h's relay creates the
// container-side tap device (via `ioctl(TUNSETIFF)`, reporting success),
// this function's own caller -- a completely separate process, joining the
// same namespace fresh via `nsenter` -- could briefly fail to find that
// device with "Cannot find device"; retrying the whole session by hand a
// few times let it eventually succeed. This retries automatically instead.
// **A tempting "fix" investigated and ruled out, not just assumed safe**:
// having the relay *itself* self-verify visibility (a same-process,
// fd-holding check, before ever reporting success) was tried first and
// found to be actively harmful -- confirmed by direct A/B testing on this
// dev machine, it made the container-side device *permanently* invisible
// to every external `nsenter` afterward, 100% reproducibly, where the
// mechanism had otherwise always worked instantly and reliably. That
// self-check was removed entirely; only this external, unrelated-process
// retry remains. Harmless when nothing is actually wrong (e.g. on the veth
// path, where the device is already proven to exist by the time this
// runs, or on a device where the relay's device is already visible
// immediately) -- the very first attempt succeeding costs nothing extra.
bool run_with_retry(const std::vector<std::string>& argv, std::string_view what, std::string_view network_name) {
constexpr int interval_ms = 25;
constexpr int timeout_ms = 500;
for (int elapsed = 0; elapsed <= timeout_ms; elapsed += interval_ms) {
if (run_process(argv).exit_code == 0) {
if (elapsed > 0) {
spdlog::debug("succeeded to {} for network '{}' after ~{}ms of retrying", what, network_name,
elapsed);
}
return true;
}
struct timespec ts {
0, static_cast<long>(interval_ms) * 1000000L
};
while (nanosleep(&ts, &ts) != 0 && errno == EINTR) {
}
}
spdlog::error("failed to {} for network '{}' (gave up after ~{}ms of retrying)", what, network_name, timeout_ms);
return false;
}
// Address allocation can't be checked by inspecting live interface state the
// way session/cgroup liveness can: a container's actual assigned IP lives on
// its own `eth<N>` inside its own private namespace, invisible from the
@@ -263,8 +219,8 @@ std::optional<JoinedNetwork> join_one_network(pid_t ns_pid, const NetworkEntry&
spdlog::error("no free IPv4 address available on network '{}'", network.name);
return fail();
}
if (!run_with_retry(wrap_in_container(ns_pid, {"ip", "addr", "add", *container_ip, "dev", container_if}),
"assign the container's IPv4 address", network.name)) {
if (!run(wrap_in_container(ns_pid, {"ip", "addr", "add", *container_ip, "dev", container_if}),
"assign the container's IPv4 address", network.name)) {
return fail();
}
@@ -274,14 +230,14 @@ std::optional<JoinedNetwork> join_one_network(pid_t ns_pid, const NetworkEntry&
spdlog::error("no free IPv6 address available on network '{}'", network.name);
return fail();
}
if (!run_with_retry(wrap_in_container(ns_pid, {"ip", "-6", "addr", "add", *container_ip6, "dev", container_if}),
"assign the container's IPv6 address", network.name)) {
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 fail();
}
}
if (!run_with_retry(wrap_in_container(ns_pid, {"ip", "link", "set", container_if, "up"}),
"bring the container's interface up", network.name)) {
if (!run(wrap_in_container(ns_pid, {"ip", "link", "set", container_if, "up"}), "bring the container's interface up",
network.name)) {
return fail();
}
+84 -13
View File
@@ -86,14 +86,42 @@ void close_inherited_fds(int keep_fd) {
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
// IFF_TUN) so the host-side end can be enslaved to a bridge like any other
// Ethernet device. Deliberately not IFF_PERSIST: the device should disappear
// on its own once this fd (the only one ever opened on it) closes, the same
// property veth already has -- see stop_tap_relay()'s own doc comment.
// Returns -1 (logging why) on failure.
// Creates a *persistent* tap device named `name` via an external `ip tuntap
// add` command (netlink-based, verifiably complete by the time
// run_process() returns) in whatever network namespace this process is
// currently in -- the same technique QEMU/libvirt use to let an
// unprivileged process attach to a tap device set up ahead of time. **Real
// bug reported from the real target device, not assumed**: an earlier
// version of this file created tap devices purely via
// `ioctl(fd, TUNSETIFF, ...)` with no external creation step at all -- a
// device created that way is tied to that one fd's lifetime with no
// IFF_PERSIST (matching veth's own "disappears once torn down, no explicit
// cleanup" property). On the real device, such a device intermittently
// disappeared *before* the relay itself ever stopped -- confirmed
// reproducible: `ip addr add` against it succeeded, then the very next `ip
// link set ... up` against the exact same device immediately failed with
// "Cannot find device". Explicitly creating it as a real, persistent
// interface first -- independent of any one process's fd -- sidesteps
// whatever fd/process-lifetime-tied kernel behavior was causing that.
// Returns false (logging why) on failure. No `pi` flag given, matching
// open_tap()'s own IFF_NO_PI attach below -- both ends must agree on
// framing.
bool create_persistent_tap(const std::string& name) {
if (run_process({"ip", "tuntap", "add", "dev", name, "mode", "tap"}).exit_code != 0) {
spdlog::error("failed to create persistent tap device '{}'", name);
return false;
}
return true;
}
// Opens /dev/net/tun and attaches to the tap device named `name` --
// create_persistent_tap(), above, must have already created it; TUNSETIFF
// attaches the returned fd to that *existing* device rather than creating a
// new one, since it's now already persistent. IFF_NO_PI so both ends of a
// relay agree on raw-frame framing with no extra header (matching
// create_persistent_tap()'s own no-`pi` creation), IFF_TAP (not IFF_TUN)
// matching how the device was already created. Returns -1 (logging why) on
// failure.
int open_tap(const std::string& name) {
int fd = open("/dev/net/tun", O_RDWR);
if (fd < 0) {
@@ -161,9 +189,13 @@ void report_line(int fd, const std::string& line) {
}
}
if (!create_persistent_tap(host_tap_name)) {
report_line(report_fd, "ERROR failed to create host-side tap device\n");
_exit(1);
}
int fd_host = open_tap(host_tap_name);
if (fd_host < 0) {
report_line(report_fd, "ERROR failed to create host-side tap device\n");
report_line(report_fd, "ERROR failed to attach to host-side tap device\n");
_exit(1);
}
@@ -191,9 +223,13 @@ void report_line(int fd, const std::string& line) {
_exit(1);
}
if (!create_persistent_tap(container_if_name)) {
report_line(report_fd, "ERROR failed to create container-side tap device\n");
_exit(1);
}
int fd_container = open_tap(container_if_name);
if (fd_container < 0) {
report_line(report_fd, "ERROR failed to create container-side tap device\n");
report_line(report_fd, "ERROR failed to attach to container-side tap device\n");
_exit(1);
}
@@ -288,7 +324,7 @@ std::optional<TapRelayHandle> create_tap_relay(const NetworkEntry& network, cons
return std::nullopt;
}
return TapRelayHandle{pid, host_tap_name};
return TapRelayHandle{pid, host_tap_name, network};
}
void stop_tap_relay(const TapRelayHandle& handle) {
@@ -297,6 +333,14 @@ void stop_tap_relay(const TapRelayHandle& handle) {
}
int status = 0;
waitpid(handle.relay_pid, &status, 0);
// The host-side device is now persistent (create_persistent_tap()) --
// it does not disappear on its own once the relay's fd closes, so it
// must be explicitly removed here.
if (run_process(wrap_for_network(handle.network, {"ip", "link", "del", handle.host_tap_name})).exit_code != 0) {
spdlog::warn("failed to remove host-side tap device '{}' for network '{}'", handle.host_tap_name,
handle.network.name);
}
}
std::filesystem::path tap_relay_state_path(std::string_view container_name, pid_t pid) {
@@ -322,7 +366,13 @@ void record_tap_relays(std::string_view container_name, pid_t pid, const std::ve
return;
}
for (const auto& relay : active) {
out << relay.relay_pid << ' ' << relay.host_tap_name << '\n';
// Network name last -- it's the one field that could contain
// whitespace (is_valid_network_name() only rejects ':'), so
// clean_stale_tap_relays() reads it via getline() over the rest of
// the line rather than another whitespace-delimited >>.
out << relay.relay_pid << ' ' << relay.host_tap_name << ' '
<< (relay.network.kind == NetworkKind::extern_ ? "extern" : "intern") << ' ' << relay.network.name
<< '\n';
}
}
@@ -359,10 +409,31 @@ std::vector<std::string> clean_stale_tap_relays() {
std::ifstream in(entry.path());
pid_t relay_pid = 0;
std::string host_tap_name;
while (in >> relay_pid >> host_tap_name) {
std::string kind_str;
while (in >> relay_pid >> host_tap_name >> kind_str) {
std::string network_name;
std::getline(in, network_name);
if (!network_name.empty() && network_name.front() == ' ') {
network_name.erase(0, 1);
}
if (kill(relay_pid, SIGKILL) != 0 && errno != ESRCH) {
spdlog::warn("failed to kill stale tap relay pid {}: {}", relay_pid, strerror(errno));
}
// Reconstructed only enough of the network to reach the
// host-side device (wrap_for_network() only needs kind/name) --
// the device is persistent now (create_persistent_tap()), so it
// doesn't disappear on its own once the relay is killed, same
// as stop_tap_relay()'s own explicit removal for the
// non-crashed case.
NetworkEntry network;
network.name = network_name;
network.kind = (kind_str == "intern") ? NetworkKind::intern : NetworkKind::extern_;
if (run_process(wrap_for_network(network, {"ip", "link", "del", host_tap_name})).exit_code != 0) {
spdlog::warn("failed to remove stale host-side tap device '{}' for network '{}'", host_tap_name,
network_name);
}
}
std::error_code remove_ec;
+42 -13
View File
@@ -35,6 +35,12 @@
struct TapRelayHandle {
pid_t relay_pid;
std::string host_tap_name;
// Needed so stop_tap_relay() can wrap_for_network() (network_bridge.h)
// to reach wherever the host-side device actually lives (host root for
// extern, the network's own persistent namespace for intern) when
// explicitly removing it -- see stop_tap_relay()'s own doc comment for
// why that's needed now.
NetworkEntry network;
};
// Creates a tap-backed substitute for one veth pair between `network`'s
@@ -66,11 +72,26 @@ std::optional<TapRelayHandle> create_tap_relay(const NetworkEntry& network, cons
const std::string& host_tap_name, pid_t container_ns_pid,
const std::string& container_if_name);
// Stops a relay started by create_tap_relay(): sends SIGTERM and reaps it.
// Both tap devices are expected to disappear on their own once the relay
// process exits and its fds close (neither was created with IFF_PERSIST) --
// the same "no explicit teardown needed" property veth already has; this
// function doesn't attempt any further cleanup beyond stopping the process.
// Stops a relay started by create_tap_relay(): sends SIGTERM, reaps it, then
// explicitly removes the host-side tap device (`ip link del`, wrapped via
// wrap_for_network() to reach wherever it lives). **Real bug reported from
// the real target device, not assumed**: an earlier version of this device
// creation relied on IFF_PERSIST-less ioctl(TUNSETIFF) semantics ("the
// device disappears once its one-and-only fd closes, no explicit teardown
// needed") -- but on that device, a tap device created that way
// intermittently disappeared on its own, seemingly at random, well before
// the relay itself ever stopped (confirmed: `ip addr add` against it
// succeeded, then the very next `ip link set ... up` against the exact same
// device immediately failed with "Cannot find device"). Devices are now
// created persistently instead (create_persistent_tap(), .cpp-local, via an
// external `ip tuntap add` -- the same technique QEMU/libvirt use to let an
// unprivileged process attach to a tap device set up ahead of time), which
// sidesteps whatever fd/process-lifetime-tied kernel behavior was causing
// that -- but persistent devices don't disappear on their own at all
// anymore, so this explicit removal step is now required. The
// container-side device needs no such step: it lives inside the container's
// own network namespace, torn down (taking every interface inside it,
// persistent or not, along with it) once the session itself ends.
void stop_tap_relay(const TapRelayHandle& handle);
// $XDG_STATE_HOME/slocker-lite/tap-relays/<container_name>-<pid> -- same
@@ -82,13 +103,18 @@ void stop_tap_relay(const TapRelayHandle& handle);
// liveness a second, drifting way.
std::filesystem::path tap_relay_state_path(std::string_view container_name, pid_t pid);
// Records `active` (one line per relay: "<relay_pid> <host_tap_name>") to
// Records `active` (one line per relay: "<relay_pid> <host_tap_name> <kind>
// <network_name>", `<kind>` = "extern"/"intern", `<network_name>` last since
// it's the one field that could contain whitespace) to
// tap_relay_state_path(container_name, pid), so a later
// clean_stale_tap_relays() run (e.g. after this process crashes before ever
// reaching its own stop_tap_relay() calls) knows which relay processes to
// stop for a session that's no longer running. A no-op if `active` is
// empty -- nothing to record. Best-effort: logs a warning and does nothing
// further on failure, never fatal.
// stop -- and, now that the host-side device is persistent and doesn't
// disappear on its own (see stop_tap_relay()'s own doc comment), which
// host-side tap device to remove and where to reach it -- for a session
// that's no longer running. A no-op if `active` is empty -- nothing to
// record. Best-effort: logs a warning and does nothing further on failure,
// never fatal.
void record_tap_relays(std::string_view container_name, pid_t pid, const std::vector<TapRelayHandle>& active);
// Removes the record written by record_tap_relays() for (container_name,
@@ -111,8 +137,11 @@ void remove_tap_relay_record(std::string_view container_name, pid_t pid);
// checked again here for safety) or one that crashed -- SIGKILLs every
// relay pid listed in it (best-effort -- an already-dead pid, or one this
// process was never the parent of and so can't waitpid(), is not treated
// as an error) and removes the record file. A record whose session is
// still running is left completely alone. Returns the container names
// actually cleaned up, mirroring clean_stale_port_forwards()'s own return
// shape.
// as an error), removes the host-side tap device it named (`ip link del`,
// via wrap_for_network() against a NetworkEntry reconstructed from the
// record's own `<kind>`/`<network_name>` fields -- best-effort, same as
// stop_tap_relay()'s own removal), and removes the record file. A record
// whose session is still running is left completely alone. Returns the
// container names actually cleaned up, mirroring
// clean_stale_port_forwards()'s own return shape.
std::vector<std::string> clean_stale_tap_relays();
+21 -47
View File
@@ -36,46 +36,6 @@
namespace {
// Polls (nanosleep, EINTR-retry -- same shape as this file's own isolation
// wait below, and network_join.cpp's own run_with_retry()) until `nsenter
// --net=/proc/<container_pid>/ns/net -- ip link show <container_if>`
// succeeds, or timeout_ms elapses. Mirrors join_one_network()'s own
// run_with_retry() (network_join.cpp) -- see that function's doc comment
// for the full story: a user's real target device reported the
// container-side tap device intermittently not yet visible immediately
// after creation. **A tempting but actively harmful fix, ruled out by
// direct testing on this dev machine, not just reasoned about**: having
// the relay itself (network_tap_relay.cpp) self-verify visibility (via its
// own `run_process()` call) before ever reporting success made the
// container-side device *permanently* invisible to every external
// `nsenter`, reproducibly, 100% of the time -- confirmed by direct A/B
// testing (adding that internal check broke a previously 100%-reliable
// real session; removing it again immediately restored first-try success,
// no retries ever needed on this machine). Root cause not fully understood
// (something about forking a subprocess that inherits the tap fd -- opened
// without `O_CLOEXEC` -- immediately after device creation, while still
// holding it open, appears to corrupt the device's external visibility on
// this kernel), but the fix is unambiguous: never add an internal,
// same-process/fd-holding self-check to the relay; an external,
// unrelated-process retry (this function, and join_one_network()'s own) is
// safe and sufficient.
bool wait_for_container_device_visible(pid_t container_pid, const std::string& container_if, int timeout_ms) {
constexpr int interval_ms = 25;
for (int elapsed = 0; elapsed <= timeout_ms; elapsed += interval_ms) {
if (run_process({"nsenter", fmt::format("--net=/proc/{}/ns/net", container_pid), "--", "ip", "link", "show",
container_if})
.exit_code == 0) {
return true;
}
struct timespec ts {
0, static_cast<long>(interval_ms) * 1000000L
};
while (nanosleep(&ts, &ts) != 0 && errno == EINTR) {
}
}
return false;
}
bool test_persistent_netns() {
constexpr std::string_view test_netns_name = "selftest";
@@ -119,10 +79,14 @@ bool test_persistent_netns() {
// (kept alive by a child blocked in pause()) stands in for a real -r/--run
// session's isolated net namespace. Confirms: the host-side tap gets created
// and attached to the bridge; the container-side tap gets created, with the
// requested name, inside the target namespace; and -- the biggest unverified
// assumption in docs/networking-design.md's addendum -- both devices
// disappear on their own once stop_tap_relay() stops the relay process, with
// no explicit `ip link del` needed.
// requested name, inside the target namespace; and, once stop_tap_relay()
// stops the relay, the host-side device is explicitly removed (`ip link
// del`, since it's now created via `ip tuntap add` and no longer disappears
// on its own just because its one-and-only fd closes -- see
// stop_tap_relay()'s own doc comment) while the container-side device
// deliberately survives, unaffected by the relay stopping: it only goes away
// once the container's own network namespace itself is torn down (below,
// when this test kills container_pid).
bool test_tap_relay() {
const std::string test_bridge = "slkselftest0";
const std::string host_tap = "thselftest0";
@@ -200,7 +164,9 @@ bool test_tap_relay() {
}
if (ok) {
if (!wait_for_container_device_visible(container_pid, container_if, 500)) {
auto container_check = run_process(
{"nsenter", fmt::format("--net=/proc/{}/ns/net", container_pid), "--", "ip", "link", "show", container_if});
if (container_check.exit_code != 0) {
spdlog::error("self-test: container-side tap device missing inside the target namespace");
ok = false;
}
@@ -213,10 +179,18 @@ bool test_tap_relay() {
spdlog::error("self-test: host-side tap device still exists after stopping the relay");
ok = false;
}
// The container-side device is expected to survive the relay
// stopping -- it's persistent now (create_persistent_tap(),
// network_tap_relay.cpp) and lives inside the container's own
// network namespace, which stopping the relay doesn't touch at all.
// It only disappears once that namespace itself is destroyed (this
// test does that below, by killing container_pid) -- not re-checked
// here, since nsenter can't target a namespace whose only holding
// process has already exited.
if (run_process({"nsenter", fmt::format("--net=/proc/{}/ns/net", container_pid), "--", "ip", "link", "show",
container_if})
.exit_code == 0) {
spdlog::error("self-test: container-side tap device still exists after stopping the relay");
.exit_code != 0) {
spdlog::error("self-test: container-side tap device unexpectedly gone after stopping the relay");
ok = false;
}
}