ROX-33217: Instrument inode tracking on directory being created path mkdir#465
Conversation
fact-ebpf/src/bpf/main.c
Outdated
| umode_t mode = BPF_CORE_READ(inode, i_mode); | ||
| if (!S_ISDIR(mode)) { | ||
| bpf_map_delete_elem(&mkdir_context, &pid_tgid); | ||
| m->d_instantiate.ignored++; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
We should check if m->d_instantiate.ignored actually increases, I have a feeling this condition should never be met.
There was a problem hiding this comment.
m->d_instantiate.ignored increases, but I don't know if it is due to this or other locations with m->d_instantiate.ignored++;.
4a6a257 to
b0ef4fe
Compare
|
/retest |
Molter73
left a comment
There was a problem hiding this comment.
Just a couple small comments, we should be good to merge soon.
fact-ebpf/src/bpf/main.c
Outdated
| if (inode == NULL) { | ||
| m->d_instantiate.ignored++; | ||
| goto cleanup; | ||
| } |
There was a problem hiding this comment.
Probably should move this check a bit higher, there is no point in getting the context if there is no inode to work upon. You can still jump to cleanup, bpf_map_delete_elem will fail if there is no context to remove from the map, but we are already silently ignoring it.
fact/src/event/mod.rs
Outdated
| process: Process, | ||
| file: FileData, | ||
| #[serde(skip)] | ||
| event_type: file_activity_type_t, |
There was a problem hiding this comment.
What is the point of this field? FileData already encodes this same information in its type, doesn't it?
There was a problem hiding this comment.
Remove. I also went with your suggestion below.
fact/src/host_scanner.rs
Outdated
| self.metrics.events.dropped(); | ||
| warn!("Failed to send event: {e}"); | ||
| // Skip directory creation events - we track them internally but don't send to sensor | ||
| if event.event_type() != fact_ebpf::file_activity_type_t::DIR_ACTIVITY_CREATION { |
There was a problem hiding this comment.
Ok, I see now why you added event_type, don't do this, add a MkDir variant to FileData and let the information be encoded in the enum. You will need to add a is_mkdir() method or something of the sort, but that's is more idiomatic anyways.
Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
3fd3840 to
0874869
Compare
Molter73
left a comment
There was a problem hiding this comment.
LGTM! Just a few small comments I would like to see addressed, but they shouldn't block merging. Thanks for the effort on this one!
Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>
Description
Directory creation events need to be handled correctly. When a directory is created in a tracked directory its inode should be added to a hash set in kernel space. In user space an entry needs to be added into a map with the inode as the key and file path as the value.
An alternative approach can be found at #449
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Checked metrics