mount, store: Handle pre-mounted ESP in status and upgrade paths

[dustinkirkland]

Fixes #2355.

bootc install to-filesystem writes
systemd.mount-extra=UUID=<ESP>:/boot:auto:ro into the deployment
cmdline; systemd's fstab-generator then mounts /boot ro at boot. Every
subsequent bootc status failed with

error: Status: Mounting /dev/vda1: Device or resource busy (os error 16)

and any code path that writes the ESP (BLS install, UKI install, gc)
would hit the same EBUSY on a pre-mounted host.

Split the ESP-mount primitive into two intent-typed helpers and thread
the choice through BootedStorage:

• mount_esp_readonly clones an existing mount when present and does a
  fresh mount otherwise; used by bootc status.
• mount_esp_writable clones an existing mount and, if the clone is
  ro, remounts it rw on the private clone only. Safe because bootc
  always runs in an unshared mount namespace
  (ensure_self_unshared_mount_namespace), so the remount does not
  affect the host's view of /boot. Used by upgrade/gc.

BootedStorage::new grows an EspAccess argument; status::get_host
passes ReadOnly, cli::get_storage passes ReadWrite. The raw
mount_esp primitive is now module-private, reachable only via the
typed wrappers.

Also adds bootc_mount::find_mount_target_by_source(dev) — a walk
over pid 1's mountinfo tree that returns the existing target path for
a source device — with a table-driven unit test covering top-level,
nested, absent, empty, and deeply-nested mountinfo shapes.

Verified live on QEMU x86_64 (systemd 261.2, linux-qemu-6.18-bootc,
/boot mounted ro via systemd.mount-extra). Before: bootc status
fails with EBUSY. After: full YAML and JSON output both work.

Assisted-by: Claude (Opus 4.7) — see #issuecomment-5138400886 and the
reply below for context on tooling and human review.

[cgwalters]

Write callers (mount_esp in bootc_composefs/boot.rs for
BLS entry install/gc) intentionally unchanged — they continue to fail
loudly on EBUSY, which is correct:

How would that be correct?

silently cloning a ro mount would cause atomic_write to fail with EROFS later, a worse UX than the EBUSY they see today. Handling ro /boot in the upgrade path is a
separate concern (needs remount-rw or unmount-first strategy).

No, I think we need to detect the case when the ESP is ro (like /sysroot) and create a temporary private mount. We're always in an unshared mountns, so no worries about remounting. Though it would be cleaner to always operate on a detached fd, but that's a distinct cleanup.

We should handle mount_esp_writable vs mount_esp_readonly more explicitly I think, i.e. bootc status shouldn't need writes.

---

This PR has all the hallmarks of LLM generation but is missing an Assisted-by: - that's not a problem per se but I am curious if it's intentional; I'd especially be curious what models/tools were used here, as the conclusion I think was wrong.

[cgwalters]

Also forgot to mention - thanks so much for the patch!

This is definitely a bootc bug where we're just not internally consistent on the ro handling for our ESP mounts.

[dustinkirkland]

Thanks Colin — right on both counts, and apologies for the missing
Assisted-by:; that's on me for not reading AGENTS.md first. Just
force-pushed a rework of the commit with the trailer added and the
approach revised:

• Added mount_esp_writable alongside mount_esp_readonly. If the ESP
  is pre-mounted ro (via systemd.mount-extra), we clone via
  open_tree(OPEN_TREE_CLONE) and then mount(NULL, target, NULL, MS_REMOUNT|MS_BIND|MS_NOEXEC|MS_NOSUID, NULL) on the private clone.
  Since we're always inside ensure_self_unshared_mount_namespace,
  that only affects our namespace, as you noted.
• Threaded an EspAccess intent through BootedStorage::new so the
  choice is explicit at the call site: status::get_host passes
  ReadOnly, cli::get_storage passes ReadWrite. The raw
  mount_esp primitive is now module-private, only reachable via the
  typed wrappers.
