From b10a1fece20d0e38738f0ae55707834ee246dce0 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 20 Jun 2024 14:49:46 -0400 Subject: [PATCH] cli: Make sysroot lock automatically check root + setup mountns Previous to this change, we were always pairing together two functions: - `prepare_for_write()` - `get_locked_sysroot()` Make the latter automatically call the former (and make it private to the cli code). This will avoid bugs like https://github.com/containers/bootc/pull/595/commits/dcfc7521acaf21072032777b9683796ff710bdfe where we forgot to call the first one and didn't end up with an unshared mountns. While we're here, drop an unnecessary `async` from these functions, and just in case make `prepare_for_write()` idempotent. Signed-off-by: Colin Walters --- lib/src/cli.rs | 25 ++++++++++++++++++------- lib/src/install.rs | 2 +- lib/src/status.rs | 1 - 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index e169bc5a..1678d9e5 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -324,7 +324,7 @@ pub(crate) enum Opt { /// TODO use https://github.com/ostreedev/ostree/pull/2779 once /// we can depend on a new enough ostree #[context("Ensuring mountns")] -pub(crate) async fn ensure_self_unshared_mount_namespace() -> Result<()> { +pub(crate) fn ensure_self_unshared_mount_namespace() -> Result<()> { let uid = rustix::process::getuid(); if !uid.is_root() { tracing::debug!("Not root, assuming no need to unshare"); @@ -354,6 +354,7 @@ pub(crate) async fn ensure_self_unshared_mount_namespace() -> Result<()> { /// TODO drain this and the above into SysrootLock #[context("Acquiring sysroot")] pub(crate) async fn get_locked_sysroot() -> Result { + prepare_for_write()?; let sysroot = ostree::Sysroot::new_default(); sysroot.set_mount_namespace_in_use(); let sysroot = ostree_ext::sysroot::SysrootLock::new_from_sysroot(&sysroot).await?; @@ -375,8 +376,21 @@ pub(crate) fn require_root() -> Result<()> { } /// A few process changes that need to be made for writing. +/// IMPORTANT: This may end up re-executing the current process, +/// so anything that happens before this should be idempotent. #[context("Preparing for write")] -pub(crate) async fn prepare_for_write() -> Result<()> { +fn prepare_for_write() -> Result<()> { + use std::sync::atomic::{AtomicBool, Ordering}; + + // This is intending to give "at most once" semantics to this + // function. We should never invoke this from multiple threads + // at the same time, but verifying "on main thread" is messy. + // Yes, using SeqCst is likely overkill, but there is nothing perf + // sensitive about this. + static ENTERED: AtomicBool = AtomicBool::new(false); + if ENTERED.load(Ordering::SeqCst) { + return Ok(()); + } if ostree_ext::container_utils::is_ostree_container()? { anyhow::bail!( "Detected container (ostree base); this command requires a booted host system." @@ -389,17 +403,17 @@ pub(crate) async fn prepare_for_write() -> Result<()> { anyhow::bail!("This command requires an ostree-booted host system"); } crate::cli::require_root()?; - ensure_self_unshared_mount_namespace().await?; + ensure_self_unshared_mount_namespace()?; if crate::lsm::selinux_enabled()? && !crate::lsm::selinux_ensure_install()? { tracing::warn!("Do not have install_t capabilities"); } + ENTERED.store(true, Ordering::SeqCst); Ok(()) } /// Implementation of the `bootc upgrade` CLI command. #[context("Upgrading")] async fn upgrade(opts: UpgradeOpts) -> Result<()> { - prepare_for_write().await?; let sysroot = &get_locked_sysroot().await?; let repo = &sysroot.repo(); let (booted_deployment, _deployments, host) = @@ -539,7 +553,6 @@ async fn switch(opts: SwitchOpts) -> Result<()> { return Ok(()); } - prepare_for_write().await?; let cancellable = gio::Cancellable::NONE; let sysroot = &get_locked_sysroot().await?; @@ -585,7 +598,6 @@ async fn switch(opts: SwitchOpts) -> Result<()> { /// Implementation of the `bootc rollback` CLI command. #[context("Rollback")] async fn rollback(_opts: RollbackOpts) -> Result<()> { - prepare_for_write().await?; let sysroot = &get_locked_sysroot().await?; crate::deploy::rollback(sysroot).await } @@ -593,7 +605,6 @@ async fn rollback(_opts: RollbackOpts) -> Result<()> { /// Implementation of the `bootc edit` CLI command. #[context("Editing spec")] async fn edit(opts: EditOpts) -> Result<()> { - prepare_for_write().await?; let sysroot = &get_locked_sysroot().await?; let (booted_deployment, _deployments, host) = crate::status::get_status_require_booted(sysroot)?; diff --git a/lib/src/install.rs b/lib/src/install.rs index 499db932..bae4664f 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -1138,7 +1138,7 @@ async fn prepare_install( // Even though we require running in a container, the mounts we create should be specific // to this process, so let's enter a private mountns to avoid leaking them. if !external_source && std::env::var_os("BOOTC_SKIP_UNSHARE").is_none() { - super::cli::ensure_self_unshared_mount_namespace().await?; + super::cli::ensure_self_unshared_mount_namespace()?; } setup_sys_mount("efivarfs", EFIVARFS)?; diff --git a/lib/src/status.rs b/lib/src/status.rs index 3311b355..5b245504 100644 --- a/lib/src/status.rs +++ b/lib/src/status.rs @@ -305,7 +305,6 @@ pub(crate) async fn status(opts: super::cli::StatusOpts) -> Result<()> { let host = if !Utf8Path::new("/run/ostree-booted").try_exists()? { Default::default() } else { - crate::cli::prepare_for_write().await?; let sysroot = super::cli::get_locked_sysroot().await?; let booted_deployment = sysroot.booted_deployment(); let (_deployments, host) = get_status(&sysroot, booted_deployment.as_ref())?;