From 9d16a26feab9911ca5fd4720df756edcdd49a6bd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 8 Jan 2025 16:27:23 -0500 Subject: [PATCH 1/3] Catch and ignore SIGTERM during update operations Since our updates are non-transactional in general, we should at least be robust against some concurrent invocation of e.g. `reboot`. Replaces: https://github.com/coreos/bootupd/pull/811 Signed-off-by: Colin Walters --- Cargo.lock | 10 ++++++++++ Cargo.toml | 1 + src/backend/statefile.rs | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 289c68c7..84555d97 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -149,6 +149,7 @@ dependencies = [ "rustix", "serde", "serde_json", + "signal-hook-registry", "tempfile", "walkdir", "widestring", @@ -899,6 +900,15 @@ dependencies = [ "digest", ] +[[package]] +name = "signal-hook-registry" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a9e9e0b4211b72e7b8b6e85c807d36c212bdb33ea8587f7569562a84df5465b1" +dependencies = [ + "libc", +] + [[package]] name = "strsim" version = "0.11.1" diff --git a/Cargo.toml b/Cargo.toml index 4c8cee1d..d0511551 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ serde_json = "^1.0" tempfile = "^3.14" widestring = "1.1.0" walkdir = "2.3.2" +signal-hook-registry = "1.4.2" [profile.release] # We assume we're being delivered via e.g. RPM which supports split debuginfo diff --git a/src/backend/statefile.rs b/src/backend/statefile.rs index 379c472c..02b4f5be 100644 --- a/src/backend/statefile.rs +++ b/src/backend/statefile.rs @@ -9,6 +9,25 @@ use std::fs::File; use std::io::prelude::*; use std::path::Path; +/// Suppress SIGTERM while active +// TODO: In theory we could record if we got SIGTERM and exit +// on drop, but in practice we don't care since we're going to exit anyways. +#[derive(Debug)] +struct SignalTerminationGuard(signal_hook_registry::SigId); + +impl SignalTerminationGuard { + pub(crate) fn new() -> Result { + let signal = unsafe { signal_hook_registry::register(libc::SIGTERM, || {})? }; + Ok(Self(signal)) + } +} + +impl Drop for SignalTerminationGuard { + fn drop(&mut self) { + signal_hook_registry::unregister(self.0); + } +} + impl SavedState { /// System-wide bootupd write lock (relative to sysroot). const WRITE_LOCK_PATH: &'static str = "run/bootupd-lock"; @@ -27,6 +46,7 @@ impl SavedState { lockfile.lock_exclusive()?; let guard = StateLockGuard { sysroot, + termguard: Some(SignalTerminationGuard::new()?), lockfile: Some(lockfile), }; Ok(guard) @@ -37,6 +57,7 @@ impl SavedState { pub(crate) fn unlocked(sysroot: openat::Dir) -> Result { Ok(StateLockGuard { sysroot, + termguard: None, lockfile: None, }) } @@ -91,6 +112,8 @@ impl SavedState { pub(crate) struct StateLockGuard { pub(crate) sysroot: openat::Dir, #[allow(dead_code)] + termguard: Option, + #[allow(dead_code)] lockfile: Option, } From 9c568398509a5ba9074ce21a0a1fb56eb34da895 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 8 Jan 2025 16:31:20 -0500 Subject: [PATCH 2/3] Set KillMode=mixed Any child processes we fork are not long running, so they don't need their own individual SIGTERM. Doing things this way ensures that when we suppress SIGTERM for our process that's sufficient to ensure the whole unit runs uninterrupted. Signed-off-by: Colin Walters --- src/cli/bootupctl.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cli/bootupctl.rs b/src/cli/bootupctl.rs index ab30decb..44cd56e2 100644 --- a/src/cli/bootupctl.rs +++ b/src/cli/bootupctl.rs @@ -13,6 +13,10 @@ static SYSTEMD_ARGS_BOOTUPD: &[&str] = &[ "PrivateNetwork=yes", "--property", "ProtectHome=yes", + // While only our main process during update catches SIGTERM, we don't + // want systemd to send it to other processes. + "--property", + "KillMode=mixed", "--property", "MountFlags=slave", "--pipe", From c9b7964acf7bc6ff27cc6fdf0d02d9817945443d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 8 Jan 2025 16:42:52 -0500 Subject: [PATCH 3/3] Sync contrib service with static systemd args And add comments in both places so we remember to keep them in sync. Signed-off-by: Colin Walters --- contrib/packaging/bootloader-update.service | 4 ++++ src/cli/bootupctl.rs | 18 ++++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/contrib/packaging/bootloader-update.service b/contrib/packaging/bootloader-update.service index 065f1730..5b6f487c 100644 --- a/contrib/packaging/bootloader-update.service +++ b/contrib/packaging/bootloader-update.service @@ -6,6 +6,10 @@ Documentation=https://github.com/coreos/bootupd Type=oneshot ExecStart=/usr/bin/bootupctl update RemainAfterExit=yes +# Keep this stuff in sync with SYSTEMD_ARGS_BOOTUPD in general +PrivateNetwork=yes +ProtectHome=yes +KillMode=mixed MountFlags=slave [Install] diff --git a/src/cli/bootupctl.rs b/src/cli/bootupctl.rs index 44cd56e2..8b42237c 100644 --- a/src/cli/bootupctl.rs +++ b/src/cli/bootupctl.rs @@ -6,20 +6,17 @@ use log::LevelFilter; use std::os::unix::process::CommandExt; use std::process::{Command, Stdio}; -static SYSTEMD_ARGS_BOOTUPD: &[&str] = &[ - "--unit", - "bootupd", - "--property", +static SYSTEMD_ARGS_BOOTUPD: &[&str] = &["--unit", "bootupd", "--pipe"]; + +/// Keep these properties (isolation/runtime state) in sync with +/// the systemd units in contrib/packaging/*.service +static SYSTEMD_PROPERTIES: &[&str] = &[ "PrivateNetwork=yes", - "--property", "ProtectHome=yes", // While only our main process during update catches SIGTERM, we don't // want systemd to send it to other processes. - "--property", "KillMode=mixed", - "--property", "MountFlags=slave", - "--pipe", ]; /// `bootupctl` sub-commands. @@ -171,6 +168,11 @@ fn ensure_running_in_systemd() -> Result<()> { .wait()?; let r = Command::new("systemd-run") .args(SYSTEMD_ARGS_BOOTUPD) + .args( + SYSTEMD_PROPERTIES + .into_iter() + .flat_map(|&v| ["--property", v]), + ) .args(std::env::args()) .exec(); // If we got here, it's always an error