ROX-33036: add mount related operations - #1059
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mauro/feat/inodes-introspection #1059 +/- ##
===================================================================
- Coverage 35.42% 35.04% -0.38%
===================================================================
Files 22 22
Lines 3241 3276 +35
Branches 3241 3276 +35
===================================================================
Hits 1148 1148
- Misses 2088 2123 +35
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3cc1980 to
b15f103
Compare
b15f103 to
0eb199b
Compare
This was originally going to be about adding just `lsm/sb_mount`, however while adding this hook it became pretty clear we needed `lsm/sb_umount` and `lsm/move_mount` for a comprehensive implementation and it really didn't add too much code, so they are all added in. These operations are not currently intended to be forwarded via gRPC, they only trigger inode tracking related behavior (scans on new/moved mounts, inode map cleanups on moved/unmounted directories). The move mount operation shares quite a bit of similarities with rename, so there is a bit of refactoring mixed in so they can be reused while keeping the code clean. TODO: add integration tests.
Add a basic test for checking mount operations are properly tracked. This test works by checking the monitored directory is tracked using the inodes introspection endpoint, then a tmpfs is mounted on top of this directory and we validate we see the new inode, then we unmount and check the introspection endpoint one last time.
0eb199b to
5e41f99
Compare
|
/retest |
a0d0d78 to
edc7efa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
657-687: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
PartialEq for FileDatamissing arms forMount,Umount,MoveMount.Every other variant (including all pre-existing ones) has an explicit equality arm, but the three new mount variants fall through to
_ => false. This means two identicalMount/Umount/MoveMountevents will never compare equal.🐛 Proposed fix
(FileData::AclSet(this), FileData::AclSet(other)) => { this.inner == other.inner && this.acl_type == other.acl_type && this.entries == other.entries } + (FileData::Mount(this), FileData::Mount(other)) => this == other, + (FileData::Umount(this), FileData::Umount(other)) => this == other, + ( + FileData::MoveMount { to: l_to, from: l_from }, + FileData::MoveMount { to: r_to, from: r_from }, + ) => l_to == r_to && l_from == r_from, _ => false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/event/mod.rs` around lines 657 - 687, Add explicit equality arms to PartialEq::eq for FileData covering Mount, Umount, and MoveMount, comparing each variant’s corresponding fields consistently with the other variant arms. Keep the fallback _ => false for differing variants.
🧹 Nitpick comments (3)
fact/src/host_scanner.rs (1)
429-439: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMount events trigger a synchronous full scan per event, with no coalescing for bursts.
handle_mount_eventcallsself.scan()directly and inline in theselect!branch, blocking the task for the scan's duration and running once per mount-related event with no debouncing. The existingscan_trigger/Notifymechanism used for periodic andpaths.changed()scans already coalesces repeated triggers (multiplenotify_one()calls before consumption collapse to a single pending scan), but that path isn't reused here. Under a burst of mount/unmount activity this could serialize many redundant full scans and delay processing of other events in the loop (e.g. introspection queries).Also applies to: 493-497
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/host_scanner.rs` around lines 429 - 439, Update handle_mount_event to signal the existing scan_trigger/Notify mechanism instead of calling self.scan() synchronously. Route mount events through the same coalesced scan path used by periodic and paths.changed() triggers, preserving a single pending scan during event bursts and keeping the select loop responsive.fact/src/event/mod.rs (1)
437-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding Rust-side test coverage for the new
Mount/Umount/MoveMountvariants.Codecov flags 130 uncovered lines in this file for this PR, and the
PartialEqgap above went unnoticed likely due to this.EventTestData(used by the#[cfg(all(test, feature = "bpf-test"))]constructor) doesn't have variants for the new mount events either.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/event/mod.rs` around lines 437 - 535, Add Rust-side tests covering FileData::Mount, FileData::Umount, and FileData::MoveMount, including their constructed fields and equality behavior. Extend EventTestData and its bpf-test constructor with corresponding mount-event variants so these cases can be exercised through the existing test path, and ensure the tests cover both source and destination data for MoveMount.tests/test_mount.py (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubprocess calls don't check exit status.
mount/umountfailures (e.g. wrong tmpfs support, misconfigured environment) will pass silently, and the test would then just fail (or worse, pass) based on stale inode state rather than surfacing the real cause.♻️ Proposed fix
- subprocess.run( - ['mount', '-t', 'tmpfs', '-o', 'size=10M', 'tmpfs', monitored_dir] - ) + subprocess.run( + ['mount', '-t', 'tmpfs', '-o', 'size=10M', 'tmpfs', monitored_dir], + check=True, + ) assert_tracked_path(monitored_dir) - subprocess.run(['umount', monitored_dir]) + subprocess.run(['umount', monitored_dir], check=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_mount.py` around lines 44 - 50, Update the subprocess.run calls in the mount/umount test to enforce successful exit status, so mount or unmount failures raise immediately instead of allowing assertions to use stale inode state. Preserve the existing command arguments and tracking assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 9: Update the ROX-33036 changelog entry to hyphenate “mount-related” in
the description of the operations.
In `@fact-ebpf/src/bpf/main.c`:
- Around line 557-603: Update trace_move_mount so events are emitted for every
monitored classification except MONITORED_NOT, matching the sb_mount/sb_umount
handling and allowing parent/path-monitored moves to reach rescan. In the same
function, derive args.parent_inode from to’s actual parent inode rather than
reusing to->dentry->d_inode, and pass that value to is_monitored.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 657-687: Add explicit equality arms to PartialEq::eq for FileData
covering Mount, Umount, and MoveMount, comparing each variant’s corresponding
fields consistently with the other variant arms. Keep the fallback _ => false
for differing variants.
---
Nitpick comments:
In `@fact/src/event/mod.rs`:
- Around line 437-535: Add Rust-side tests covering FileData::Mount,
FileData::Umount, and FileData::MoveMount, including their constructed fields
and equality behavior. Extend EventTestData and its bpf-test constructor with
corresponding mount-event variants so these cases can be exercised through the
existing test path, and ensure the tests cover both source and destination data
for MoveMount.
In `@fact/src/host_scanner.rs`:
- Around line 429-439: Update handle_mount_event to signal the existing
scan_trigger/Notify mechanism instead of calling self.scan() synchronously.
Route mount events through the same coalesced scan path used by periodic and
paths.changed() triggers, preserving a single pending scan during event bursts
and keeping the select loop responsive.
In `@tests/test_mount.py`:
- Around line 44-50: Update the subprocess.run calls in the mount/umount test to
enforce successful exit status, so mount or unmount failures raise immediately
instead of allowing assertions to use stale inode state. Preserve the existing
command arguments and tracking assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 05fc42f0-315c-46df-9f9b-5b00f13b4fd4
📒 Files selected for processing (12)
CHANGELOG.mdfact-ebpf/src/bpf/bound_path.hfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/host_scanner.rsfact/src/metrics/kernel_metrics.rstests/conftest.pytests/test_config_hotreload.pytests/test_mount.py
|
|
||
| ## Next | ||
|
|
||
| * ROX-33036: add mount related operations (#1059) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor grammar: hyphenate "mount-related".
✏️ Proposed fix
-* ROX-33036: add mount related operations (`#1059`)
+* ROX-33036: add mount-related operations (`#1059`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * ROX-33036: add mount related operations (#1059) | |
| * ROX-33036: add mount-related operations (`#1059`) |
🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...acking. ## Next * ROX-33036: add mount related operations (#1059) * feat(endpoi...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 9, Update the ROX-33036 changelog entry to hyphenate
“mount-related” in the description of the operations.
Source: Linters/SAST tools
| SEC("lsm/move_mount") | ||
| int BPF_PROG(trace_move_mount, struct path* from, struct path* to) { | ||
| struct metrics_t* m = get_metrics(); | ||
| if (m == NULL) { | ||
| return 0; | ||
| } | ||
| struct submit_event_args_t args = {.metrics = &m->move_mount}; | ||
|
|
||
| args.metrics->total++; | ||
|
|
||
| struct bound_path_t* to_path = path_read_unchecked(to, false); | ||
| if (to_path == NULL) { | ||
| bpf_printk("Failed to read to_path"); | ||
| goto error; | ||
| } | ||
| args.filename = to_path->path; | ||
|
|
||
| struct bound_path_t* from_path = path_read_alt_unchecked(from, false); | ||
| if (from_path == NULL) { | ||
| bpf_printk("Failed to read from_path"); | ||
| goto error; | ||
| } | ||
|
|
||
| args.inode = inode_to_key(to->dentry->d_inode); | ||
| args.parent_inode = inode_to_key(to->dentry->d_inode); | ||
| args.monitored = is_monitored(&args.inode, to_path, &args.parent_inode); | ||
|
|
||
| inode_key_t from_inode = inode_to_key(from->dentry->d_inode); | ||
| monitored_t from_monitored = is_monitored(&from_inode, from_path, NULL); | ||
|
|
||
| if (args.monitored != MONITORED_BY_INODE) { | ||
| args.metrics->ignored++; | ||
| return 0; | ||
| } | ||
|
|
||
| // Ensure the new mount is tracked. | ||
| if (from_monitored != MONITORED_BY_INODE) { | ||
| inode_add(&from_inode); | ||
| } | ||
|
|
||
| submit_move_mount_event(&args, from_path->path, &from_inode, from_monitored); | ||
| return 0; | ||
|
|
||
| error: | ||
| args.metrics->error++; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant hooks and inspect the surrounding code.
git ls-files fact-ebpf/src/bpf/main.c
ast-grep outline fact-ebpf/src/bpf/main.c --view expanded
printf '\n--- trace_sb_mount / trace_sb_umount / trace_move_mount ---\n'
sed -n '470,620p' fact-ebpf/src/bpf/main.cRepository: stackrox/fact
Length of output: 5221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the monitoring helper and the enum values it returns.
rg -n "is_monitored|monitored_t|MONITORED_BY_PARENT|MONITORED_BY_INODE|NOT_MONITORED" fact-ebpf/src/bpf/main.c fact-ebpf/src/bpf -g '!**/target/**'
printf '\n--- is_monitored context ---\n'
# Read the helper definition once located.
helper_file=$(rg -l "is_monitored" fact-ebpf/src/bpf | head -n 1)
if [ -n "${helper_file:-}" ]; then
sed -n '1,260p' "$helper_file"
fiRepository: stackrox/fact
Length of output: 9290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- inode_is_monitored ---'
sed -n '100,140p' fact-ebpf/src/bpf/inode.h
printf '\n%s\n' '--- submit_move_mount_event ---'
sed -n '210,280p' fact-ebpf/src/bpf/events.hRepository: stackrox/fact
Length of output: 2181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any move-mount specific handling and nearby monitoring rules.
rg -n "FILE_ACTIVITY_MOVE_MOUNT|move_mount|MONITORED_BY_PARENT|MONITORED_BY_PATH|ignored\+\+" fact-ebpf -g '!**/target/**'
printf '\n--- event type definitions ---\n'
sed -n '1,260p' fact-ebpf/src/bpf/types.h
printf '\n--- move-mount event payload and consumers ---\n'
sed -n '1,320p' fact-ebpf/src/bpf/events.hRepository: stackrox/fact
Length of output: 17119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# See whether parent_inode is consumed anywhere outside event serialization.
rg -n "\.parent_inode\b|parent_inode" fact-ebpf fact -g '!**/target/**' -g '!**/node_modules/**'Repository: stackrox/fact
Length of output: 4186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- host_scanner.rs move_mount handling ---'
sed -n '250,390p' fact/src/host_scanner.rs
printf '\n%s\n' '--- event/mod.rs relevant accessors ---'
sed -n '150,240p' fact/src/event/mod.rsRepository: stackrox/fact
Length of output: 10010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- host_scanner.rs move_mount handling later in file ---'
sed -n '390,520p' fact/src/host_scanner.rs
printf '\n%s\n' '--- FileData::MoveMount definitions ---'
sed -n '430,760p' fact/src/event/mod.rsRepository: stackrox/fact
Length of output: 18576
trace_move_mount is too strict about monitored state: it only emits when args.monitored == MONITORED_BY_INODE, so move_mounts under monitored parents/paths are ignored and never reach the rescan path. Match the sb_mount/sb_umount NOT_MONITORED check here; if parent-based classification is intended, also read to’s real parent inode instead of reusing to->dentry->d_inode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact-ebpf/src/bpf/main.c` around lines 557 - 603, Update trace_move_mount so
events are emitted for every monitored classification except MONITORED_NOT,
matching the sb_mount/sb_umount handling and allowing parent/path-monitored
moves to reach rescan. In the same function, derive args.parent_inode from to’s
actual parent inode rather than reusing to->dentry->d_inode, and pass that value
to is_monitored.
Source: Coding guidelines
2 participants
Description
This was originally going to be about adding just
lsm/sb_mount, however while adding this hook it became pretty clear we neededlsm/sb_umountandlsm/move_mountfor a comprehensive implementation and it really didn't add too much code, so they are all added in.These operations are not currently intended to be forwarded via gRPC, they only trigger inode tracking related behavior (scans on new/moved mounts, inode map cleanups on moved/unmounted directories).
The move mount operation shares quite a bit of similarities with rename, so there is a bit of refactoring mixed in so they can be reused while keeping the code clean.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Added integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation