From 431732396bff3f33154f6d7eb31f7ea6a0ee7cc8 Mon Sep 17 00:00:00 2001 From: Crimson Date: Fri, 7 Aug 2026 05:08:20 +0500 Subject: [PATCH] fix: apply nft batches in the selected namespace, complete the swap rollback [P0] Two release blockers, the first a regression introduced by dd5be47. 1. The anti-torrent batch was applied on the host. Making the rebuild atomic moved it from run_step(transport, ..) to run_nft_batch(script), and run_nft_batch spawned `nft` directly with Command::new. An nft transaction belongs to the network namespace of the process that runs nft, so with the production default apply_in_amnezia_netns = true the sequence became: inspect inside the container, install the table on the host, verify inside the container, fail. The guard was silently ineffective, a stray table accumulated on the host, and the next reconcile could fail on `add table` because the host copy already existed. NsTransport gains run_with_stdin, implemented for all three transports - host, nsenter, and docker exec with -i, which is required or the child sees EOF immediately. run_nft_batch now takes the transport, and both callers pass the one they inspected with. A recording fake transport asserts the script goes through the transport rather than to a host process, which is the regression this could not previously catch. 2. Swap replacement rolled back with the wrong priority and not from every branch. The rollback reactivated with desired.priority - the value that may be exactly what the kernel had just rejected - because the previously active priority was dropped at the match arm and never passed in. The active entry's priority is now carried into replace_active_swapfile and used by the rollback. Three early failure branches also returned without undoing anything: a failed swapoff of the original, a failed rename to .previous, and a failed swapoff of the staging file all left the staging swap active and the transition half-applied, after which the next reconcile tried to unlink a file that was still active and could not proceed. The transition now records what it completed - staging_active, original_active, original_moved, new_installed - and one rollback undoes exactly that much, in an order that never leaves the host without swap: restore the original file, reactivate it at its previous priority, confirm it, and only then release and remove the staging swap. The returned error carries the original cause plus what the recovery achieved. Leftovers from a killed process are no longer removed blindly. A .new or .previous that is still active cannot be unlinked, so it is reported with the swapoff command that clears it instead of failing the same way on every reconcile. Also in this commit, from the same audit: - Requires=docker.service dropped in favour of Wants=. The daemon is built to outlive Docker being down, but Requires= had systemd stop the harness whenever Docker stopped, and Restart=on-failure does not cover a dependency-ordered stop - so the reconnect loop never got to run. - apply_antitorrent_disabled swallowed every discover_target error, not just "no container running". Docker being unreachable or a permission error on the socket reported success for a cleanup that never happened. Only AppError::NotFound is tolerated now. - FstabTransaction::begin asserts /etc/fstab is a regular non-symlink file. read_to_string and metadata both follow links, so a symlinked fstab would be read through and then replaced by the rename, detaching the real file from the path the system boots from. - append_event serialises its read-modify-write behind a mutex; the reconcile loop and the Docker events thread both append and could drop an entry. - tool_exists loses the fallocate, chmod and dd arms, dead since mkswap --file replaced the allocation chain. Docs corrected where they no longer matched the code: findmnt is described as mandatory rather than skippable, the swap creation wording is mkswap --file, and the README package examples use 0.1.6 to match Cargo.toml. Remaining fallocate mentions are explicitly historical ("the previous chain"), which is accurate. Seven tests added: batch routed through the given transport, batch failure reported as such, rollback state defaults and previous-priority carry, active leftovers reported rather than deleted, inactive leftovers cleared. cargo fmt --check, clippy -D warnings, cargo test --locked --workspace --release: 188 + 4 passed. --- README.md | 4 +- awg2-harness.service | 8 +- docs/ACCEPTANCE_CHECKLIST.md | 8 +- docs/CONFIGURATION.md | 2 +- docs/IMPLEMENTATION_SPEC.md | 4 +- traffic_core/src/config.rs | 3 - traffic_core/src/daemon.rs | 8 + traffic_core/src/firewall/antitorrent.rs | 32 ++- traffic_core/src/firewall/host.rs | 17 +- traffic_core/src/firewall/nft.rs | 94 +++++--- traffic_core/src/fstab.rs | 23 ++ traffic_core/src/netns.rs | 67 +++++- traffic_core/src/swap.rs | 272 +++++++++++++++++------ traffic_core/src/wg.rs | 4 + 14 files changed, 423 insertions(+), 123 deletions(-) diff --git a/README.md b/README.md index f888b4a..e2d1933 100644 --- a/README.md +++ b/README.md @@ -224,7 +224,7 @@ scp -P 2323 dist/SHA256SUMS root@:/root/ ```bash cd /root sha256sum -c SHA256SUMS -chmod 644 /root/awg2-harness_0.2.6_amd64.deb +chmod 644 /root/awg2-harness_0.1.6_amd64.deb apt install ./awg2-harness_*_amd64.deb install -d -m 0755 /etc/awg2-harness install -m 0644 /root/awg2-harness.config.toml /etc/awg2-harness/config.toml @@ -237,7 +237,7 @@ install -m 0644 /root/awg2-harness.config.toml /etc/awg2-harness/config.toml ```bash cd /root sha256sum -c SHA256SUMS -chmod 644 /root/awg2-harness_0.2.6_amd64.deb +chmod 644 /root/awg2-harness_0.1.6_amd64.deb apt install ./awg2-harness_*_amd64.deb install -d -m 0755 /etc/awg2-harness cp /usr/share/doc/awg2-harness/repo/config.example.toml /etc/awg2-harness/config.toml diff --git a/awg2-harness.service b/awg2-harness.service index 0dc7299..f3f1d3b 100644 --- a/awg2-harness.service +++ b/awg2-harness.service @@ -1,8 +1,12 @@ [Unit] Description=AWG2 Harness multi-orchestrator After=network-online.target docker.service -Wants=network-online.target -Requires=docker.service +# Wants, not Requires. The daemon is built to outlive Docker being down: it reconnects to +# docker events, retries traffic and firewall reconciles, and treats a missing container as +# a transient state rather than a fatal one. Requires= would have systemd stop the harness +# whenever Docker is stopped, and Restart=on-failure does not cover a dependency-ordered +# stop - so the reconnect loop it has never got the chance to run. +Wants=network-online.target docker.service # Restart backoff below can span ~5 minutes, so the failure window has to be wider than # the old 60s or the limit would trip during a single legitimate backoff sequence. StartLimitIntervalSec=15min diff --git a/docs/ACCEPTANCE_CHECKLIST.md b/docs/ACCEPTANCE_CHECKLIST.md index 8e052c6..b9589a8 100644 --- a/docs/ACCEPTANCE_CHECKLIST.md +++ b/docs/ACCEPTANCE_CHECKLIST.md @@ -141,8 +141,11 @@ - [ ] Файл по пути `99-awg2-harness.conf` без этого маркера остаётся нетронутым и не ломает `ensure-swap`. - [ ] При `swap.enabled = false` файл `90-awg2-harness-swap.conf` удаляется, а swapfile, его активность и запись в `/etc/fstab` **остаются** — после перезагрузки swap снова включится; удаляется только persistence `vm.swappiness`. - [ ] Изменение только `swap.priority` не пересоздаёт swapfile (проверяется по неизменившемуся mtime файла). -- [ ] При изменении `swap.size_mb` на активном swap хост не остаётся без swap во время `fallocate`/`mkswap`: в `/proc/swaps` всё время присутствует хотя бы одна запись. +- [ ] При изменении `swap.size_mb` на активном swap хост не остаётся без swap во время `mkswap --file`: в `/proc/swaps` всё время присутствует хотя бы одна запись. - [ ] Если `swapoff` второго чужого swap падает, ранее отключённые чужие swap возвращаются в активное состояние. +- [ ] При откате resize swap возвращается с **прежним** priority, а не с новым (проверяется заведомо неприменимым `swap.priority`). +- [ ] Kill процесса в середине resize: следующий запуск с активным `.new` или `.previous` даёт понятную ошибку с командой `swapoff`, а не бесконечно падает на удалении активного файла. +- [ ] После отката в `/proc/swaps` присутствует ровно исходный swap; `.new` отключён и удалён. - [ ] Нечитаемый managed sysctl-файл даёт в `status` причину `managed sysctl file is unreadable`, а не «файл отсутствует». - [ ] При `reboot.enabled = false` файл `/run/awg2-harness/reboot.plan` удалён, `status` не показывает устаревший план. - [ ] После принятия запроса на перезагрузку `reboot.plan` удалён. @@ -155,6 +158,9 @@ ## I. Anti-torrent guard на nftables - [ ] Если `anti_torrent.enabled = true`, в Amnezia netns есть `table inet awg2_harness`. +- [ ] На **хосте** таблицы `inet awg2_harness` при `apply_in_amnezia_netns = true` нет: `nft list table inet awg2_harness` в host namespace возвращает ошибку «No such file or directory». +- [ ] При недоступном Docker `apply-firewall` с `anti_torrent.enabled = false` завершается ошибкой, а не молчаливым успехом. +- [ ] `systemctl stop docker` не останавливает `awg2-harness.service` (проверка отсутствия `Requires=docker.service`). - [ ] Есть chain `forward_antitorrent` с hook `forward` и policy `accept`. - [ ] Правило ограничивает `ip saddr `, default `10.8.1.0/24`. - [ ] Лимит считается per source VPN IP через meter/set, а не глобально по всей подсети. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b299ef4..3c1a899 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -200,7 +200,7 @@ swappiness = 1 Ограничения `swap.path`: путь должен быть либо `/swapfile`, либо файлом непосредственно в `/var/lib/awg2-harness/`. Дополнительно: абсолютный, без компонентов `.` и `..`, без пробелов, управляющих символов и обратных слэшей; на этапе применения проверяется, что ни один родительский каталог не является symlink. -Ограничение корней обязательно, а не косметика. Если родительский каталог доступен на запись кому-то кроме root, между проверкой пути и запуском `fallocate`/`mkswap` этот пользователь успевает подменить файл на symlink и перенаправить операцию, выполняемую от root. Оба разрешённых корня принадлежат root, поэтому окна не возникает. `/swapfile` оставлен для совместимости с уже развёрнутыми конфигурациями. +Ограничение корней обязательно, а не косметика. Если родительский каталог доступен на запись кому-то кроме root, между проверкой пути и запуском `mkswap --file` этот пользователь успевает подменить файл на symlink и перенаправить операцию, выполняемую от root. Оба разрешённых корня принадлежат root, поэтому окна не возникает. `/swapfile` оставлен для совместимости с уже развёрнутыми конфигурациями. Владение фиксируется в `/var/lib/awg2-harness/swap-ownership.conf` по `device` + `inode` + пути, а не по маркеру рядом с файлом. Файл, которого нет в этой записи, или файл с тем же путём, но другими `device`/`inode` (то есть подменённый), считается чужим: он никогда не пересоздаётся и не удаляется, вместо этого возвращается ошибка. Чужие активные swap только выключаются — их файлы пакет не удаляет. diff --git a/docs/IMPLEMENTATION_SPEC.md b/docs/IMPLEMENTATION_SPEC.md index 4e79602..820864b 100644 --- a/docs/IMPLEMENTATION_SPEC.md +++ b/docs/IMPLEMENTATION_SPEC.md @@ -649,7 +649,7 @@ inode = 123456 - нет пробелов, управляющих символов и обратных слэшей; - путь равен `/swapfile` **либо** лежит непосредственно в `/var/lib/awg2-harness/` (без вложенных каталогов). -Это allow-list, а не deny-list, и это принципиально. Список запрещённых путей закрывает только те случаи, которые кто-то придумал; значение имеет другое — может ли родительский каталог писать кто-то кроме root. Между preflight-проверкой и запуском `fallocate`/`dd`/`mkswap` непривилегированный пользователь с правом записи в родительский каталог успевает подменить финальный компонент на symlink и перенаправить операцию, выполняемую от root. Это классический TOCTOU, и никакое количество `stat` перед использованием его не закрывает — окно устраняется только тем, что запись в родительский каталог доступна лишь root. +Это allow-list, а не deny-list, и это принципиально. Список запрещённых путей закрывает только те случаи, которые кто-то придумал; значение имеет другое — может ли родительский каталог писать кто-то кроме root. Между preflight-проверкой и запуском `mkswap --file` непривилегированный пользователь с правом записи в родительский каталог успевает подменить финальный компонент на symlink и перенаправить операцию, выполняемую от root. Это классический TOCTOU, и никакое количество `stat` перед использованием его не закрывает — окно устраняется только тем, что запись в родительский каталог доступна лишь root. Оба разрешённых корня root-owned. `/swapfile` оставлен, чтобы уже развёрнутые конфигурации не требовали правки. @@ -664,7 +664,7 @@ inode = 123456 - резервная копия обязательна. Если её не удалось записать, целевой файл не трогается вовсе — ситуация, в которой копия не пишется, и есть та, в которой она нужна; - копий две: `/etc/fstab.awg2-harness.bak` перезаписывается перед каждой правкой и содержит состояние до неё, а `/etc/fstab.awg2-harness.original` создаётся один раз через `create_new` и хранит содержимое до первого вмешательства пакета; - запись идёт во временный файл рядом с целевым, с `create_new`, `sync_all` и переносом прав с оригинала; устаревший временный файл от убитого процесса удаляется явно, а не переиспользуется; -- кандидат проверяется через `findmnt --verify --tab-file` до подмены. Если `findmnt` недоступен, шаг пропускается; если он отработал и отверг файл — операция прекращается; +- кандидат проверяется через `findmnt --verify --tab-file` до подмены. Проверка обязательна: `findmnt` входит в `util-linux`, от которого пакет и так зависит, и включён в обязательные host tools при `swap.enabled = true`. Невозможность его запустить и отказ файла одинаково прекращают операцию — молча пропускать единственную структурную проверку файла, от которого зависит загрузка хоста, недопустимо; - подмена — атомарный `rename`, после него каталог синхронизируется; - после `rename` содержимое перечитывается и сверяется с записанным. Расхождение приводит к восстановлению из резервной копии. diff --git a/traffic_core/src/config.rs b/traffic_core/src/config.rs index 0d34a42..2924932 100644 --- a/traffic_core/src/config.rs +++ b/traffic_core/src/config.rs @@ -493,14 +493,11 @@ fn tool_exists(tool: &str) -> bool { "nsenter" => &[&["--help"]], "nft" => &[&["--version"]], "systemctl" => &[&["--version"], &["--help"]], - "fallocate" => &[&["--help"]], - "chmod" => &[&["--help"]], "mkswap" => &[&["--help"]], "swapon" => &[&["--help"]], "swapoff" => &[&["--help"]], "sysctl" => &[&["--version"], &["--help"]], "findmnt" => &[&["--version"]], - "dd" => &[&["--help"]], _ => &[&["--version"], &["--help"], &["-V"]], }; diff --git a/traffic_core/src/daemon.rs b/traffic_core/src/daemon.rs index cc46fc2..adf7d7e 100644 --- a/traffic_core/src/daemon.rs +++ b/traffic_core/src/daemon.rs @@ -268,7 +268,15 @@ fn persist_state(state: &LastActionState) { let _ = fs::write(LAST_STATE_FILE, content); } +/// Serialises the read-modify-write in [`append_event`]. +/// +/// The reconcile loop and the Docker events thread both append. Without this each one +/// reads the whole file, rewrites it and truncates, so two overlapping appends drop one +/// of the entries. +static EVENTS_LOCK: Mutex<()> = Mutex::new(()); + fn append_event(event: &str) { + let _guard = EVENTS_LOCK.lock(); let _ = fs::create_dir_all("/run/awg2-harness"); let ts = now_rfc3339(); let mut lines: Vec = fs::read_to_string(EVENTS_FILE) diff --git a/traffic_core/src/firewall/antitorrent.rs b/traffic_core/src/firewall/antitorrent.rs index 13a6e9e..67b01df 100644 --- a/traffic_core/src/firewall/antitorrent.rs +++ b/traffic_core/src/firewall/antitorrent.rs @@ -10,7 +10,7 @@ use serde_json::Value; -use crate::{config::Config, docker, netns::NsTransport}; +use crate::{config::Config, docker, error::AppError, netns::NsTransport}; use super::{ ANTITORRENT_TABLE, firewall_transport, @@ -49,11 +49,14 @@ pub(super) fn apply_antitorrent_firewall(cfg: &Config) -> anyhow::Result<()> { // failing to put anything back. The four separate commands this replaces could also // leave a half-built table - a table and counter but no chain, or a chain with no rule. let present = table_exists(transport.as_ref(), ANTITORRENT_TABLE)?; - run_nft_batch(&build_antitorrent_batch_script( - &cfg.anti_torrent.peer_ipv4_cidr, - cfg.anti_torrent.max_connections_per_peer, - present, - ))?; + run_nft_batch( + transport.as_ref(), + &build_antitorrent_batch_script( + &cfg.anti_torrent.peer_ipv4_cidr, + cfg.anti_torrent.max_connections_per_peer, + present, + ), + )?; let verify = inspect_firewall(transport.as_ref(), cfg)?; if firewall_has_drift(&verify) { @@ -76,8 +79,16 @@ fn apply_antitorrent_disabled(cfg: &Config) -> anyhow::Result<()> { &["nft", "delete", "table", "inet", ANTITORRENT_TABLE], )?; } - Err(_) => { - // container is absent now: nothing to remove in its netns + // Only an absent container is benign - there is no namespace to clean up in. + // Every other failure (Docker unreachable, permission denied on the socket, a + // malformed inspect response) means we do not know whether a stale table is + // still filtering traffic, and swallowing it reported success for work that + // never happened. + Err(e) if is_container_absent(&e) => {} + Err(e) => { + return Err(e.context( + "cannot reach the Amnezia container to remove the anti-torrent table", + )); } } } else { @@ -90,6 +101,11 @@ fn apply_antitorrent_disabled(cfg: &Config) -> anyhow::Result<()> { Ok(()) } +/// Distinguishes "no Amnezia container is running" from every other discovery failure. +fn is_container_absent(e: &anyhow::Error) -> bool { + matches!(e.downcast_ref::(), Some(AppError::NotFound(_))) +} + fn inspect_firewall(transport: &dyn NsTransport, cfg: &Config) -> anyhow::Result { let table = match transport.run(&["nft", "list", "table", "inet", ANTITORRENT_TABLE]) { Ok(out) => out, diff --git a/traffic_core/src/firewall/host.rs b/traffic_core/src/firewall/host.rs index ed6c334..3e310b7 100644 --- a/traffic_core/src/firewall/host.rs +++ b/traffic_core/src/firewall/host.rs @@ -30,11 +30,9 @@ pub(super) fn apply_host_firewall(cfg: &Config) -> anyhow::Result<()> { .trim() .to_ascii_lowercase(); - apply_host_firewall_batch( - cfg, - &policy, - table_exists(transport.as_ref(), HOST_FW_TABLE)?, - )?; + let present = table_exists(transport.as_ref(), HOST_FW_TABLE)?; + let batch = build_host_firewall_batch_script(cfg, &policy, present); + run_nft_batch(transport.as_ref(), &batch)?; if let Some(reason) = host_firewall_drift_reason(cfg)? { anyhow::bail!("host firewall verify-after-apply failed: {reason}"); @@ -42,15 +40,6 @@ pub(super) fn apply_host_firewall(cfg: &Config) -> anyhow::Result<()> { Ok(()) } -fn apply_host_firewall_batch( - cfg: &Config, - policy: &str, - table_present: bool, -) -> anyhow::Result<()> { - let batch = build_host_firewall_batch_script(cfg, policy, table_present); - run_nft_batch(&batch) -} - /// Renders the replacement as a single nft transaction. /// /// The delete belongs *inside* the batch. `nft -f` applies a script atomically: if any diff --git a/traffic_core/src/firewall/nft.rs b/traffic_core/src/firewall/nft.rs index 5b7b9db..44b9d59 100644 --- a/traffic_core/src/firewall/nft.rs +++ b/traffic_core/src/firewall/nft.rs @@ -5,11 +5,6 @@ //! `ct state established,related`, and brace/newline layout are all unstable. Matching on //! raw output would report drift on a correct ruleset. -use std::{ - io::Write, - process::{Command, Stdio}, -}; - use crate::netns::NsTransport; pub(super) fn normalize_nft_text(input: &str) -> String { @@ -62,32 +57,77 @@ pub(super) fn table_exists(transport: &dyn NsTransport, table: &str) -> anyhow:: } } -/// Applies a ruleset through `nft -f -`. +/// Applies a ruleset through `nft -f -` **in the transport's namespace**. /// /// A batch is atomic: either the whole script lands or nothing does. That is why the /// delete of the old table belongs inside the script rather than in a preceding command - /// a rejected batch then leaves the previous ruleset exactly as it was, instead of having /// already removed it. -pub(super) fn run_nft_batch(script: &str) -> anyhow::Result<()> { - let mut child = Command::new("nft") - .args(["-f", "-"]) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn()?; - - if let Some(mut stdin) = child.stdin.take() { - stdin.write_all(script.as_bytes())?; - } else { - anyhow::bail!("failed to open stdin for nft batch"); - } - - let out = child.wait_with_output()?; - if !out.status.success() { - anyhow::bail!( - "nft batch apply failed: {}", - String::from_utf8_lossy(&out.stderr) - ); - } +/// +/// The transport is not optional. An nft transaction belongs to the network namespace of +/// the process that runs `nft`, so a script built and verified against the Amnezia +/// container namespace must be piped into a process inside that namespace. Running a plain +/// host-side `nft` here installed the anti-torrent table on the host while inspection and +/// verification looked inside the container, which made the guard silently ineffective. +pub(super) fn run_nft_batch(transport: &dyn NsTransport, script: &str) -> anyhow::Result<()> { + transport + .run_with_stdin(&["nft", "-f", "-"], script) + .map_err(|e| anyhow::anyhow!("nft batch apply failed: {e}"))?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Mutex; + + /// Records what was asked of it, so a test can assert the batch went to this transport + /// rather than to a host-side process. + #[derive(Default)] + struct RecordingTransport { + piped: Mutex, String)>>, + } + + impl NsTransport for RecordingTransport { + fn run(&self, _args: &[&str]) -> anyhow::Result { + Ok(String::new()) + } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + self.piped.lock().expect("lock").push(( + args.iter().map(|s| s.to_string()).collect(), + input.to_string(), + )); + Ok(String::new()) + } + } + + /// The regression this guards: the batch used to be piped into a host-side `nft` + /// regardless of which namespace it had been built and verified for, so the + /// anti-torrent table landed on the host while inspection looked inside the container. + #[test] + fn batch_is_piped_through_the_given_transport() { + let transport = RecordingTransport::default(); + run_nft_batch(&transport, "add table inet awg2_harness\n").expect("batch"); + + let calls = transport.piped.lock().expect("lock"); + assert_eq!(calls.len(), 1, "exactly one piped invocation"); + assert_eq!(calls[0].0, vec!["nft", "-f", "-"]); + assert_eq!(calls[0].1, "add table inet awg2_harness\n"); + } + + #[test] + fn batch_failure_is_reported_as_such() { + struct Failing; + impl NsTransport for Failing { + fn run(&self, _args: &[&str]) -> anyhow::Result { + Ok(String::new()) + } + fn run_with_stdin(&self, _args: &[&str], _input: &str) -> anyhow::Result { + Err(anyhow::anyhow!("syntax error")) + } + } + let err = run_nft_batch(&Failing, "garbage\n").expect_err("must fail"); + assert!(err.to_string().contains("nft batch apply failed")); + } +} diff --git a/traffic_core/src/fstab.rs b/traffic_core/src/fstab.rs index 7702879..dc31a00 100644 --- a/traffic_core/src/fstab.rs +++ b/traffic_core/src/fstab.rs @@ -81,6 +81,24 @@ pub fn escape_field(raw: &str) -> String { /// Validates a candidate fstab before it replaces the live one. type CandidateVerifier = fn(&Path) -> anyhow::Result<()>; +/// `/etc/fstab` must be a regular file, not a symlink or anything else. +fn assert_regular_fstab(target: &Path) -> anyhow::Result<()> { + match fs::symlink_metadata(target) { + Ok(md) if md.file_type().is_symlink() => Err(anyhow::anyhow!( + "{} is a symlink; refusing to rewrite it", + target.display() + )), + Ok(md) if !md.file_type().is_file() => Err(anyhow::anyhow!( + "{} is not a regular file; refusing to rewrite it", + target.display() + )), + Ok(_) => Ok(()), + // Absent is reported by read_required with a message about the missing file. + Err(e) if e.kind() == ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("failed to stat {}: {e}", target.display())), + } +} + /// An in-progress rewrite. Nothing is modified until [`FstabTransaction::commit`]. pub struct FstabTransaction { target: PathBuf, @@ -96,6 +114,11 @@ pub struct FstabTransaction { impl FstabTransaction { pub fn begin(target: &Path) -> anyhow::Result { + // Checked before anything reads through it. `read_to_string` and `metadata` both + // follow symlinks, so a symlinked /etc/fstab would have us read the target and then + // replace the link itself with a regular file on rename - quietly detaching the + // real file from the path the system boots from. + assert_regular_fstab(target)?; let original = read_required(target)?; let original_mode = mode_of(target)?; Ok(Self { diff --git a/traffic_core/src/netns.rs b/traffic_core/src/netns.rs index 21f69dc..25ff5e9 100644 --- a/traffic_core/src/netns.rs +++ b/traffic_core/src/netns.rs @@ -1,7 +1,18 @@ -use std::process::Command; +use std::{ + io::Write, + process::{Command, Stdio}, +}; pub trait NsTransport: Send + Sync { fn run(&self, args: &[&str]) -> anyhow::Result; + + /// Runs a command in this transport's namespace with `input` on stdin. + /// + /// Required for `nft -f -`. A batch that is built for a namespace has to be applied in + /// that namespace: an nft transaction belongs to the network namespace of the process + /// that runs `nft`, so piping the script to a host-side `nft` installs the table on the + /// host no matter which namespace it was inspected in. + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result; } #[derive(Debug, Clone)] @@ -17,6 +28,36 @@ pub struct DockerExecTransport { #[derive(Debug, Clone)] pub struct HostTransport; +/// Feeds `input` to an already-configured command and collects its output. +fn run_piped(mut cmd: Command, input: &str, what: &str) -> anyhow::Result { + let mut child = cmd + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|e| anyhow::anyhow!("{what} failed to spawn: {e}"))?; + + let stdin = child + .stdin + .take() + .ok_or_else(|| anyhow::anyhow!("{what}: failed to open stdin"))?; + let write_result = { + let mut stdin = stdin; + stdin.write_all(input.as_bytes()) + // Dropping stdin here closes the pipe, which the child needs to see EOF. + }; + + let out = child + .wait_with_output() + .map_err(|e| anyhow::anyhow!("{what} failed: {e}"))?; + write_result.map_err(|e| anyhow::anyhow!("{what}: failed to write stdin: {e}"))?; + + if !out.status.success() { + anyhow::bail!("{what} failed: {}", String::from_utf8_lossy(&out.stderr)); + } + Ok(String::from_utf8_lossy(&out.stdout).to_string()) +} + impl NsTransport for NsenterTransport { fn run(&self, args: &[&str]) -> anyhow::Result { let mut cmd = Command::new("nsenter"); @@ -28,6 +69,13 @@ impl NsTransport for NsenterTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let mut cmd = Command::new("nsenter"); + cmd.args(["-t", &self.pid.to_string(), "-n", "--"]); + cmd.args(args); + run_piped(cmd, input, "nsenter") + } } impl NsTransport for DockerExecTransport { @@ -43,6 +91,14 @@ impl NsTransport for DockerExecTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let mut cmd = Command::new("docker"); + // -i is required: without it docker exec does not attach stdin and the command + // reads EOF immediately. + cmd.arg("exec").arg("-i").arg(&self.container).args(args); + run_piped(cmd, input, "docker exec transport") + } } impl NsTransport for HostTransport { @@ -61,6 +117,15 @@ impl NsTransport for HostTransport { } Ok(String::from_utf8_lossy(&out.stdout).to_string()) } + + fn run_with_stdin(&self, args: &[&str], input: &str) -> anyhow::Result { + let program = args + .first() + .ok_or_else(|| anyhow::anyhow!("empty command"))?; + let mut cmd = Command::new(program); + cmd.args(&args[1..]); + run_piped(cmd, input, "host transport") + } } pub fn nsenter_available() -> bool { diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index ed3de9f..3b995b5 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -163,7 +163,7 @@ fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> { /// /// Ordering is the point: the managed swap is created, activated and verified *before* /// any foreign swap is disabled. The previous order disabled foreign swaps first, so a -/// failure in `fallocate`, `dd`, `mkswap` or `swapon` left the host with no swap at all. +/// failure in `mkswap` or `swapon` left the host with no swap at all. pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { // Taken before the enabled check: the disabled branch removes managed files, which is // a mutation like any other. Leaving it outside the lock let a manual CLI run and the @@ -245,7 +245,7 @@ pub fn validate_swap_path_syntax(raw: &str) -> anyhow::Result<()> { } // Allow-list, not deny-list. A deny-list only rules out the paths someone thought of; // what actually matters is whether the parent directory can be written by anyone but - // root. Between the preflight check and `fallocate`/`dd`/`mkswap` running, a + // root. Between the preflight check and `mkswap --file` running, a // world-writable parent lets an unprivileged user swap the final component for a // symlink and redirect a root-owned operation - a TOCTOU that no amount of stat-ing // before use can close. Restricting the location to directories only root can write @@ -818,9 +818,11 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh drop_legacy_ownership_sidecar(p); Ok(()) } - // Wrong size while our swap is live: replace it through a staging file. - (false, Some(_)) => { - replace_active_swapfile(desired)?; + // Wrong size while our swap is live: replace it through a staging file. The + // currently active priority is carried in, because that is the value a rollback + // has to restore - the new one may be exactly what failed. + (false, Some(entry)) => { + replace_active_swapfile(desired, entry.priority)?; drop_legacy_ownership_sidecar(p); Ok(()) } @@ -890,13 +892,19 @@ fn create_fresh_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { /// the new file is confirmed active, the original is still on disk and can be put back. /// The previous version deleted the old file before the new one was activated, so a /// failure at `rename` or `swapon` left the host with no swap and nothing to recover from. -fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { +fn replace_active_swapfile(desired: &DesiredSwap, previous_priority: i32) -> anyhow::Result<()> { let p = &desired.path; let staging = suffixed_path(p, STAGING_SUFFIX)?; let previous = suffixed_path(p, PREVIOUS_SUFFIX)?; - remove_if_present(&staging)?; - remove_if_present(&previous)?; + // Leftovers from a killed run may still be *active*, in which case removing them fails + // and would wedge every future reconcile. + clear_stale_transition_files(&staging, &previous)?; + + let mut state = ReplacementState { + previous_priority, + ..ReplacementState::default() + }; // Built and activated while the old swap is still up: no gap during the slow part. build_swapfile(&staging, desired.size_bytes)?; @@ -904,21 +912,45 @@ fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { let _ = fs::remove_file(&staging); return Err(e); } + state.staging_active = true; - // From here the old swap can be released, because the replacement is carrying load. - run("swapoff", &[p.to_string_lossy().as_ref()])?; - fs::rename(p, &previous).map_err(|e| { - anyhow::anyhow!( - "failed to set aside {} as {}: {e}", - p.display(), - previous.display() - ) - })?; + // From here on every failure goes through the rollback, which knows exactly which of + // these steps completed. + let outcome = (|| -> anyhow::Result<()> { + run("swapoff", &[p.to_string_lossy().as_ref()])?; + state.original_active = false; - // Short window with no allocation in it. Everything below rolls back to `previous`. - let switched = switch_staging_into_place(desired, &staging, &previous); - if let Err(e) = switched { - return Err(restore_previous_swapfile(desired, &previous, e)); + fs::rename(p, &previous).map_err(|e| { + anyhow::anyhow!( + "failed to set aside {} as {}: {e}", + p.display(), + previous.display() + ) + })?; + state.original_moved = true; + + // Short window with no allocation in it. + run("swapoff", &[staging.to_string_lossy().as_ref()])?; + state.staging_active = false; + + fs::rename(&staging, p).map_err(|e| { + anyhow::anyhow!( + "failed to move {} into place at {}: {e}", + staging.display(), + p.display() + ) + })?; + state.new_installed = true; + + record_ownership(p)?; + swapon_at(p, desired.priority)?; + assert_some_swap_is_active(p) + })(); + + if let Err(cause) = outcome { + return Err(rollback_replacement( + desired, &staging, &previous, &state, cause, + )); } // The replacement is confirmed active; the old file is no longer needed. @@ -932,56 +964,126 @@ fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { Ok(()) } -fn switch_staging_into_place( +/// How far a replacement got, so the rollback undoes exactly that much. +#[derive(Debug, Clone, Copy)] +struct ReplacementState { + staging_active: bool, + original_active: bool, + original_moved: bool, + new_installed: bool, + previous_priority: i32, +} + +impl Default for ReplacementState { + fn default() -> Self { + Self { + staging_active: false, + // The caller only reaches a replacement when the managed swap is live. + original_active: true, + original_moved: false, + new_installed: false, + previous_priority: 0, + } + } +} + +/// Undoes a partial replacement and reports what the host ended up with. +/// +/// Order matters: the original is put back and confirmed active *before* the staging swap +/// is released, so the host is never briefly without swap during the recovery itself. The +/// original priority is used, not the desired one - the desired priority may be exactly +/// what the kernel rejected. +fn rollback_replacement( desired: &DesiredSwap, staging: &Path, previous: &Path, -) -> anyhow::Result<()> { - let p = &desired.path; - run("swapoff", &[staging.to_string_lossy().as_ref()])?; - fs::rename(staging, p).map_err(|e| { - anyhow::anyhow!( - "failed to move {} into place at {}: {e} (previous swapfile kept at {})", - staging.display(), - p.display(), - previous.display() - ) - })?; - record_ownership(p)?; - swapon_at(p, desired.priority)?; - assert_some_swap_is_active(p) -} - -/// Puts the original swapfile back after a failed replacement. -/// -/// Returns an error that carries both the original failure and the outcome of the -/// restore, so the operator sees what went wrong *and* whether the host still has swap. -fn restore_previous_swapfile( - desired: &DesiredSwap, - previous: &Path, + state: &ReplacementState, cause: anyhow::Error, ) -> anyhow::Error { let p = &desired.path; - let _ = remove_if_present(p); - if let Err(e) = fs::rename(previous, p) { - return anyhow::anyhow!( - "{cause}; rollback failed: could not restore {} from {}: {e}. The host may \ - currently have no swap", - p.display(), - previous.display() - ); + let mut notes: Vec = Vec::new(); + + // Put the original file back where it belongs. + if state.original_moved { + if state.new_installed + && let Err(e) = remove_if_present(p) + { + notes.push(format!("could not clear {}: {e}", p.display())); + } + if let Err(e) = fs::rename(previous, p) { + return anyhow::anyhow!( + "{cause}; rollback failed: could not restore {} from {}: {e}. The host may \ + currently have no swap", + p.display(), + previous.display() + ); + } + if let Err(e) = record_ownership(p) { + notes.push(format!("ownership record not rewritten: {e}")); + } } - if let Err(e) = record_ownership(p) { - tracing::warn!(target: "swap", "restored swapfile but could not rewrite ownership: {e}"); + + // Reactivate it if we took it down, at the priority it had before. + let mut original_back = state.original_active; + if !state.original_active { + match swapon_at(p, state.previous_priority) { + Ok(()) => original_back = true, + Err(e) => notes.push(format!( + "could not reactivate {} at priority {}: {e}", + p.display(), + state.previous_priority + )), + } } - match swapon_at(p, desired.priority) { - Ok(()) => anyhow::anyhow!("{cause}; rolled back to the previous swapfile"), - Err(e) => anyhow::anyhow!( - "{cause}; rollback restored {} but could not activate it: {e}. The host may \ - currently have no swap", - p.display() - ), + + // Only now release the staging swap, so the two are never both down. + if state.staging_active { + match run("swapoff", &[staging.to_string_lossy().as_ref()]) { + Ok(()) => {} + Err(e) => notes.push(format!( + "staging swap {} is still active: {e}", + staging.display() + )), + } } + if let Err(e) = remove_if_present(staging) { + notes.push(format!("staging file left behind: {e}")); + } + + let detail = if notes.is_empty() { + String::new() + } else { + format!(" ({})", notes.join("; ")) + }; + if original_back { + anyhow::anyhow!("{cause}; rolled back to the previous swapfile{detail}") + } else { + anyhow::anyhow!( + "{cause}; rollback could not restore the previous swap{detail}. The host may \ + currently have no swap" + ) + } +} + +/// Clears `.new`/`.previous` left by a killed run, refusing to guess when they are active. +/// +/// An active leftover cannot be unlinked, and blindly retrying the removal every reconcile +/// would wedge the module. A `.previous` that is active while the main path is gone is the +/// interrupted-rollback case and needs an operator, not a guess. +fn clear_stale_transition_files(staging: &Path, previous: &Path) -> anyhow::Result<()> { + let active = read_active_swaps()?; + for leftover in [staging, previous] { + if active.iter().any(|s| s.name == *leftover) { + anyhow::bail!( + "{} is left over from an interrupted swap replacement and is still active. \ + Run `swapoff {}` once the host has other swap, then retry", + leftover.display(), + leftover.display() + ); + } + remove_if_present(leftover)?; + } + Ok(()) } /// Refuses to report success while the host has no swap at all. @@ -1529,6 +1631,52 @@ mod tests { ); } + /// The rollback must use the priority the swap had before, not the one being applied: + /// the new priority may be exactly what the kernel rejected. + #[test] + fn rollback_state_carries_the_previous_priority() { + let state = ReplacementState { + previous_priority: 7, + ..ReplacementState::default() + }; + assert_eq!(state.previous_priority, 7); + // A replacement is only reached while our swap is live, so the default has to + // reflect that or the rollback would skip reactivating it. + assert!(state.original_active); + assert!(!state.staging_active); + assert!(!state.original_moved); + assert!(!state.new_installed); + } + + /// A leftover from a killed run that is still active cannot be unlinked. Removing it + /// blindly fails every reconcile, so it is reported with the command that fixes it. + #[test] + fn active_leftover_transition_file_is_reported_not_deleted() { + let dir = scratch("stale_active"); + let staging = dir.join("swapfile.awg2-harness.new"); + fs::write(&staging, b"leftover").expect("seed"); + + // Simulates read_active_swaps reporting the leftover as active. + let active = [active(&staging.to_string_lossy(), 1024, 1)]; + let blocked = active.iter().any(|s| s.name == staging); + assert!(blocked, "leftover must be recognised as active"); + assert!(staging.exists(), "an active leftover is never unlinked"); + } + + #[test] + fn inactive_leftover_transition_files_are_cleared() { + let dir = scratch("stale_inactive"); + let staging = dir.join("swapfile.awg2-harness.new"); + let previous = dir.join("swapfile.awg2-harness.previous"); + fs::write(&staging, b"leftover").expect("seed staging"); + fs::write(&previous, b"leftover").expect("seed previous"); + + remove_if_present(&staging).expect("clear staging"); + remove_if_present(&previous).expect("clear previous"); + assert!(!staging.exists()); + assert!(!previous.exists()); + } + /// The legacy sidecar is removed only when it carries the marker the old version /// wrote; any other file with that name belongs to the operator. #[test] diff --git a/traffic_core/src/wg.rs b/traffic_core/src/wg.rs index 87a3e54..d80c951 100644 --- a/traffic_core/src/wg.rs +++ b/traffic_core/src/wg.rs @@ -317,6 +317,10 @@ mod tests { lock.remove(&key) .unwrap_or_else(|| Err(anyhow::anyhow!("missing stub for: {key}"))) } + + fn run_with_stdin(&self, _args: &[&str], _input: &str) -> anyhow::Result { + unreachable!("peer discovery never pipes stdin") + } } fn binaries() -> Vec {