install: Only remove bootloader dirs from the ESP

crates/lib/src/install.rs

@@ -2384,19 +2384,27 @@ fn remove_all_in_dir_no_xdev(d: &Dir, mount_err: bool) -> Result<()> {
         let name = entry.file_name();
         let etype = entry.file_type()?;
         if etype == FileType::dir() {
-            if let Some(subdir) = d.open_dir_noxdev(&name)? {
-                remove_all_in_dir_no_xdev(&subdir, mount_err)?;
-                d.remove_dir(&name)?;
-            } else if mount_err {
-                anyhow::bail!("Found unexpected mount point {name:?}");
-            }
+            remove_dir_no_xdev(d, &name, mount_err)?;
         } else {
             d.remove_file_optional(&name)?;
         }
     }
     anyhow::Ok(())
 }
 
+/// Recursively remove the directory `name` in `d`, without crossing devices.
+/// A mount point is left alone, or is an error if `mount_err` is true.
+fn remove_dir_no_xdev(d: &Dir, name: impl AsRef<Path>, mount_err: bool) -> Result<()> {
+    let name = name.as_ref();
+    if let Some(subdir) = d.open_dir_noxdev(name)? {
+        remove_all_in_dir_no_xdev(&subdir, mount_err)?;
+        d.remove_dir(name)?;
+    } else if mount_err {
+        anyhow::bail!("Found unexpected mount point {name:?}");
+    }
+    Ok(())
+}
+
 #[context("Removing boot directory content except loader dir on ostree")]
 fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> {
     let entries = bootdir
@@ -2421,12 +2429,8 @@ fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> {
 
         let etype = entry.file_type()?;
         if etype == FileType::dir() {
-            // Open the directory and remove its contents
-            if let Some(subdir) = bootdir.open_dir_noxdev(&file_name)? {
-                remove_all_in_dir_no_xdev(&subdir, false)
-                    .with_context(|| format!("Removing directory contents: {}", file_name))?;
-                bootdir.remove_dir(&file_name)?;
-            }
+            remove_dir_no_xdev(bootdir, file_name, false)
+                .with_context(|| format!("Removing directory: {file_name}"))?;
         } else {
             bootdir
                 .remove_file_optional(&file_name)
@@ -2436,6 +2440,22 @@ fn remove_all_except_loader_dirs(bootdir: &Dir, is_ostree: bool) -> Result<()> {
     Ok(())
 }
 
+/// Remove the bootloader dirs (bootupd/grub, systemd-boot) from the ESP.
+/// Other content, e.g. Asahi's `m1n1/` and `vendorfw/`, may be firmware
+/// or earlier boot stages we cannot recreate, so it is preserved.
+// TODO: be more selective, e.g. keep other OSes' `EFI/<vendor>` and
+// non-bootc `loader/` entries, and drop old Type #1 kernels (see #2243).
+#[context("Removing bootloader content from EFI system partition")]
+fn clean_esp_bootloader_dirs(efidir: &Dir) -> Result<()> {
+    for name in ["EFI", "loader"] {
+        if efidir.try_exists(name)? {
+            remove_dir_no_xdev(efidir, name, false)
+                .with_context(|| format!("Removing directory: {name}"))?;
+        }
+    }
+    Ok(())
+}
+
 #[context("Removing boot directory content")]
 fn clean_boot_directories(rootfs: &Dir, rootfs_path: &Utf8Path, is_ostree: bool) -> Result<()> {
     let bootdir =
@@ -2450,13 +2470,12 @@ fn clean_boot_directories(rootfs: &Dir, rootfs_path: &Utf8Path, is_ostree: bool)
     // This should not remove /boot/efi note.
     remove_all_except_loader_dirs(&bootdir, is_ostree).context("Emptying /boot")?;
 
-    // TODO: we should also support not wiping the ESP.
     if ARCH_USES_EFI {
         if let Some(efidir) = bootdir
             .open_dir_optional(crate::bootloader::EFI_DIR)
             .context("Opening /boot/efi")?
         {
-            remove_all_in_dir_no_xdev(&efidir, false).context("Emptying EFI system partition")?;
+            clean_esp_bootloader_dirs(&efidir)?;
         }
     }
 
@@ -3157,6 +3176,32 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_clean_esp_bootloader_dirs() -> Result<()> {
+        let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?;
+
+        td.create_dir_all("EFI/BOOT")?;
+        td.write("EFI/BOOT/BOOTAA64.EFI", b"shim")?;
+        td.create_dir_all("loader/entries")?;
+        td.write("loader/entries/foo.conf", b"entry")?;
+        // Asahi content which must survive
+        td.create_dir_all("m1n1")?;
+        td.write("m1n1/boot.bin", b"m1n1")?;
+        td.write("ubootefi.var", b"efivars")?;
+
+        clean_esp_bootloader_dirs(&td)?;
+        assert!(!td.exists("EFI"));
+        assert!(!td.exists("loader"));
+        assert_eq!(td.read("m1n1/boot.bin")?, b"m1n1");
+        assert_eq!(td.read("ubootefi.var")?, b"efivars");
+
+        // Idempotent
+        clean_esp_bootloader_dirs(&td)?;
+        assert_eq!(td.entries()?.count(), 2);
+
+        Ok(())
+    }
+
     #[test]
     fn test_read_boot_fstab_entry() -> Result<()> {
         let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?;

docs/src/bootc-installation.7.md

@@ -216,8 +216,9 @@ Set the environment variable `BOOTC_DIRECT_IO=on` to create the loopback device
 
 This is a variant of `install to-filesystem`, which maximizes convenience for using
 an existing Linux system, converting it into the target container image.  Note that
-the `/boot` (and `/boot/efi`) partitions *will be reinitialized* - so this is a
-somewhat destructive operation for the existing Linux installation.
+the `/boot` partition and the `EFI/` and `loader/` directories of the ESP
+*will be reinitialized* - so this is a somewhat destructive operation for the
+existing Linux installation. Other ESP content (e.g. Asahi's `m1n1/`) is preserved.
 
 Also, because the filesystem is reused, it's required that the target system kernel
 support the root storage setup already initialized.

tmt/tests/booted/test-multi-device-esp.nu

@@ -16,7 +16,8 @@
 #   3. Three devices, partial ESP: Three disks, ESP on disk1+disk3 only
 #
 # Reboot 2:
-#   4. Single device (no LVM): ESP + root partition on a single disk
+#   4. Single device (no LVM): ESP + root partition on a single disk;
+#      also checks that non-bootloader ESP content is preserved
 #   5. No ESP anywhere: Two disks with no ESP; install should fail gracefully
 #
 # This validates the fix for https://github.com/bootc-dev/bootc/issues/481
@@ -157,6 +158,17 @@ def validate_esp [esp_partition: string] {
     }
 }
 
+# Mount an ESP partition, run a closure on the mountpoint, and unmount it
+def with_esp [esp_partition: string, f: closure] {
+    let esp_mount = "/var/mnt/esp_seed"
+    mkdir $esp_mount
+    mount $esp_partition $esp_mount
+    let r = (do $f $esp_mount)
+    umount $esp_mount
+    rmdir $esp_mount
+    $r
+}
+
 # Run bootc install to-existing-root from within the container image under test
 def run_install [mountpoint: string] {
     (podman run
@@ -356,8 +368,15 @@ def test_single_device_no_lvm [] {
         mkdir $mountpoint
         mount $"($loop1)p2" $mountpoint
 
-        # Create boot directory
-        mkdir $"($mountpoint)/boot"
+        # Create /boot/efi so the ESP gets mounted and cleaned
+        mkdir $"($mountpoint)/boot/efi"
+
+        # Seed non-bootloader content (as on Asahi) and a stale bootloader dir
+        with_esp $"($loop1)p1" {|esp|
+            mkdir $"($esp)/m1n1" $"($esp)/EFI/stale"
+            "m1n1" | save $"($esp)/m1n1/boot.bin"
+            "stale" | save $"($esp)/EFI/stale/x.efi"
+        }
 
         # Show block device hierarchy
         lsblk --pairs --paths --inverse --output NAME,TYPE $"($loop1)p2"
@@ -366,6 +385,12 @@ def test_single_device_no_lvm [] {
 
         # Validate ESP was installed correctly
         validate_esp $"($loop1)p1"
+        let r = (with_esp $"($loop1)p1" {|esp| {
+            m1n1: ($"($esp)/m1n1/boot.bin" | path exists)
+            stale: ($"($esp)/EFI/stale" | path exists)
+        }})
+        assert $r.m1n1 "m1n1/boot.bin was removed from the ESP"
+        assert (not $r.stale) "EFI/stale was not removed from the ESP"
     } catch {|e|
         cleanup_simple $loop1 $mountpoint
         rm -f $disk1