ROX-33198: Instrument inode tracking on file open lsm hook #391
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,11 @@ impl HostScanner { | |
| self.update_entry(path.as_path()).with_context(|| { | ||
| format!("Failed to update entry for {}", path.display()) | ||
| })?; | ||
| } else if path.is_dir() { | ||
|
Contributor
There was a problem hiding this comment. This is slightly related to a conversation I've been having with @Stringy. IMO, in the case of finding a directory that matches a glob pattern, we should recursively scan into it, the reason being that tracking the directory itself isn't of much value if the pattern doesn't hit any files inside the directory. For now we can leave it like this, but I think we need to take a close look at what users might expect when configuring different types of glob patterns (i.e do they want that strict set of patterns to be monitored, or are they expressing a set of paths that work as the base to be monitored?) |
||
| self.metrics.scan_inc(ScanLabels::DirectoryScanned); | ||
| self.update_entry(path.as_path()).with_context(|| { | ||
| format!("Failed to update entry for {}", path.display()) | ||
| })?; | ||
| } else { | ||
| self.metrics.scan_inc(ScanLabels::FsItemIgnored); | ||
| } | ||
|
|
@@ -154,17 +159,26 @@ impl HostScanner { | |
| dev: metadata.st_dev(), | ||
| }; | ||
|
|
||
| let host_path = host_info::remove_host_mount(path); | ||
| self.update_entry_with_inode(&inode, host_path)?; | ||
|
|
||
| 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<()> { | ||
| self.kernel_inode_map | ||
| .borrow_mut() | ||
| .insert(*inode, 0, 0) | ||
| .with_context(|| format!("Failed to insert kernel entry for {}", path.display()))?; | ||
|
|
||
| let mut inode_map = self.inode_map.borrow_mut(); | ||
| let entry = inode_map.entry(*inode).or_default(); | ||
| *entry = path; | ||
|
|
||
| self.metrics.scan_inc(ScanLabels::FileUpdated); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -178,6 +192,34 @@ impl HostScanner { | |
| self.inode_map.borrow().get(inode?).cloned() | ||
| } | ||
|
|
||
| /// Handle file creation events by adding new inodes to the map. | ||
| /// | ||
| /// We use the parent inode provided by the eBPF code | ||
| /// to look up the parent directory's host path, then construct the full | ||
| /// path by appending the new file's name. | ||
| fn handle_creation_event(&self, event: &Event) -> anyhow::Result<()> { | ||
|
Contributor
There was a problem hiding this comment. I'm hoping this is still WIP and the debug statements are for testing, we really don't need a debug statement on each return condition, particularly not in the hot path where monitoring sets of files mildly noisy will just flood the logs and make it hard to see what is actually going on. Please cut down on the debug statements, if something is expected to happen, don't put a statement there.
Contributor
There was a problem hiding this comment. I tried removing all the debug statements in this method and managed to boil it down from 50+ lines to just this: let inode = event.get_inode();
let parent_inode = event.get_parent_inode();
if self.get_host_path(Some(inode)).is_some() || parent_inode.empty() {
return Ok(());
}
if let Some(filename) = event.get_filename().file_name()
&& 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)
.with_context(|| {
format!(
"Failed to add creation event entry for {}",
filename.display()
)
})?;
}
Ok(())Given this is a lot more compact and easy to ready and the additional verbosity in the logs the implementation in the PR doesn't bring much of a benefit IMO, I would propose we try to move the implementation closer to this. |
||
| let inode = event.get_inode(); | ||
| let parent_inode = event.get_parent_inode(); | ||
| if self.get_host_path(Some(inode)).is_some() || parent_inode.empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(filename) = event.get_filename().file_name() { | ||
| if 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) | ||
| .with_context(|| { | ||
| format!( | ||
| "Failed to add creation event entry for {}", | ||
| filename.display() | ||
| ) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Periodically notify the host scanner main task that a scan needs | ||
| /// to happen. | ||
| /// | ||
|
|
@@ -219,6 +261,13 @@ impl HostScanner { | |
| }; | ||
| self.metrics.events.added(); | ||
|
|
||
| // Handle file creation events by adding new inodes to the map | ||
| if event.is_creation() { | ||
| if let Err(e) = self.handle_creation_event(&event) { | ||
| warn!("Failed to handle creation event: {e}"); | ||
| } | ||
| } | ||
|
|
||
| if let Some(host_path) = self.get_host_path(Some(event.get_inode())) { | ||
| self.metrics.scan_inc(ScanLabels::InodeHit); | ||
| event.set_host_path(host_path); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent inode was added to the event so that it could be used to find the parent path. It is added for all event types, not just when a file is opened. Perhaps NULL could be passed instead of the actual parent inode for the other type of events for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, passing
NULLshould be enough for nowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done