BLS shared entries GC fix

crates/lib/src/bootc_composefs/boot.rs

@@ -116,8 +116,7 @@ use crate::{
 };
 use crate::{
     composefs_consts::{
-        BOOT_LOADER_ENTRIES, ORIGIN_KEY_BOOT, ORIGIN_KEY_BOOT_DIGEST, STAGED_BOOT_LOADER_ENTRIES,
-        STATE_DIR_ABS, USER_CFG, USER_CFG_STAGED,
+        BOOT_LOADER_ENTRIES, STAGED_BOOT_LOADER_ENTRIES, USER_CFG, USER_CFG_STAGED,
     },
     spec::{Bootloader, Host},
 };
@@ -328,6 +327,27 @@ fn compute_boot_digest(
     Ok(hex::encode(digest))
 }
 
+#[context("Computing boot digest for Type1 entries")]
+fn compute_boot_digest_type1(dir: &Dir) -> Result<String> {
+    let mut vmlinuz = dir
+        .open(VMLINUZ)
+        .with_context(|| format!("Opening {VMLINUZ}"))?;
+
+    let mut initrd = dir
+        .open(INITRD)
+        .with_context(|| format!("Opening {INITRD}"))?;
+
+    let mut hasher = openssl::hash::Hasher::new(openssl::hash::MessageDigest::sha256())
+        .context("Creating hasher")?;
+
+    std::io::copy(&mut vmlinuz, &mut hasher)?;
+    std::io::copy(&mut initrd, &mut hasher)?;
+
+    let digest: &[u8] = &hasher.finish().context("Finishing digest")?;
+
+    Ok(hex::encode(digest))
+}
+
 /// Compute SHA256Sum of .linux + .initrd section of the UKI
 ///
 /// # Arguments
