ROX-34920: track symlink events - #1440
Conversation
📝 WalkthroughWalkthroughThis change adds end-to-end symlink event support. eBPF hooks capture symlink creation and targets. Rust event types and serialization expose symlink data. Host scanning resolves targets and handles symlink events. Integration tests cover creation, overwrites, ignored paths, and filesystem variants. ChangesSymlink event support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Filesystem
participant path_symlink
participant trace_d_instantiate
participant EventModel
participant HostScanner
Filesystem->>path_symlink: create symlink
path_symlink->>trace_d_instantiate: capture path and target context
trace_d_instantiate->>EventModel: submit symlink event
EventModel->>HostScanner: deliver parsed symlink event
HostScanner->>Filesystem: scan symlink target
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1440 +/- ##
==========================================
- Coverage 34.78% 34.18% -0.61%
==========================================
Files 22 22
Lines 3300 3358 +58
Branches 3300 3358 +58
==========================================
Hits 1148 1148
- Misses 2147 2205 +58
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/test_path_symlink.py (2)
475-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ignored_diris unused intest_mounted_dir.The test requests the
ignored_dirfixture but never references it. If the fixture is required to set up the mounted directory, add a comment. Otherwise remove the parameter.🤖 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_path_symlink.py` around lines 475 - 479, Update test_mounted_dir to remove the unused ignored_dir fixture parameter unless it is required for setup; if it must remain, add a comment explaining how the fixture establishes the mounted directory.
485-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exit codes of the container commands.
test_ovfschecksres.exit_code == 0for both commands.test_mounted_dirignores the results. Iftouchorlnfails inside the container, the test fails later with a timeout inwait_eventsinstead of a clear error.💚 Proposed fix
- test_container.exec_run(f'touch {file}') - test_container.exec_run(f'ln -s {file} {link}') + res = test_container.exec_run(f'touch {file}') + assert res.exit_code == 0 + res = test_container.exec_run(f'ln -s {file} {link}') + assert res.exit_code == 0🤖 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_path_symlink.py` around lines 485 - 486, Update test_mounted_dir’s container setup around the touch and ln -s exec_run calls to capture each command result and assert exit_code == 0, matching test_ovfs. Keep the existing command order and make failures surface immediately before wait_events runs.fact-ebpf/src/bpf/bound_path.h (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated failure log.
path_read_into_append_d_entrylogs "Failed to read path". The callers infact-ebpf/src/bpf/main.cat lines 309-311 and 623-625 log the same message for the same failure. Each failure then produces two identical trace lines. Keep the log in one place.🤖 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/bound_path.h` around lines 72 - 75, Remove the bpf_printk failure log from the path_read_into call in path_read_into_append_d_entry, while retaining its NULL return behavior; keep the existing caller-side logs in main.c as the single failure log location.fact-ebpf/src/bpf/maps.h (1)
112-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
get_or_insert_d_instantiate_ctxreuses stale field values.If an entry already exists, the function resets only
event_type. The fieldspath,parent_inode,monitored, andsymlink_targetkeep the values of the previous operation for the same thread.trace_path_symlinknever assignsmonitored, so the entry carries a stale value intotrace_d_instantiate. The symlink branch recomputesmonitoredwithis_monitored, so the current code is correct. A future hook that readsctx->monitoredbefore assignment would read stale data.Set the remaining scalar fields to defaults on reuse.
🛡️ Proposed defensive reset
struct d_instantiate_ctx_t* ctx = bpf_map_lookup_elem(&d_instantiate_ctx, &pid); if (ctx != NULL) { // Clear the event type so `d_instantiate` doesn't trigger by accident ctx->event_type = FILE_ACTIVITY_INIT; + ctx->monitored = NOT_MONITORED; + ctx->path.len = 0; + ctx->symlink_target[0] = '\0'; return ctx; }🤖 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/maps.h` around lines 112 - 134, Update get_or_insert_d_instantiate_ctx so an existing context is fully reset before reuse, not just event_type. Restore path, parent_inode, monitored, and symlink_target to their default values while preserving the existing return behavior and initialization defaults.fact-ebpf/src/bpf/main.c (1)
391-392: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
delete_d_instantiate_ctx()for the cleanup.Line 392 calls
bpf_map_delete_elemdirectly with the locally capturedpid_tgid. The new helperdelete_d_instantiate_ctx()performs the same operation. Use the helper so all access tod_instantiate_ctxgoes through one place. The localpid_tgidat line 343 then becomes unnecessary.🤖 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 391 - 392, Update the cleanup path to call delete_d_instantiate_ctx() instead of directly invoking bpf_map_delete_elem on d_instantiate_ctx. Remove the now-unused local pid_tgid declaration and retain the existing cleanup behavior.
🤖 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 `@fact-ebpf/src/bpf/bound_path.h`:
- Around line 57-63: Update the d_path handling in the bound path construction
flow to capture its signed result in a signed local variable before assigning
bound_path->len. Check that local result for len <= 0, return NULL on failure,
then assign the validated value to bound_path->len before applying
PATH_LEN_CLAMP.
In `@fact-ebpf/src/bpf/events.h`:
- Around line 249-258: Update submit_symlink_event to check the return value of
bpf_probe_read_str; on failure, either initialize args->event->from.filename[0]
to '\0' before __submit_event or release/discard the reserved event instead of
submitting uninitialized symlink-target data.
In `@fact-ebpf/src/bpf/main.c`:
- Line 628: Update the parent inode assignment in the relevant hook to obtain
the inode through a CO-RE read of the dentry chain, matching the existing
trace_path_mkdir pattern with BPF_CORE_READ(dir, dentry, d_inode), then pass
that result to inode_to_key.
In `@fact-ebpf/src/bpf/maps.h`:
- Around line 86-100: Reduce the `d_instantiate_ctx` map’s `max_entries` from
16384 to a value bounded by the maximum number of concurrent pid_tgid contexts,
or redesign `d_instantiate_ctx_t` storage to avoid embedding both path buffers
per entry. Preserve context correlation while ensuring the preallocated LRU hash
value memory fits the intended resource budget.
In `@tests/test_path_symlink.py`:
- Line 89: Update the pytest.param case with id='Invalid' in the symlink path
tests so the symlink argument is a bytes literal containing the undecodable
filename bytes, matching the b'test\xff\xfe.txt' input and exercising the
intended invalid filename behavior.
---
Nitpick comments:
In `@fact-ebpf/src/bpf/bound_path.h`:
- Around line 72-75: Remove the bpf_printk failure log from the path_read_into
call in path_read_into_append_d_entry, while retaining its NULL return behavior;
keep the existing caller-side logs in main.c as the single failure log location.
In `@fact-ebpf/src/bpf/main.c`:
- Around line 391-392: Update the cleanup path to call
delete_d_instantiate_ctx() instead of directly invoking bpf_map_delete_elem on
d_instantiate_ctx. Remove the now-unused local pid_tgid declaration and retain
the existing cleanup behavior.
In `@fact-ebpf/src/bpf/maps.h`:
- Around line 112-134: Update get_or_insert_d_instantiate_ctx so an existing
context is fully reset before reuse, not just event_type. Restore path,
parent_inode, monitored, and symlink_target to their default values while
preserving the existing return behavior and initialization defaults.
In `@tests/test_path_symlink.py`:
- Around line 475-479: Update test_mounted_dir to remove the unused ignored_dir
fixture parameter unless it is required for setup; if it must remain, add a
comment explaining how the fixture establishes the mounted directory.
- Around line 485-486: Update test_mounted_dir’s container setup around the
touch and ln -s exec_run calls to capture each command result and assert
exit_code == 0, matching test_ovfs. Keep the existing command order and make
failures surface immediately before wait_events runs.
🪄 Autofix
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: 19c859c2-bd26-4af1-a990-a77bfc286ed2
📒 Files selected for processing (14)
fact-ebpf/src/bpf/bound_path.hfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/file.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/maps.hfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/host_scanner.rsfact/src/metrics/host_scanner.rsfact/src/metrics/kernel_metrics.rstests/event.pytests/server.pytests/test_path_symlink.py
💤 Files with no reviewable changes (1)
- fact-ebpf/src/bpf/file.h
| // Context for correlating operations in d_instantiate | ||
| struct d_instantiate_ctx_t { | ||
| struct bound_path_t path; | ||
| inode_key_t parent_inode; | ||
| monitored_t monitored; | ||
| file_activity_type_t event_type; | ||
| char symlink_target[PATH_MAX]; | ||
| }; | ||
|
|
||
| struct { | ||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); | ||
| __type(key, __u64); | ||
| __type(value, struct mkdir_context_t); | ||
| __type(value, struct d_instantiate_ctx_t); | ||
| __uint(max_entries, 16384); | ||
| } mkdir_context SEC(".maps"); | ||
| } d_instantiate_ctx SEC(".maps"); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare entry counts and value sizes of the BPF maps in this repo.
fd -e h -e c . fact-ebpf/src/bpf --exec rg -n -C6 'BPF_MAP_TYPE_[A-Z_]+|max_entries' {}Repository: stackrox/fact
Length of output: 10918
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- maps.h ---'
sed -n '1,155p' fact-ebpf/src/bpf/maps.h
printf '%s\n' '--- relevant type definitions ---'
rg -n -C4 'typedef .*inode_key_t|typedef .*monitored_t|file_activity_type_t|struct bound_path_t|struct d_instantiate_ctx_t' fact-ebpf/src/bpf
printf '%s\n' '--- d_instantiate_ctx accesses ---'
rg -n -C8 'd_instantiate_ctx|get_d_instantiate_ctx|symlink_target' fact-ebpf/src/bpfRepository: stackrox/fact
Length of output: 39288
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import ctypes
PATH_MAX = 4096
class BoundPath(ctypes.Structure):
_fields_ = [('len', ctypes.c_uint), ('path', ctypes.c_char * (PATH_MAX * 2))]
# These scalar fields are resolved from the source output in the first probe.
print('BoundPath size:', ctypes.sizeof(BoundPath))
PYRepository: stackrox/fact
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- prior context-map references ---'
rg -n -C5 'mkdir_context|d_instantiate_ctx' .
printf '%s\n' '--- map declarations in project BPF sources ---'
rg -n -C3 '__uint\(type, BPF_MAP_TYPE_(LRU_HASH|HASH)\)|__uint\(max_entries' fact-ebpf/src/bpf -g '*.{h,c}'
printf '%s\n' '--- target and compiler metadata ---'
rg -n -C2 'TARGET_ARCH|__TARGET_ARCH|aarch64|x86_64|clang|bpf' fact-ebpf/Makefile fact-ebpf -g 'Makefile' -g '*.mk' -g '*.sh' -g '*.yml' -g '*.yaml' 2>/dev/null || trueRepository: stackrox/fact
Length of output: 10785
🌐 Web query:
Linux kernel BPF_MAP_TYPE_LRU_HASH preallocation BPF_F_NO_PREALLOC support value_size max_entries memory allocation
💡 Result:
The Linux kernel does not support the BPF_F_NO_PREALLOC flag for BPF_MAP_TYPE_LRU_HASH and its variants [1]. While standard hash maps (BPF_MAP_TYPE_HASH) support BPF_F_NO_PREALLOC to disable pre-allocation [2][3][4], LRU hash maps require pre-allocation of nodes [5][6][1]. Key technical details regarding memory allocation for BPF_MAP_TYPE_LRU_HASH include: Pre-allocation Requirement: All nodes for an LRU hash map are pre-allocated at map creation time and placed into a "free" list [5][6][1]. Because this mechanism is fundamental to the LRU eviction logic, opting out of pre-allocation is not supported [1]. Memory Limits: The size of keys and values is restricted such that their combined size, along with implementation overhead, must not exceed KMALLOC_MAX_SIZE [5][6]. If this limit is exceeded during map creation, the syscall will return an -E2BIG error [5][6]. Allocation Behavior: The map maintains internal lists (typically active, inactive, and free lists) to manage elements [5][6]. In the default configuration, these lists are global, but the BPF_F_NO_COMMON_LRU flag can be used to request per-CPU LRU lists for potentially improved performance in specific multi-CPU scenarios [2][5][1]. The maximum number of entries (max_entries) specified at creation determines the size of the pre-allocated pool [2][5]. In summary, users cannot use BPF_F_NO_PREALLOC with LRU hash maps, and the memory footprint is determined at creation based on max_entries, key_size, and value_size [5][6][1].
Citations:
- 1: https://lists.openwall.net/linux-kernel/2022/11/03/1530
- 2: https://kernel.org/doc/html/latest/bpf/map_hash.html
- 3: https://docs.kernel.org/bpf/map_hash.html
- 4: https://docs.kernel.org/6.7/bpf/map_hash.html
- 5: https://docs.ebpf.io/linux/map-type/BPF_MAP_TYPE_LRU_HASH/
- 6: https://github.com/isovalent/ebpf-docs/blob/master/docs/linux/map-type/BPF_MAP_TYPE_LRU_HASH.md
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import ctypes
from pathlib import Path
PATH_MAX = 4096
max_entries = 16384
class BoundPath(ctypes.Structure):
_fields_ = [
("len", ctypes.c_uint),
("path", ctypes.c_char * (PATH_MAX * 2)),
]
class InodeKey(ctypes.Structure):
_fields_ = [
("inode", ctypes.c_ulong),
("dev", ctypes.c_ulong),
]
class Context(ctypes.Structure):
_fields_ = [
("path", BoundPath),
("parent_inode", InodeKey),
("monitored", ctypes.c_int),
("event_type", ctypes.c_int),
("symlink_target", ctypes.c_char * PATH_MAX),
]
maps = Path("fact-ebpf/src/bpf/maps.h").read_text()
assert "__uint(type, BPF_MAP_TYPE_LRU_HASH)" in maps
assert "__uint(max_entries, 16384)" in maps
print("BoundPath:", ctypes.sizeof(BoundPath))
print("InodeKey:", ctypes.sizeof(InodeKey))
print("Context:", ctypes.sizeof(Context))
print("Context alignment:", ctypes.alignment(Context))
print("Key:", ctypes.sizeof(ctypes.c_uint64))
print("Value payload:", ctypes.sizeof(Context) * max_entries)
print("Value payload MiB:", ctypes.sizeof(Context) * max_entries / (1024 * 1024))
print("Value payload at 1024 entries MiB:", ctypes.sizeof(Context) * 1024 / (1024 * 1024))
for field, _ in Context._fields_:
print(f"offset {field}: {getattr(Context, field).offset}")
PYRepository: stackrox/fact
Length of output: 433
Reduce d_instantiate_ctx’s preallocated capacity
struct d_instantiate_ctx_t is 12,320 bytes on the 64-bit BPF layout. BPF_MAP_TYPE_LRU_HASH preallocates all entries, so max_entries = 16384 requires at least 192.5 MiB for values, before key, bucket, and LRU-node overhead. Set max_entries from the maximum number of concurrent pid_tgid contexts, or redesign the storage to avoid embedding both path buffers in every entry.
🤖 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/maps.h` around lines 86 - 100, Reduce the
`d_instantiate_ctx` map’s `max_entries` from 16384 to a value bounded by the
maximum number of concurrent pid_tgid contexts, or redesign
`d_instantiate_ctx_t` storage to avoid embedding both path buffers per entry.
Preserve context correlation while ensuring the preallocated LRU hash value
memory fits the intended resource budget.
There was a problem hiding this comment.
I can't think of a way we could redesign this buffer ATM, I'll look into reducing the number of entries by default and make it configurable in case we need to tweak it in production, but will do so in a follow up PR.
There was a problem hiding this comment.
@Molter73, understood. The current finding remains open because d_instantiate_ctx still preallocates the large value capacity.
A follow-up that reduces the default capacity and adds production configuration is a reasonable approach. Would you like me to create a GitHub issue for that follow-up?
You are interacting with an AI system.
There was a problem hiding this comment.
@CodeRabbit follow up PR is already up: #1453
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="stackrox/fact"
echo "--- pull request metadata ---"
gh pr view 1453 --repo "$repo" --json number,title,state,url,body,headRefName,baseRefName
echo "--- changed files and patch for maps.h ---"
gh pr diff 1453 --repo "$repo" --name-only
gh pr diff 1453 --repo "$repo" -- fact-ebpf/src/bpf/maps.h 2>/dev/null || trueLength of output: 1829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="stackrox/fact"
patch="$(mktemp)"
trap 'rm -f "$patch"' EXIT
gh pr diff 1453 --repo "$repo" > "$patch"
echo "--- fact-ebpf/src/bpf/maps.h patch ---"
sed -n '/^diff --git a\/fact-ebpf\/src\/bpf\/maps\.h /,/^diff --git /p' "$patch"
echo "--- runtime configuration patches ---"
sed -n '/^diff --git a\/fact\/src\/bpf\/mod\.rs /,/^diff --git /p' "$patch"
sed -n '/^diff --git a\/fact\/src\/config\/mod\.rs /,/^diff --git /p' "$patch"
sed -n '/^diff --git a\/fact\/src\/config\/tests\.rs /,/^diff --git /p' "$patch"Length of output: 11785
@Molter73, verified. #1453 reduces the default d_instantiate_ctx capacity from 16,384 to 512 entries. It also adds runtime configuration through YAML, FACT_D_INSTANTIATE_CTX_SIZE, and --d-inst-size.
#1453 directly addresses this finding. The finding remains open on #1440 because the change is in the follow-up pull request.
You are interacting with an AI system.
With the changes introduced by #1440, the d_instantiate_ctx map has grown considerably in size. This patch reduces the default size and exposes a method for setting the size at runtime for fine-tweaking.
|
Looks like the symlink tests are very flaky, will set the PR back as open once I've managed to figure out why and make them more reliable. |
Add a LSM hook for path_symlink, allowing us to handle events that create and modify symlinks in monitored directories. We also change the host scanner methods to use `symlink_metadata` in order to properly capture these types of files.
Since path_symlink does not have an inode available for the symlink being created yet, we delegate obtaining that information to the d_instantiate hook as we do with path_mkdir. This means we will reliably get an inode in kernel space and we won't have to depend on userspace being fast enough to retrieve this information when the symlink is short lived. While working on this I also refactored some of the code in path_mkdir and d_instantiate to make them a bit simpler.
When reaching a symlink, whatever it points to will be added to the list of inodes to be tracked.
* Properly count the number of events for path_symlink as soon as possible. * Remove unused method `set_inode * Keep scanning when failing to retrieve metadata for a path.
* Properly check length returned by d_path. * Check return value of bpf_probe_read_str call. * Change string to bytes blob in test_path_symlink.py.
3a0a7ff to
bb01edc
Compare
With the changes introduced by #1440, the d_instantiate_ctx map has grown considerably in size. This patch reduces the default size and exposes a method for setting the size at runtime for fine-tweaking.
d8e986c to
bcd36b2
Compare
bcd36b2 to
40459ba
Compare
With the changes introduced by #1440, the d_instantiate_ctx map has grown considerably in size. This patch reduces the default size and exposes a method for setting the size at runtime for fine-tweaking.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_path_symlink.py`:
- Around line 204-207: Fix the path translation used by
test_follow_symlink_to_dir so absolute symlink targets resolve correctly when
running inside a container with host-mounted paths, then remove its
pytest.mark.skip decorator so CI executes the test.
🪄 Autofix
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: 3fb41b5d-5c60-4843-aaa0-1e04ce03344b
📒 Files selected for processing (4)
fact-ebpf/src/bpf/main.cfact-ebpf/src/lib.rsfact/src/host_scanner.rstests/test_path_symlink.py
🚧 Files skipped from review as they are similar to previous changes (3)
- fact-ebpf/src/lib.rs
- fact/src/host_scanner.rs
- fact-ebpf/src/bpf/main.c
2 participants
Description
Add a LSM hook for path_symlink, allowing us to handle events that create and modify symlinks in monitored directories. We also change the host scanner methods to use
symlink_metadatain order to properly capture these types of files. This hook does not have the required inode for the symlink, so generating the event is delegated tod_instantiateinstead.While working on delegating to
d_instatiatesome refactoring was done:path_readhelpers allowing for readingbound_path_tinto arbitrary buffers.path_readmethods to use these new helpers.d_instantiate_ctxmap.andpath_mkdir` hooks with the new helper functions.should_track_mkdirand replaced it with a call tois_monitoredpassingNULLas the first argument.Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Added integration tests for multiple cases.
Summary by CodeRabbit
New Features
Bug Fixes
Tests