• BLS install and UKI install both go through mount_esp_writable now,
  so the ro-ESP case is handled there too, not just in status.
• Collapsed the five findmnt-walk unit tests into a single table-driven
  one per REVIEW.md.

Re: tooling — Claude Opus 4.7 via Claude Code. I authored issue #2355,
directed the design, and did the live verification on a real
bootc-composefs QEMU host (both before and after the fix, and again
after the rework). Happy to add more detail or split anything up if
you'd prefer the change land as separate commits.

[cgwalters]

I dug in with my agents (and my eyeballs), came up with this, WDYT?

From b9c99f85c99f1aff8792dfb4bb81bebaeb75e17e Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Fri, 31 Jul 2026 14:31:19 -0400
Subject: [PATCH] boot, mount: Fix mount_esp_writable to actually grant write
 access

`open_tree(OPEN_TREE_CLONE)` creates a mount that shares the
original's underlying superblock, and a bind remount only clears the
clone's own `MNT_READONLY` flag, never the shared superblock's
`SB_RDONLY` (`do_reconfigure_mnt()` in `fs/namespace.c` says as much).
So the existing `mount_esp_writable` silently produced a mount that
still failed writes with `EROFS`. Verified with a loopback vfat image.

Fix it to remount the *existing* mount read-write in place instead of
a private clone, the same handling `open_dir_remount_rw` already gives
`/sysroot`. This needs `find_mount_target_by_source` to look up mounts
in the caller's own mount namespace rather than pid 1's, since bootc
already runs unshared by the time these functions are called.

Apply the same fix to `mount_esp_at()`, used during install.

Use `rustix::mount::mount_remount()` directly instead of shelling out
to `mount(8)`, matching the rest of this module.

Assisted-by: AI
---
 crates/lib/src/bootc_composefs/boot.rs | 57 ++++++++++++--------------
 crates/mount/src/mount.rs              | 15 ++++---
 2 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/crates/lib/src/bootc_composefs/boot.rs b/crates/lib/src/bootc_composefs/boot.rs