@@ -355,52 +375,35 @@ pub(crate) fn compute_boot_digest_uki(uki: &[u8]) -> Result<String> {
 /// Given the SHA256 sum of current VMlinuz + Initrd combo, find boot entry with the same SHA256Sum
 ///
 /// # Returns
-/// Returns the verity of all deployments that have a boot digest same as the one passed in
+/// Returns the directory name that has the same sha256 digest for vmlinuz + initrd as the one
+/// that's passed in
 #[context("Checking boot entry duplicates")]
-pub(crate) fn find_vmlinuz_initrd_duplicates(digest: &str) -> Result<Option<Vec<String>>> {
-    let deployments = Dir::open_ambient_dir(STATE_DIR_ABS, ambient_authority());
-
-    let deployments = match deployments {
-        Ok(d) => d,
-        // The first ever deployment
-        Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None),
-        Err(e) => anyhow::bail!(e),
-    };
-
-    let mut symlink_to: Option<Vec<String>> = None;
-
-    for depl in deployments.entries()? {
-        let depl = depl?;
-
-        let depl_file_name = depl.file_name();
-        let depl_file_name = depl_file_name.as_str()?;
-
-        let config = depl
-            .open_dir()
-            .with_context(|| format!("Opening {depl_file_name}"))?
-            .read_to_string(format!("{depl_file_name}.origin"))
-            .context("Reading origin file")?;
+pub(crate) fn find_vmlinuz_initrd_duplicate(
+    storage: &Storage,
+    digest: &str,
+) -> Result<Option<String>> {
+    let boot_dir = storage.bls_boot_binaries_dir()?;
+
+    for entry in boot_dir.entries_utf8()? {
+        let entry = entry?;
+        let dir_name = entry.file_name()?;
+
+        if !entry.file_type()?.is_dir() {
+            continue;
+        }
 
-        let ini = tini::Ini::from_string(&config)
-            .with_context(|| format!("Failed to parse file {depl_file_name}.origin as ini"))?;
+        let Some(..) = dir_name.strip_prefix(TYPE1_BOOT_DIR_PREFIX) else {
+            continue;
+        };
 
-        match ini.get::<String>(ORIGIN_KEY_BOOT, ORIGIN_KEY_BOOT_DIGEST) {
-            Some(hash) => {
-                if hash == digest {
-                    match symlink_to {
-                        Some(ref mut prev) => prev.push(depl_file_name.to_string()),
-                        None => symlink_to = Some(vec![depl_file_name.to_string()]),
-                    }
-                }
-            }
+        let entry_digest = compute_boot_digest_type1(&boot_dir.open_dir(&dir_name)?)?;
 
-            // No SHASum recorded in origin file
-            // `symlink_to` is already none, but being explicit here
-            None => symlink_to = None,
-        };
+        if entry_digest == digest {
+            return Ok(Some(dir_name));
+        }
     }
 
-    Ok(symlink_to)
+    Ok(None)
 }
 
 #[context("Writing BLS entries to disk")]
@@ -687,45 +690,20 @@ pub(crate) fn setup_composefs_bls_boot(
                     options: Some(cmdline_refs),
                 });
 
-            match find_vmlinuz_initrd_duplicates(&boot_digest)? {
-                Some(shared_entries) => {
+            let shared_entry = match setup_type {
+                BootSetupType::Setup(_) => None,
+                BootSetupType::Upgrade((storage, ..)) => {
+                    find_vmlinuz_initrd_duplicate(storage, &boot_digest)?
+                }
+            };
+
+            match shared_entry {
+                Some(shared_entry) => {
                     // Multiple deployments could be using the same kernel + initrd, but there
                     // would be only one available
                     //
                     // Symlinking directories themselves would be better, but vfat does not support
                     // symlinks
-
-                    let mut shared_entry: Option<String> = None;
-
-                    let entries =
-                        Dir::open_ambient_dir(entry_paths.entries_path, ambient_authority())
-                            .context("Opening entries path")?
-                            .entries_utf8()
-                            .context("Getting dir entries")?;
-
-                    for ent in entries {
-                        let ent = ent?;
-                        // We shouldn't error here as all our file names are UTF-8 compatible
-                        let ent_name = ent.file_name()?;
-
-                        let Some(entry_verity_part) = ent_name.strip_prefix(TYPE1_BOOT_DIR_PREFIX)
-                        else {
-                            // Not our directory
-                            continue;
-                        };
-
-                        if shared_entries
-                            .iter()
-                            .any(|shared_ent| shared_ent == entry_verity_part)
-                        {
-                            shared_entry = Some(ent_name);
-                            break;
-                        }
-                    }
-
-                    let shared_entry = shared_entry
-                        .ok_or_else(|| anyhow::anyhow!("Shared boot binaries not found"))?;
-
                     match bls_config.cfg_type {
                         BLSConfigType::NonEFI {
                             ref mut linux,

crates/lib/src/bootc_composefs/gc.rs

@@ -91,11 +91,11 @@ fn collect_uki_binaries(boot_dir: &Dir, boot_binaries: &mut Vec<BootBinary>) ->
         let entry = entry?;
         let name = entry.file_name()?;
 
-        let Some(verity) = name.strip_prefix(UKI_NAME_PREFIX) else {
+        let Some(efi_name_no_prefix) = name.strip_prefix(UKI_NAME_PREFIX) else {
             continue;
         };
 
-        if name.ends_with(EFI_EXT) {
+        if let Some(verity) = efi_name_no_prefix.strip_suffix(EFI_EXT) {
             boot_binaries.push((BootType::Uki, verity.into()));
         }
     }
@@ -235,7 +235,11 @@ pub(crate) async fn composefs_gc(
             // filter the ones that are not referenced by any bootloader entry
             !bootloader_entries
                 .iter()
-                .any(|boot_entry| bin_path.1 == *boot_entry)
+                // We compare the name of directory containing the binary instead of comparing the
+                // fsverity digest. This is because a shared entry might differing directory
+                // name and fsverity digest in the cmdline. And since we want to GC the actual
+                // binaries, we compare with the directory name
+                .any(|boot_entry| boot_entry.boot_artifact_name == bin_path.1)
         })
         .collect::<Vec<_>>();
 
@@ -263,11 +267,28 @@ pub(crate) async fn composefs_gc(
 
     // Collect the deployments that have an image but no bootloader entry
     // and vice versa
-    let img_bootloader_diff = images
+    //
+    // Images without corresponding bootloader entries
+    let orphaned_images: Vec<&String> = images
         .iter()
-        .filter(|i| !bootloader_entries.contains(i))
-        .chain(bootloader_entries.iter().filter(|b| !images.contains(b)))
-        .collect::<Vec<_>>();
+        .filter(|image| {
+            !bootloader_entries
+                .iter()
+                .any(|entry| &entry.fsverity == *image)
+        })
+        .collect();
+
+    // Bootloader entries without corresponding images
+    let orphaned_bootloader_entries: Vec<&String> = bootloader_entries
+        .iter()
+        .map(|entry| &entry.fsverity)
+        .filter(|verity| !images.contains(verity))
+        .collect();
+
+    let img_bootloader_diff: Vec<&String> = orphaned_images
+        .into_iter()
+        .chain(orphaned_bootloader_entries)
+        .collect();
 
     tracing::debug!("img_bootloader_diff: {img_bootloader_diff:#?}");
 

crates/lib/src/bootc_composefs/status.rs

@@ -121,6 +121,26 @@ pub(crate) struct StagedDeployment {
     pub(crate) finalization_locked: bool,
 }
 
+#[derive(Debug, PartialEq)]
+pub(crate) struct BootloaderEntry {
+    /// The fsverity digest associated with the bootloader entry
+    /// This is the value of composefs= param
+    pub(crate) fsverity: String,
+    /// The name of the (UKI/Kernel+Initrd directory) related to the entry
+    ///
+    /// For UKI, this is the name of the UKI stripped of our custom
+    /// prefix and .efi suffix
+    ///
+    /// For Type1 entries, this is the name to the directory containing
+    /// Kernel+Initrd, stripped of our custom prefix
+    ///
+    /// Since this is stripped of all our custom prefixes + file extensions
+    /// this is basically the verity digest part of the name
+    ///
+    /// We mainly need this in order to GC shared Type1 entries
+    pub(crate) boot_artifact_name: String,
+}
+
 /// Detect if we have `composefs=<digest>` in `/proc/cmdline`
 pub(crate) fn composefs_booted() -> Result<Option<&'static ComposefsCmdline>> {
     static CACHED_DIGEST_VALUE: OnceLock<Option<ComposefsCmdline>> = OnceLock::new();
@@ -263,7 +283,7 @@ fn get_sorted_type1_boot_entries_helper(
     Ok(all_configs)
 }
 
-fn list_type1_entries(boot_dir: &Dir) -> Result<Vec<String>> {
+fn list_type1_entries(boot_dir: &Dir) -> Result<Vec<BootloaderEntry>> {
     // Type1 Entry
     let boot_entries = get_sorted_type1_boot_entries(boot_dir, true)?;
 
@@ -274,7 +294,12 @@ fn list_type1_entries(boot_dir: &Dir) -> Result<Vec<String>> {
     boot_entries
         .into_iter()
         .chain(staged_boot_entries)
-        .map(|entry| entry.get_verity())
+        .map(|entry| {
+            Ok(BootloaderEntry {
+                fsverity: entry.get_verity()?,
+                boot_artifact_name: entry.boot_artifact_name()?.to_string(),
+            })
+        })
         .collect::<Result<Vec<_>, _>>()
 }
 
@@ -283,7 +308,7 @@ fn list_type1_entries(boot_dir: &Dir) -> Result<Vec<String>> {
 /// # Returns
 /// The fsverity of EROFS images corresponding to boot entries
 #[fn_error_context::context("Listing bootloader entries")]
-pub(crate) fn list_bootloader_entries(storage: &Storage) -> Result<Vec<String>> {
+pub(crate) fn list_bootloader_entries(storage: &Storage) -> Result<Vec<BootloaderEntry>> {
     let bootloader = get_bootloader()?;
     let boot_dir = storage.require_boot_dir()?;
 
@@ -304,8 +329,13 @@ pub(crate) fn list_bootloader_entries(storage: &Storage) -> Result<Vec<String>>
                 boot_entries
                     .into_iter()
                     .chain(boot_entries_staged)
-                    .map(|entry| entry.get_verity())
-                    .collect::<Result<Vec<_>, _>>()?
+                    .map(|entry| {
+                        Ok(BootloaderEntry {
+                            fsverity: entry.get_verity()?,
+                            boot_artifact_name: entry.boot_artifact_name()?,
+                        })
+                    })
+                    .collect::<Result<Vec<_>, anyhow::Error>>()?
             } else {
                 list_type1_entries(boot_dir)?
             }
@@ -739,7 +769,11 @@ async fn composefs_deployment_status_from(
     // Rollback deployment is in here, but may also contain stale deployment entries
     let mut extra_deployment_boot_entries: Vec<BootEntry> = Vec::new();
 
-    for verity_digest in bootloader_entry_verity {
+    for BootloaderEntry {
+        fsverity: verity_digest,
+        ..
+    } in bootloader_entry_verity
+    {
         // read the origin file
         let config = state_dir
             .open_dir(&verity_digest)
@@ -877,6 +911,7 @@ async fn composefs_deployment_status_from(
                 .map(|menu| menu.get_verity()),
         )
         .collect::<Result<HashSet<_>>>()?;
+
     let rollback_candidates: Vec<_> = extra_deployment_boot_entries
         .into_iter()
         .filter(|entry| {