From d340324785bf7f8a78b28c670a754bc884e8b9e8 Mon Sep 17 00:00:00 2001 From: Mauro Ezequiel Moltrasio Date: Tue, 21 Jul 2026 15:52:41 +0200 Subject: [PATCH] chore(host_scanner): improve cleanup algorithm in scans In order to remove items from the inode maps for files that may have been deleted and may have missed events, or paths that are no longer monitored due to a configuration change, we used to: * Iterate over the userspace map. * Check if the path matches the configured globs. * Check if the path still exists in the filesystem (via statx). * If either of the previous checks failed, we remove the inode from the maps After this cleanup is done, we proceed to glob the configured paths and check all of them on disk, adding any missing inodes and updating any changed paths. The most common situation for these scans is that most inodes should hit (unless the configuration is changed or massive changes to the fs were missed), which means we are essentially doing two statx calls per element in the inode (once during the cleanup iteration and another during the glob expansion). This change does a few things: * Add a marker to each tracked inode (a counter that increments every scan iteration). * Do the glob expansion first, marking each found inode with the current scan number. * Once we are done exploring the glob expansion, iterate over the map and remove any entries that have an out of date scan marker (i.e: they were not found as part of the glob expansion). This saves us some `statx` calls during scans, making them faster. Of note, this change works because scanning pauses processing of events until the scan is done, otherwise it could be subject to race conditions. --- fact/src/host_scanner.rs | 91 +++++++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 34 deletions(-) diff --git a/fact/src/host_scanner.rs b/fact/src/host_scanner.rs index b0415a0d..326701bf 100644 --- a/fact/src/host_scanner.rs +++ b/fact/src/host_scanner.rs @@ -20,6 +20,7 @@ use std::{ cell::RefCell, + collections::HashMap, fs::Metadata, io, os::linux::fs::MetadataExt, @@ -30,7 +31,7 @@ use std::{ use anyhow::{Context, bail}; use aya::{ - maps::{MapData, MapError}, + maps::{HashMap as AyaHashMap, MapData, MapError}, sys::SyscallError, }; use fact_ebpf::{inode_key_t, inode_value_t, monitored_t}; @@ -50,8 +51,10 @@ use crate::{ }; pub struct HostScanner { - kernel_inode_map: RefCell>, - inode_map: RefCell>, + kernel_inode_map: RefCell>, + inode_map: RefCell>, + + scan_count: RefCell, paths: watch::Receiver>, scan_interval: watch::Receiver, @@ -82,6 +85,7 @@ impl HostScanner { let host_scanner = HostScanner { kernel_inode_map, inode_map, + scan_count: RefCell::new(0), paths, scan_interval, running, @@ -113,32 +117,45 @@ impl HostScanner { Ok(builder.build()?) } + fn new_scan(&self) -> u8 { + let mut scan_count = self.scan_count.borrow_mut(); + *scan_count += 1; + *scan_count + } + fn scan(&self) -> anyhow::Result<()> { info!("Host scan started"); let start = Instant::now(); self.metrics.scan_inc(ScanLabels::Scans); - let config = self.paths.borrow(); + + let scan_count = self.new_scan(); + + for pattern in self.paths.borrow().iter() { + let path = host_info::prepend_host_mount(pattern); + self.scan_inner(&path, scan_count)?; + } // Cleanup any items that are either: // * Not configured to be monitored anymore. // * Are configured to be monitored but no longer are found in // the file system. - self.inode_map.borrow_mut().retain(|inode, path| { - if config.iter().any(|prefix| path.starts_with(prefix)) - && host_info::prepend_host_mount(path).exists() - { - true - } else { - let _ = self.kernel_inode_map.borrow_mut().remove(inode); - self.metrics.scan_inc(ScanLabels::InodeRemoved); - false - } - }); - - for pattern in self.paths.borrow().iter() { - let path = host_info::prepend_host_mount(pattern); - self.scan_inner(&path)?; + // + // The scan happens inline with processing of events, so at this + // point all elements in the map that we saw during the scan + // should have the corresponding scan count set to the same + // value as the one we have now. Everything that has a different + // value can be assumed to have been removed or no longer + // monitored. + for (inode, _) in self + .inode_map + .borrow_mut() + .extract_if(|_, (_, c)| *c != scan_count) + { + // TODO: Turn this into a bulk operation. https://github.com/stackrox/fact/issues/1133 + let _ = self.kernel_inode_map.borrow_mut().remove(&inode); + self.metrics.scan_inc(ScanLabels::InodeRemoved); } + let duration = start.elapsed(); self.metrics.scan_duration.observe(duration.as_secs_f64()); info!( @@ -149,7 +166,7 @@ impl HostScanner { Ok(()) } - fn scan_inner(&self, path: &Path) -> anyhow::Result<()> { + fn scan_inner(&self, path: &Path, scan_count: u8) -> anyhow::Result<()> { self.metrics.scan_inc(ScanLabels::ElementsScanned); let Some(glob_str) = path.to_str() else { @@ -176,11 +193,11 @@ impl HostScanner { if metadata.is_file() { self.metrics.scan_inc(ScanLabels::FileScanned); - self.update_entry(path.as_path(), &metadata) + self.update_entry(path.as_path(), &metadata, scan_count) .with_context(|| format!("Failed to update entry for {}", path.display()))?; } else if metadata.is_dir() { self.metrics.scan_inc(ScanLabels::DirectoryScanned); - self.update_entry(path.as_path(), &metadata) + self.update_entry(path.as_path(), &metadata, scan_count) .with_context(|| format!("Failed to update entry for {}", path.display()))?; } else { self.metrics.scan_inc(ScanLabels::FsItemIgnored); @@ -189,34 +206,40 @@ impl HostScanner { Ok(()) } - fn update_entry(&self, path: &Path, metadata: &Metadata) -> anyhow::Result<()> { + fn update_entry(&self, path: &Path, metadata: &Metadata, scan_count: u8) -> anyhow::Result<()> { let inode = inode_key_t { inode: metadata.st_ino(), dev: metadata.st_dev(), }; let host_path = host_info::remove_host_mount(path); - self.update_entry_with_inode(inode, host_path)?; + self.update_entry_with_inode(inode, host_path, scan_count)?; debug!("Added entry for {}: {inode:?}", path.display()); Ok(()) } /// Similar to update_entry except we are are directly using the inode instead of the path. - fn update_entry_with_inode(&self, inode: inode_key_t, path: PathBuf) -> anyhow::Result<()> { + fn update_entry_with_inode( + &self, + inode: inode_key_t, + path: PathBuf, + scan_count: u8, + ) -> anyhow::Result<()> { let mut inode_map = self.inode_map.borrow_mut(); match inode_map.get_mut(&inode) { - Some(p) => { + Some((p, c)) => { // inode is already tracked. if path != *p { *p = path; self.metrics.scan_inc(ScanLabels::FileUpdated); } + *c = scan_count; return Ok(()); } None => { self.metrics.scan_inc(ScanLabels::FileUpdated); - inode_map.insert(inode, path.clone()); + inode_map.insert(inode, (path.clone(), scan_count)); } }; @@ -240,7 +263,7 @@ You can increase this limit with: fn get_host_path(&self, inode: Option<&inode_key_t>) -> Option { // The path here needs to be cloned because we won't keep the // inode_map borrow long enough. - self.inode_map.borrow().get(inode?).cloned() + self.inode_map.borrow().get(inode?).cloned().map(|(p, _)| p) } /// Handle file creation events by adding new inodes to the map. @@ -259,7 +282,7 @@ You can increase this limit with: && let Some(parent_host_path) = self.get_host_path(Some(parent_inode)) { let host_path = parent_host_path.join(filename); - self.update_entry_with_inode(*inode, host_path) + self.update_entry_with_inode(*inode, host_path, *self.scan_count.borrow()) .with_context(|| { format!( "Failed to add creation event entry for {}", @@ -310,7 +333,7 @@ You can increase this limit with: warn!("Rename event did not have old host path for inode tracked item"); return; }; - self.inode_map.borrow_mut().retain(|inode, path| { + self.inode_map.borrow_mut().retain(|inode, (path, _)| { if !path.starts_with(old_host_path) { return true; } @@ -339,7 +362,7 @@ You can increase this limit with: // path that didn't hold anything, we need to figure out the // host path and check if we should track it. let mut inode_map = self.inode_map.borrow_mut(); - let Some(new_host_parent) = inode_map.get(event.get_parent_inode()) else { + let Some((new_host_parent, _)) = inode_map.get(event.get_parent_inode()) else { warn!("Failed to get parent host path"); return; }; @@ -355,7 +378,7 @@ You can increase this limit with: if self.paths_globset.is_match(&new_host_path) { // New path needs to be tracked. // Move all entries for the old host path to the new one - for path in inode_map.values_mut() { + for (path, _) in inode_map.values_mut() { if let Ok(suffix) = path.strip_prefix(old_host_path) { if suffix == Path::new("") { *path = new_host_path.clone(); @@ -369,7 +392,7 @@ You can increase this limit with: event.set_host_path(new_host_path); } else { // New path is not tracked, remove old entries - inode_map.retain(|inode, path| { + inode_map.retain(|inode, (path, _)| { if !path.starts_with(old_host_path) { return true; } @@ -391,7 +414,7 @@ You can increase this limit with: // Attempt to update the host path with the old inode if let Some(old_inode) = event.get_old_inode() - && let Some(path) = self.inode_map.borrow().get(old_inode) + && let Some((path, _)) = self.inode_map.borrow().get(old_inode) { event.set_host_path(path.clone()); }