From 516e6209fcde734d6b8cdc5da48882c0c226ec23 Mon Sep 17 00:00:00 2001 From: Crimson Date: Fri, 7 Aug 2026 04:41:46 +0500 Subject: [PATCH] fix(swap): make every swap transition reversible [P0] The previous commit moved the slow part of a resize out of the swapless window but left the window itself unrecoverable. Both remaining transitions could take a host from a working state to no swap at all on a single failure. 1. Live resize destroyed the old file before the new one was active. The sequence ended: swapoff old, remove old, swapoff staging, rename staging into place, and only later swapon. A failure at the rename, at the ownership write, or at the activation left the old file already deleted and the staging file inactive - no swap, and nothing on disk to go back to. On a host given swap because memory is tight, this is the worst possible outcome of a routine size change. The old file is now moved aside as .awg2-harness.previous instead of deleted, and is only removed once the replacement is confirmed present in the active swap list. Every step past that point rolls back: the staging file is discarded, the previous file is renamed back, ownership is rewritten and the original priority reactivated. The returned error carries both the original cause and the outcome of the rollback, so the operator can see whether the host still has swap. A related failure mode is closed with it: rename succeeding but the ownership write failing left a file with a new inode and a stale record, which the next run correctly classified as StaleOwnership and refused to touch. Ownership is written inside the rolled-back section, so that state cannot persist. 2. A priority change could leave the host swapless. swapoff followed by swapon is two steps and the second can fail. Changing one integer in the config had no recovery path. change_swap_priority now reactivates at the previous priority if the new one is rejected, and says explicitly in the error whether the restore worked. 3. A partially created swapfile wedged the module. In the create-from-nothing branch, a failure after the file existed but before ownership was recorded left an unowned regular file at the managed path. The next run classified it as Foreign - correctly - and refused to proceed, so a transient mkswap failure required manual intervention. The file is created by this run inside an allowed root-owned location, so it is now removed on failure, leaving the host as it was found. Activation moved out of ensure_swap into the reconciliation branches. That is what makes the above possible: each branch owns both the old and the new file for the whole of its transition, so it can undo it. A transition also refuses to report success while the expected swap is not in the active list. Also in this commit, from the same audit: the legacy ownership sidecar was removed on the basis of being a regular file with the right name, without checking its contents, so a file an operator happened to create at that path would be deleted. It now goes through the same marker check as every other managed file - foreign files are left alone with a warning. Two tests added: the suffixed path helpers, and that the sidecar is removed only when it carries the old marker. cargo test --workspace: 180 + 4 passed. clippy -D warnings clean. --- traffic_core/src/swap.rs | 319 +++++++++++++++++++++++++++++---------- 1 file changed, 238 insertions(+), 81 deletions(-) diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index b8a3ccb..c040c56 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -56,8 +56,14 @@ const DEFAULT_SWAP_PATH: &str = "/swapfile"; const SWAP_OWNERSHIP_PATH: &str = "/var/lib/awg2-harness/swap-ownership.conf"; const SWAP_OWNERSHIP_MARKER: &str = "# Managed by awg2-harness swap-ownership"; const SWAP_OWNERSHIP_VERSION: u32 = 1; -/// Sidecar written by earlier versions next to the swapfile. Only ever removed. +/// Sidecar written by earlier versions next to the swapfile. Only ever removed, and only +/// when it carries the marker that version wrote. const LEGACY_OWNERSHIP_SUFFIX: &str = ".awg2-harness-owned"; +const LEGACY_OWNERSHIP_MARKER: &str = "# Managed by awg2-harness swap"; +/// Replacement being built while the current swapfile is still active. +const STAGING_SUFFIX: &str = ".awg2-harness.new"; +/// The outgoing swapfile, kept until the replacement is confirmed active. +const PREVIOUS_SUFFIX: &str = ".awg2-harness.previous"; /// Identity of a file on disk, independent of its name. /// @@ -173,9 +179,9 @@ pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { assert_swap_path_is_safe(&desired.path)?; let active_before = read_active_swaps()?; - // 2-3. Prepare and activate the managed swap. + // 2-3. Prepare and activate the managed swap. Activation belongs to this step so that + // every transition can roll back while it still owns both the old and the new file. ensure_desired_swapfile(&desired, &active_before)?; - enable_desired_swap(&desired)?; // 4. Confirm it is really live before touching anything else. let active_after = read_active_swaps()?; @@ -475,15 +481,17 @@ fn set_dir_mode(_dir: &Path) -> anyhow::Result<()> { /// Only removed, never trusted: ownership now lives in the state directory. A sidecar at /// a path we do not own is left alone. fn drop_legacy_ownership_sidecar(swap_path: &Path) { - let Some(name) = swap_path.file_name() else { + let Ok(sidecar) = suffixed_path(swap_path, LEGACY_OWNERSHIP_SUFFIX) else { return; }; - let mut sidecar = name.to_os_string(); - sidecar.push(LEGACY_OWNERSHIP_SUFFIX); - let sidecar = swap_path.with_file_name(sidecar); - match fs::symlink_metadata(&sidecar) { - Ok(md) if md.file_type().is_file() => { - if let Err(e) = fs::remove_file(&sidecar) { + // Goes through the same marker check as every other managed file. Removing any regular + // file that happened to match the name would delete an operator's file purely because + // of what it was called. + let file = ManagedFile::new(sidecar.clone(), LEGACY_OWNERSHIP_MARKER); + match file.ownership() { + Ok(Ownership::Absent) => {} + Ok(Ownership::Managed) => { + if let Err(e) = file.remove() { tracing::warn!( target: "swap", "could not remove legacy ownership sidecar {}: {e}", @@ -491,7 +499,16 @@ fn drop_legacy_ownership_sidecar(swap_path: &Path) { ); } } - _ => {} + Ok(Ownership::Foreign) => tracing::warn!( + target: "swap", + "{} exists but is not the legacy ownership sidecar; leaving it alone", + sidecar.display() + ), + Err(e) => tracing::warn!( + target: "swap", + "could not inspect {}: {e}", + sidecar.display() + ), } } @@ -710,20 +727,19 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh drop_legacy_ownership_sidecar(p); Ok(()) } - // Right file, right size, but the active priority differs. Re-activating is - // enough; the previous code fell through to a full recreate, so changing + // Right file, right size, but the active priority differs. A reactivation, not a + // rebuild - the previous code fell through to a full recreate, so changing // swap.priority destroyed and rebuilt a perfectly good multi-gigabyte file. - (true, Some(_)) => { - run("swapoff", &[p.to_string_lossy().as_ref()])?; + (true, Some(entry)) => { set_swapfile_mode(p)?; drop_legacy_ownership_sidecar(p); - Ok(()) + change_swap_priority(desired, entry.priority) } - // Right file, not active: the caller activates it next. + // Right file, not active: activate it. (true, None) => { set_swapfile_mode(p)?; drop_legacy_ownership_sidecar(p); - Ok(()) + swapon_at(p, desired.priority) } // Wrong size or no file at all, and nothing of ours is active: build in place. (false, None) => { @@ -731,74 +747,210 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh fs::remove_file(p) .map_err(|e| anyhow::anyhow!("failed to remove {}: {e}", p.display()))?; } - build_swapfile(p, desired.size_bytes)?; - record_ownership(p)?; + create_fresh_swapfile(desired)?; 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), + (false, Some(_)) => { + replace_active_swapfile(desired)?; + drop_legacy_ownership_sidecar(p); + Ok(()) + } } } -/// Resizes the managed swapfile while it is active, without a swapless gap during the -/// slow part. +/// Re-activates the managed swap at a new priority, restoring the old one on failure. /// -/// A live swapfile cannot be resized in place, so it has to be replaced. Doing that as -/// swapoff-then-create left the host with no swap for the whole duration of -/// `fallocate`/`dd`/`mkswap` - on a machine that was given swap precisely because memory -/// is tight, and with no way back if any of those steps failed. The allocation now happens -/// while the old swap is still serving, and the only gap left is the swapoff/rename/swapon -/// triple, which allocates nothing. +/// `swapoff` followed by `swapon` is two steps, and the second can fail - a bad priority, +/// a transient kernel error. Without the restore, changing one integer in the config could +/// leave the host with no swap at all. +fn change_swap_priority(desired: &DesiredSwap, previous_priority: i32) -> anyhow::Result<()> { + let p = &desired.path; + run("swapoff", &[p.to_string_lossy().as_ref()])?; + + let Err(e) = swapon_at(p, desired.priority) else { + return Ok(()); + }; + match swapon_at(p, previous_priority) { + Ok(()) => Err(anyhow::anyhow!( + "failed to activate {} with priority {}: {e}; restored the previous priority {}", + p.display(), + desired.priority, + previous_priority + )), + Err(re) => Err(anyhow::anyhow!( + "failed to activate {} with priority {} ({e}) and could not restore priority {} \ + ({re}); the host may currently have no swap", + p.display(), + desired.priority, + previous_priority + )), + } +} + +/// Builds and activates a swapfile where none of ours is running. +/// +/// The file is removed again if anything after its creation fails. Leaving it behind would +/// produce an unowned regular file at the managed path, which the next run correctly +/// classifies as `Foreign` and then refuses to touch - so a single transient `mkswap` +/// failure would wedge the module until an operator intervened. +fn create_fresh_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { + let p = &desired.path; + build_swapfile(p, desired.size_bytes)?; + + let prepared = record_ownership(p).and_then(|()| swapon_at(p, desired.priority)); + if let Err(e) = prepared { + // Created by this run inside an allowed root-owned location, so removing it is + // safe and leaves the host exactly as it was found. + if let Err(re) = fs::remove_file(p) { + return Err(anyhow::anyhow!( + "{e}; additionally failed to clean up the partially prepared {}: {re}", + p.display() + )); + } + let _ = ownership_file().remove(); + return Err(e); + } + Ok(()) +} + +/// Replaces a live managed swapfile with one of a different size. +/// +/// A live swapfile cannot be resized in place, so it has to be swapped out. Allocation and +/// `mkswap` happen while the old swap is still serving. The old file is then kept aside as +/// `.previous` rather than deleted, so every step after that point can be undone: until +/// 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<()> { let p = &desired.path; - let staging = staging_path(p)?; + let staging = suffixed_path(p, STAGING_SUFFIX)?; + let previous = suffixed_path(p, PREVIOUS_SUFFIX)?; - match fs::remove_file(&staging) { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => anyhow::bail!("failed to clear stale {}: {e}", staging.display()), - } + remove_if_present(&staging)?; + remove_if_present(&previous)?; - // Built and activated while the old swap is still up. + // Built and activated while the old swap is still up: no gap during the slow part. build_swapfile(&staging, desired.size_bytes)?; - if let Err(e) = run( - "swapon", - &[ - "-p", - &desired.priority.to_string(), - staging.to_string_lossy().as_ref(), - ], - ) { + if let Err(e) = swapon_at(&staging, desired.priority) { let _ = fs::remove_file(&staging); return Err(e); } - // The old file is only released once the replacement is carrying load. + // From here the old swap can be released, because the replacement is carrying load. run("swapoff", &[p.to_string_lossy().as_ref()])?; - fs::remove_file(p).map_err(|e| anyhow::anyhow!("failed to remove {}: {e}", p.display()))?; - - // Short gap: no allocation happens between here and the swapon below. - run("swapoff", &[staging.to_string_lossy().as_ref()])?; - fs::rename(&staging, p).map_err(|e| { + fs::rename(p, &previous).map_err(|e| { anyhow::anyhow!( - "failed to move {} into place at {}: {e}; activate it manually with swapon", - staging.display(), - p.display() + "failed to set aside {} as {}: {e}", + p.display(), + previous.display() ) })?; - record_ownership(p)?; - drop_legacy_ownership_sidecar(p); + + // 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)); + } + + // The replacement is confirmed active; the old file is no longer needed. + if let Err(e) = fs::remove_file(&previous) { + tracing::warn!( + target: "swap", + "swap replaced successfully but the old file {} could not be removed: {e}", + previous.display() + ); + } Ok(()) } -fn staging_path(path: &Path) -> anyhow::Result { +fn switch_staging_into_place( + 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, + 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() + ); + } + if let Err(e) = record_ownership(p) { + tracing::warn!(target: "swap", "restored swapfile but could not rewrite ownership: {e}"); + } + 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() + ), + } +} + +/// Refuses to report success while the host has no swap at all. +fn assert_some_swap_is_active(expected: &Path) -> anyhow::Result<()> { + let active = read_active_swaps()?; + if active.iter().any(|s| s.name == expected) { + return Ok(()); + } + anyhow::bail!( + "{} is not present in the active swap list after activation", + expected.display() + ) +} + +fn swapon_at(path: &Path, priority: i32) -> anyhow::Result<()> { + run( + "swapon", + &["-p", &priority.to_string(), path.to_string_lossy().as_ref()], + ) +} + +fn remove_if_present(path: &Path) -> anyhow::Result<()> { + match fs::remove_file(path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("failed to remove {}: {e}", path.display())), + } +} + +fn suffixed_path(path: &Path, suffix: &str) -> anyhow::Result { let name = path .file_name() .ok_or_else(|| anyhow::anyhow!("invalid swap.path: {}", path.display()))?; - let mut staging = name.to_os_string(); - staging.push(".awg2-harness.new"); - Ok(path.with_file_name(staging)) + let mut out = name.to_os_string(); + out.push(suffix); + Ok(path.with_file_name(out)) } /// Allocates a swapfile and formats it, with the restrictive mode applied before `mkswap` @@ -894,26 +1046,6 @@ fn render_fstab_line(path: &Path, priority: i32) -> String { ) } -fn enable_desired_swap(desired: &DesiredSwap) -> anyhow::Result<()> { - let active = read_active_swaps()?; - if let Some(found) = active.iter().find(|v| v.name == desired.path) - && active_swap_size_matches(found.size_bytes, desired.size_bytes) - && found.priority == desired.priority - { - return Ok(()); - } - - run( - "swapon", - &[ - "-p", - &desired.priority.to_string(), - desired.path.to_string_lossy().as_ref(), - ], - )?; - Ok(()) -} - fn apply_swappiness(v: u8) -> anyhow::Result<()> { run("sysctl", &["-w", &format!("vm.swappiness={v}")])?; let current = read_current_swappiness()?; @@ -1330,11 +1462,36 @@ mod tests { #[test] fn staging_path_is_a_sibling_of_the_swapfile() { - let staging = staging_path(Path::new("/var/lib/awg2-harness/swapfile")).expect("staging"); + let base = Path::new("/var/lib/awg2-harness/swapfile"); assert_eq!( - staging, + suffixed_path(base, STAGING_SUFFIX).expect("staging"), PathBuf::from("/var/lib/awg2-harness/swapfile.awg2-harness.new") ); + assert_eq!( + suffixed_path(base, PREVIOUS_SUFFIX).expect("previous"), + PathBuf::from("/var/lib/awg2-harness/swapfile.awg2-harness.previous") + ); + assert_eq!( + suffixed_path(base, LEGACY_OWNERSHIP_SUFFIX).expect("sidecar"), + PathBuf::from("/var/lib/awg2-harness/swapfile.awg2-harness-owned") + ); + } + + /// 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] + fn legacy_sidecar_is_only_removed_when_it_carries_the_old_marker() { + let dir = scratch("sidecar"); + let swapfile = dir.join("swapfile"); + let sidecar = dir.join("swapfile.awg2-harness-owned"); + + fs::write(&sidecar, "# something an operator wrote\n").expect("seed foreign"); + drop_legacy_ownership_sidecar(&swapfile); + assert!(sidecar.exists(), "a foreign file must not be removed"); + + fs::write(&sidecar, format!("{LEGACY_OWNERSHIP_MARKER}\n")).expect("seed ours"); + drop_legacy_ownership_sidecar(&swapfile); + assert!(!sidecar.exists(), "our own sidecar must be removed"); } /// Foreign swaps are identified by not being the managed path; the managed one is