index 5f8de1e123..feb14a7477 100644
--- a/crates/lib/src/bootc_composefs/boot.rs
+++ b/crates/lib/src/bootc_composefs/boot.rs
@@ -228,47 +228,44 @@ pub fn mount_esp_readonly(device: &str) -> Result<TempMount> {
 
 /// Get a read-write view of the ESP for the provided device, gracefully
 /// handling the case where the ESP is already mounted (possibly read-only)
-/// in the root mount namespace.
+/// in the current mount namespace.
 ///
-/// If the ESP is already mounted, that mount is cloned privately into a
-/// tempdir; if the clone came in read-only (e.g. because
-/// `systemd.mount-extra=UUID=...:/boot:auto:ro` was in the deployment
-/// cmdline), it is remounted rw on the private clone only. This is safe
-/// because bootc always runs in an unshared mount namespace (see
-/// `cli::ensure_self_unshared_mount_namespace`), so the remount does not
-/// affect the host's view. If the ESP is not already mounted, a fresh
-/// read-write mount is performed.
+/// If the ESP's device is already mounted, that mount is unconditionally
+/// remounted read-write *in place* first -- the same approach already
+/// used for `/sysroot` in `open_dir_remount_rw`, which likewise doesn't
+/// bother checking whether it's already writable before remounting.
+/// This matters because our own fresh `mount(2)` below would otherwise
+/// get `EBUSY` from the kernel's `get_tree_bdev_flags()` if the existing
+/// mount's `MS_RDONLY` state doesn't match what we're requesting (e.g.
+/// via a `systemd.mount-extra=UUID=...:/boot:auto:ro` cmdline karg
+/// written by `bootc install to-filesystem`). Note remounting a private
+/// *clone* of the existing mount would not be sufficient here: that only
+/// clears the clone's own mount-level read-only flag, not the shared
+/// superblock's, so writes through it would still fail with `EROFS`.
+/// Remounting the existing mount directly is safe because bootc always
+/// runs in its own unshared mount namespace (see
+/// `cli::ensure_self_unshared_mount_namespace`).
 pub fn mount_esp_writable(device: &str) -> Result<TempMount> {
-    let Some(existing) = bootc_mount::find_mount_target_by_source(device)? else {
-        return mount_esp(device);
-    };
-    let mount = TempMount::clone_existing_mount(&existing)?;
-    let target = mount.dir.path();
-    let st =
-        rustix::fs::statvfs(target).with_context(|| format!("statvfs {}", target.display()))?;
-    if st.f_flag.contains(rustix::fs::StatVfsMountFlags::RDONLY) {
-        rustix::mount::mount_remount(target, MountFlags::BIND | ESP_MOUNT_FLAGS, "").with_context(
-            || {
-                format!(
-                    "Remounting cloned ESP mount at {} read-write",
-                    target.display()
-                )
-            },
-        )?;
-        tracing::debug!(
-            "Cloned ESP mount at {} was read-only; remounted rw in private namespace",
-            target.display()
-        );
+    if let Some(existing) = bootc_mount::find_mount_target_by_source(device)? {
+        rustix::mount::mount_remount(existing.as_str(), ESP_MOUNT_FLAGS, "")
+            .with_context(|| format!("Remounting {existing} read-write"))?;
     }
-    Ok(mount)
+    mount_esp(device)
 }
 
 /// Mount the ESP from `device` at the given path and return a guard that
 /// synchronously unmounts (and flushes) it on drop.
+///
+/// Applies the same pre-emptive remount as `mount_esp_writable`; see
+/// there for details and caveats.
 pub(crate) fn mount_esp_at(
     device: &str,
     path: std::path::PathBuf,
 ) -> Result<bootc_mount::tempmount::MountGuard> {
+    if let Some(existing) = bootc_mount::find_mount_target_by_source(device)? {
+        rustix::mount::mount_remount(existing.as_str(), ESP_MOUNT_FLAGS, "")
+            .with_context(|| format!("Remounting {existing} read-write"))?;
+    }
     bootc_mount::tempmount::MountGuard::mount(
         device,
         path,
diff --git a/crates/mount/src/mount.rs b/crates/mount/src/mount.rs
index f6ea9e00eb..2a87c769cb 100644
--- a/crates/mount/src/mount.rs
+++ b/crates/mount/src/mount.rs
@@ -126,16 +126,21 @@ pub fn is_mounted_in_pid1_mountns(path: &str) -> Result<bool> {
     Ok(mounted)
 }
 
-/// Find the mount target of a given source device in the root mount
+/// Find the mount target of a given source device in the *current* mount
 /// namespace. Returns `Ok(None)` if the device is not mounted.
 ///
 /// Used by callers that want to gracefully handle the case where a
 /// device they intended to mount is already mounted somewhere else
-/// (which would cause a fresh `mount(2)` to return `EBUSY`). Combined
-/// with `TempMount::clone_existing_mount`, this lets read-only callers
-/// like `bootc status` reuse an existing mount without disturbing it.
+/// (which would cause a fresh `mount(2)` to return `EBUSY`). Note this
+/// intentionally queries the caller's own mount namespace, not pid 1's
+/// (unlike `is_mounted_in_pid1_mountns`, which exists to answer a
+/// different question: whether the *host* already has something
+/// mounted before bootc unshares its own namespace). Callers of this
+/// function are expected to have already unshared their own mount
+/// namespace and are looking up a mount they intend to operate on
+/// directly (e.g. remount) in that same namespace.
 pub fn find_mount_target_by_source(dev: &str) -> Result<Option<camino::Utf8PathBuf>> {
-    let o = pid1_mounts()?;
+    let o = run_findmnt(&[], None, None)?;
     let found = find_source_mount_target(dev, &o.filesystems);
     if found.is_none() {
         tracing::debug!(
-- 
2.52.0

Agree and OK to add that to this PR?