Migrate cardwire-ebpf (C) to aya-ebpf (Rust) - #137
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces C eBPF components with Rust programs and a userspace loader. It updates daemon PID classification, GPU map handling, workspace membership, CI, Nix builds, and Arch packaging for nightly Rust and ChangesRust eBPF migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CardwireDaemon
participant EbpfBlocker
participant RustEbpfPrograms
CardwireDaemon->>EbpfBlocker: create blocker
EbpfBlocker->>RustEbpfPrograms: load and attach programs
RustEbpfPrograms-->>EbpfBlocker: expose maps and ring buffers
EbpfBlocker-->>CardwireDaemon: return typed eBPF handles
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
… and finis writing the dentry_blocked function
be8ae46 to
c7962f1
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cardwire-daemon/src/analyzer/models.rs (1)
193-198: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
spawn_remove_analyzerforgets to sweep the newly conjuredforced_map.This function only calls
pid_map.remove(&event.pid)on process exit. The newforced_map(introduced alongsidePidType::Forcedin this same file) is never cleaned up here.Every process the daemon marks
PidType::Forcedleaves a permanent entry inCW_FORCED_PID, a bounded 16384-entry eBPF map. Over the lifetime of a long-running daemon, this map fills with stale entries for exited processes, and once the kernel recycles a PID, a stale forced-map entry can silently misroute an unrelated new process to whatever GPU the old, unrelated process was forced onto.Remove from both maps on exit.
🔮 Proposed fix
async fn spawn_remove_analyzer(&self, event: CloseEvent) -> () { let mut pid_map = self.pid_map.write().await; if pid_map.remove(&event.pid).is_ok() { debug!("REMOVE: pid: {}", event.pid); } + drop(pid_map); + let mut forced_map = self.forced_map.write().await; + let _ = forced_map.remove(&event.pid); }🤖 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 `@crates/cardwire-daemon/src/analyzer/models.rs` around lines 193 - 198, Update spawn_remove_analyzer to remove event.pid from both pid_map and the newly introduced forced_map during process cleanup. Preserve the existing debug logging behavior while ensuring stale forced-map entries are deleted whenever a process exits.
🤖 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 @.github/workflows/cicd.yml:
- Around line 82-87: Update the “Clippy Workspace” step’s check-args to restore
the --locked flag alongside --all-targets and --all-features, preserving
reproducible CI behavior unless the nightly migration explicitly requires
lockfile regeneration.
- Around line 70-81: Pin all floating action references in the rust-lint,
rust-test, and rust-format jobs: replace each dtolnay/rust-toolchain@nightly
reference with a full commit SHA and explicitly set toolchain: nightly, and
replace both taiki-e/install-action@v2 references with full commit SHAs. Apply
the changes at .github/workflows/cicd.yml lines 70-81 and 99-108.
In `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 171-177: Update the PidType::Forced handling to insert the
resolved dGPU ID assigned by read_gpu, rather than hardcoded 1, into forced_map.
In the process-exit cleanup path, remove each closed PID from both eBPF maps,
including forced_map, so stale entries cannot persist or affect recycled PIDs.
In `@crates/cardwire-ebpf-userspace/src/lib.rs`:
- Around line 217-248: Replace the unwraps in get_exec_ring, get_close_ring,
get_report_ring, get_pid_map, get_forced_pid_map, and get_mode_map with the
established fallible error-handling pattern: convert missing take_map results
using CardwireEbpfError::missing_map and map try_from failures with
CardwireEbpfError::aya, propagating both through CardwireEbpfResult without
panicking.
- Around line 118-164: Update is_inode_blocked to treat successful lookup of the
inode in CW_BLOCKED_INO as blocked regardless of the stored GPU key; return
false only for MapError::KeyNotFound and preserve existing error propagation.
Remove the value == 1 check and update the nearby comment/doc wording so it
reflects presence-based blocking rather than fixed 0/1 values.
In `@crates/cardwire-ebpf/build.rs`:
- Around line 15-16: Update the bpf-linker lookup in the build script to replace
the bare unwrap with an explicit failure message naming the missing bpf-linker
executable and instructing users to install it or add it to PATH. Keep the
existing rerun-if-changed behavior unchanged when the lookup succeeds.
In `@crates/cardwire-ebpf/src/helpers.rs`:
- Around line 99-125: Extract the repeated CW_REPORT_EVENTS reserve/write/submit
sequence into an #[inline(always)] report_blocked helper accepting pid and
constructing ReportEvent internally. Replace all three duplicated reporting
blocks in the surrounding logic with calls to this helper, preserving the
existing conditional submission behavior.
- Around line 21-28: Update the map accesses in the inode-checking logic and
related settings/directory-entry operations to pass references as keys: use
borrowed keys for CW_BLOCKED_INO, CW_EXP_BLK_INO, CW_SETTINGS, CW_DIRENT.insert,
and CW_DIRENT.get. Preserve the existing lookup and insertion behavior while
applying the reference form consistently.
- Around line 55-61: The kernel and daemon use incompatible ReportEvent layouts
across the ring-buffer boundary, causing all reports to be rejected by the
daemon size check. Align the ReportEvent definition and
serialization/deserialization between the eBPF helper and daemon, either by
using one shared matching layout or by parsing the kernel’s 8-byte pid-only
record; update affected tests to validate the corrected ABI.
In `@crates/cardwire-ebpf/src/main.rs`:
- Around line 291-293: Update the surrounding function to convert the i64 errors
from TracePointContext::read_at and CW_DIRENT.insert into the function’s i32
error type before returning, using the required ReturnCode::SUCCESS success
value; pass references to pid and dirp_ptr to CW_DIRENT.insert instead of
values.
- Line 223: Wrap both unsafe operations in explicit unsafe blocks in the
function containing the path_ptr assignment, including the ctx.arg(0) call and
its companion call. Keep the existing mod vmlinux allowance unchanged, since it
does not cover this function.
In `@nix/default.nix`:
- Around line 47-63: Update the aya-build patch commands in postPatch so each
expected replacement is verified and the build fails if its target pattern is
absent. Preserve the existing replacements for "+nightly", "-Z", and
"build-std=core", and make the corresponding config.toml cleanup fail when its
expected build-std entry is missing, ensuring future vendored-source changes
surface during patching.
In `@packages/arch-linux/cardwire-PKGBUILD`:
- Around line 12-18: Update the Arch package build flow in prepare() to run the
locked dependency fetch through the nightly toolchain using rustup run nightly
cargo fetch --locked, and update build() to invoke cargo through rustup run
nightly as well. Apply the same explicit-nightly changes to the corresponding
cardwire-git-PKGBUILD while preserving the existing build arguments.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 193-198: Update spawn_remove_analyzer to remove event.pid from
both pid_map and the newly introduced forced_map during process cleanup.
Preserve the existing debug logging behavior while ensuring stale forced-map
entries are deleted whenever a process exits.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 61c277b1-9404-4adf-87aa-a65ad96a5d88
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.github/workflows/cicd.ymlCargo.tomlcrates/cardwire-daemon/Cargo.tomlcrates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/core/errors.rscrates/cardwire-daemon/src/interface/config.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/interface/switcheroo.rscrates/cardwire-daemon/src/models.rscrates/cardwire-daemon/src/tasks/watch_power_state.rscrates/cardwire-ebpf-userspace/Cargo.tomlcrates/cardwire-ebpf-userspace/build.rscrates/cardwire-ebpf-userspace/src/errors.rscrates/cardwire-ebpf-userspace/src/lib.rscrates/cardwire-ebpf/Cargo.tomlcrates/cardwire-ebpf/build.rscrates/cardwire-ebpf/src/c/bpf.ccrates/cardwire-ebpf/src/c/bpf.hcrates/cardwire-ebpf/src/c/helpers.hcrates/cardwire-ebpf/src/helpers.rscrates/cardwire-ebpf/src/lib.rscrates/cardwire-ebpf/src/main.rscrates/cardwire-ebpf/src/maps.rscrates/cardwire-ebpf/src/vmlinux.rscrates/cardwire-gui/src/ui.rsflake.nixnix/default.nixpackages/arch-linux/cardwire-PKGBUILDpackages/arch-linux/cardwire-git-PKGBUILD
💤 Files with no reviewable changes (3)
- crates/cardwire-ebpf/src/c/helpers.h
- crates/cardwire-ebpf/src/c/bpf.c
- crates/cardwire-ebpf/src/c/bpf.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cardwire-daemon/src/interface/gpu.rs (1)
62-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the hotplug caller for the GPU map-key contract.
By the GPU runes,
block_gpunow usesvalueas the GPU map key.crates/cardwire-daemon/src/interface/debug.rsLine 150 still callsgpu.block_gpu(1). If a re-discovered blocked GPU has an ID other than1, hotplug writes its inode policy under GPU key1.Iterate over
gpu_interfaceswith its key and pass that key toblock_gpu, or makeblock_gpuuseself.id.Proposed fix in `crates/cardwire-daemon/src/interface/debug.rs`
- for gpu in gpu_interfaces.values_mut() { + for (id, gpu) in gpu_interfaces.iter_mut() { let should_block = match mode { Modes::Integrated | Modes::Smart => !gpu.device.is_default(), Modes::Hybrid => false, @@ - if let Err(e) = gpu.block_gpu(1).await { + if let Err(e) = gpu.block_gpu(*id as u32).await {🤖 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 `@crates/cardwire-daemon/src/interface/gpu.rs` at line 62, Update the hotplug caller in the GPU interface handling around gpu_interfaces to iterate with each map key and pass that key to block_gpu instead of the hardcoded value 1, preserving the correct GPU key for re-discovered devices..github/workflows/cicd.yml (1)
78-83: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftCast a dedicated nightly Clippy check for
crates/cardwire-ebpf.
crates/cardwire-ebpfis not a default member. The action does not pass--workspace, so-D warningsdoes not lint this crate. Add a dedicated nightly check for it.🤖 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 @.github/workflows/cicd.yml around lines 78 - 83, Add a dedicated nightly Clippy workflow step targeting the non-default-member crate `crates/cardwire-ebpf`, ensuring it runs with warnings treated as errors and uses the existing bpf-linker setup. Keep the existing `Clippy Workspace` check unchanged.Source: MCP tools
🤖 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.
Outside diff comments:
In @.github/workflows/cicd.yml:
- Around line 78-83: Add a dedicated nightly Clippy workflow step targeting the
non-default-member crate `crates/cardwire-ebpf`, ensuring it runs with warnings
treated as errors and uses the existing bpf-linker setup. Keep the existing
`Clippy Workspace` check unchanged.
In `@crates/cardwire-daemon/src/interface/gpu.rs`:
- Line 62: Update the hotplug caller in the GPU interface handling around
gpu_interfaces to iterate with each map key and pass that key to block_gpu
instead of the hardcoded value 1, preserving the correct GPU key for
re-discovered devices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a94be4f-1244-4e41-8d76-7c989daa0333
📒 Files selected for processing (6)
.github/workflows/cicd.ymlcrates/cardwire-daemon/src/analyzer/dynamic_analysis.rscrates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-daemon/src/models.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cardwire-ebpf-userspace/src/lib.rs (2)
78-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win🧙 Correct the missing tracepoint name in the error.
The lookup uses
tracepoint_exit_getdents64, but the error reportstry_tracepoint_exit_getdents64. When the program is missing, the diagnostic names a program that does not exist. Use the lookup name inCardwireEbpfError::missing_lsm.🔧 Proposed fix
- .ok_or_else(|| CardwireEbpfError::missing_lsm("try_tracepoint_exit_getdents64"))? + .ok_or_else(|| CardwireEbpfError::missing_lsm("tracepoint_exit_getdents64"))?🤖 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 `@crates/cardwire-ebpf-userspace/src/lib.rs` around lines 78 - 82, Update the CardwireEbpfError::missing_lsm argument in the tracepoint_exit_getdents64 lookup to report the exact lookup name, tracepoint_exit_getdents64, instead of the mismatched try_tracepoint_exit_getdents64 value.
118-145: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift🧙 Preserve shared PCI-inode ownership when unblocking.
pci_to_inodewalks parent PCI devices without deduplication, so GPUs can share a parent inode.CW_BLOCKED_INOstores one GPU ID per inode, butunblock_inoderemoves entries by inode only. Unblocking one GPU can stop blocking a shared parent inode for another GPU. Pass the GPU ID tounblock_inodeand use reference counting or multi-owner storage.gpu_blockeddoes not check parent PCI inodes and cannot detect this condition.🤖 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 `@crates/cardwire-ebpf-userspace/src/lib.rs` around lines 118 - 145, Update unblock_inode and its callers to accept the GPU ID alongside the inode, and preserve CW_BLOCKED_INO entries still owned by other GPUs when unblocking a shared parent PCI inode. Replace inode-only removal with ownership-aware reference counting or multi-owner storage, and update block_inode consistently so shared inode ownership is retained until the final GPU is unblocked.
🤖 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.
Outside diff comments:
In `@crates/cardwire-ebpf-userspace/src/lib.rs`:
- Around line 78-82: Update the CardwireEbpfError::missing_lsm argument in the
tracepoint_exit_getdents64 lookup to report the exact lookup name,
tracepoint_exit_getdents64, instead of the mismatched
try_tracepoint_exit_getdents64 value.
- Around line 118-145: Update unblock_inode and its callers to accept the GPU ID
alongside the inode, and preserve CW_BLOCKED_INO entries still owned by other
GPUs when unblocking a shared parent PCI inode. Replace inode-only removal with
ownership-aware reference counting or multi-owner storage, and update
block_inode consistently so shared inode ownership is retained until the final
GPU is unblocked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5beaecb8-5f0e-47b3-bda9-2286d87bb426
📒 Files selected for processing (2)
crates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-ebpf-userspace/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/cardwire-ebpf/src/main.rs (1)
183-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCast explicit
unsafeblocks around the barectx.arg(0)calls.Two sites still call
ctx.arg(0)outside anunsafeblock, even though the enclosing function isunsafe fn:
- Line 183 in
try_inode_permission.- Line 241 in
try_inode_getattr(raised in a prior review round, still unresolved here).Edition 2024 emits
unsafe_op_in_unsafe_fnfor these bare calls, and the crate's#[allow]only coversmod vmlinux. A build with-D warningsfails on both sites.🪄 Proposed fix
- let inode_ptr: *mut inode = ctx.arg(0); + let inode_ptr: *mut inode = unsafe { ctx.arg(0) };- let path_ptr: *const path = ctx.arg(0); + let path_ptr: *const path = unsafe { ctx.arg(0) };Also applies to: 241-241
🤖 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 `@crates/cardwire-ebpf/src/main.rs` at line 183, Wrap each bare ctx.arg(0) call in explicit unsafe blocks in try_inode_permission and try_inode_getattr. Update both inode_ptr initializations so Edition 2024 no longer reports unsafe_op_in_unsafe_fn warnings, without changing surrounding logic.crates/cardwire-ebpf-userspace/src/lib.rs (1)
46-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the missing-program error for tracepoints; "missing lsm" is the wrong rune for them.
exec_program,close_program,cardwire_sys_enter_getdents64, andcardwire_sys_exit_getdents64(lines 46-92) all useCardwireEbpfError::missing_lsm(...)whenprogram_mutreturnsNone. These areTracePointprograms, not LSM hooks, yet the resulting message reads "missing lsm: tracepoint_sched_process_exec". Anyone debugging a failed daemon startup gets a misleading diagnostic pointing at the wrong subsystem.Add a
missing_program(or similarly generic) variant and constructor inerrors.rs, and use it for the fourTracePointlookups here, keepingmissing_lsmfor the three actual LSM hooks at lines 35-44.🤖 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 `@crates/cardwire-ebpf-userspace/src/lib.rs` around lines 46 - 92, add a generic missing_program error variant and constructor in errors.rs, then replace CardwireEbpfError::missing_lsm with it for the exec_program, close_program, cardwire_sys_enter_getdents64, and cardwire_sys_exit_getdents64 TracePoint lookups. Leave missing_lsm unchanged for the actual LSM hook lookups.crates/cardwire-daemon/src/analyzer/models.rs (1)
236-241: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCast the
Forceddecision from presence, not GPU ID
CARDWIRE_FORCE_GPUaccepts arbitrary GPU IDs, butvalue == 1blocks IDs such as0and2. UseSome(_)for the boolean decision and preservevalueas the target GPU ID.🤖 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 `@crates/cardwire-daemon/src/analyzer/models.rs` around lines 236 - 241, Update the CARDWIRE_FORCE_DGPU and CARDWIRE_FORCE_GPU branches in the environment decision logic so the boolean result is true whenever the variable is present, regardless of its numeric GPU ID. Preserve each parsed value as the returned target GPU ID.
🤖 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 `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 457-484: Update the stale size comment in
test_report_event_deserialization_from_valid_bytes to describe ReportEvent as
containing only the 4-byte pid field. Rename
test_report_event_comm_extraction_with_full_length_name to reflect that it
validates ReportEvent deserialization or pid extraction, while preserving its
current assertions.
In `@crates/cardwire-ebpf-userspace/src/lib.rs`:
- Around line 362-368: In the AsyncFd::with_interest match, replace the Err(err)
arm’s Err(CardwireEbpfError::aya(err))? expression with a direct return
Err(CardwireEbpfError::aya(err)); while preserving the existing error log.
- Around line 218-350: Introduce a shared generic helper on the containing type,
such as take_typed_map<T, E>, to take a named map, return
CardwireEbpfError::missing_map when absent, and convert TryFrom failures through
CardwireEbpfError::aya. Refactor get_exec_ring, get_close_ring, get_report_ring,
get_pid_map, get_forced_pid_map, and get_mode_map to call this helper while
preserving their existing map names and return types.
In `@crates/cardwire-ebpf/src/main.rs`:
- Around line 308-317: Verify the eBPF-target build for
try_tracepoint_enter_getdents64, specifically the ? operators on ctx.read_at and
CW_DIRENT.insert, using the aya_build::build_ebpf path. If their i64 errors
cannot convert into the function’s i32 error type, explicitly map each error to
i32 before propagation while preserving successful values and the Result<i32,
i32> contract.
- Around line 505-508: Update the error message in
try_tracepoint_sched_process_exit to name tracepoint_sched_process_exit instead
of tracepoint_sched_process_exec, leaving the corresponding message in
try_tracepoint_sched_process_exec unchanged.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 236-241: Update the CARDWIRE_FORCE_DGPU and CARDWIRE_FORCE_GPU
branches in the environment decision logic so the boolean result is true
whenever the variable is present, regardless of its numeric GPU ID. Preserve
each parsed value as the returned target GPU ID.
In `@crates/cardwire-ebpf-userspace/src/lib.rs`:
- Around line 46-92: add a generic missing_program error variant and constructor
in errors.rs, then replace CardwireEbpfError::missing_lsm with it for the
exec_program, close_program, cardwire_sys_enter_getdents64, and
cardwire_sys_exit_getdents64 TracePoint lookups. Leave missing_lsm unchanged for
the actual LSM hook lookups.
In `@crates/cardwire-ebpf/src/main.rs`:
- Line 183: Wrap each bare ctx.arg(0) call in explicit unsafe blocks in
try_inode_permission and try_inode_getattr. Update both inode_ptr
initializations so Edition 2024 no longer reports unsafe_op_in_unsafe_fn
warnings, without changing surrounding logic.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06fecf5a-d46e-459c-b832-4a203c46e48d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/cardwire-daemon/Cargo.tomlcrates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-ebpf-userspace/src/lib.rscrates/cardwire-ebpf/src/helpers.rscrates/cardwire-ebpf/src/main.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cardwire-daemon/src/analyzer/models.rs (1)
240-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow non-
1GPU IDs through the force path.
CARDWIRE_FORCE_GPU=2returns(false, PidType::Forced, 2).spawn_exec_analyzerthen skips the forced-map insertion because it requiresresult.0to be true.Treat a parsed
CARDWIRE_FORCE_GPUvalue as a force request. Preserve the value as the GPU map ID. Validate the ID separately if required.Proposed fix
if let Some(value) = check_env("CARDWIRE_FORCE_GPU", &environ) { - return Some((value == 1, PidType::Forced, value)); + return Some((true, PidType::Forced, value)); }🤖 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 `@crates/cardwire-daemon/src/analyzer/models.rs` around lines 240 - 245, Update the CARDWIRE_FORCE_GPU branch in the environment-selection logic so any successfully parsed value requests the forced path, rather than setting the boolean from value == 1; preserve the parsed value as the GPU map ID and leave CARDWIRE_FORCE_DGPU behavior unchanged. Ensure spawn_exec_analyzer can insert non-1 forced GPU IDs, with separate validation only if required by existing ID rules.crates/cardwire-ebpf-userspace/src/lib.rs (1)
196-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the kernel’s terminating NUL byte in
CW_ALLOWED_COMM.bpf_get_current_comm()returns[u8; 16]with at most 15 command bytes and a trailing NUL. For inputs of 16 bytes or more, this code stores 16 non-NUL bytes, so the lookup fails. Limit the copied length to 15 bytes or reject longer names.🤖 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 `@crates/cardwire-ebpf-userspace/src/lib.rs` around lines 196 - 203, Update allow_comm so the generated 16-byte key always reserves the final byte for the kernel’s terminating NUL: cap the copied command length at 15 bytes, or reject inputs longer than 15 bytes while preserving the existing result 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 `@crates/cardwire-ebpf/src/helpers.rs`:
- Around line 152-160: Replace the authorization logic in is_comm_whitelisted so
it no longer uses the mutable bpf_get_current_comm value or CW_ALLOWED_COMM as
the trust decision. Base authorization on a daemon-controlled identity, such as
verified executable identity or managed PID/cgroup membership, and preserve the
boolean contract used by the hooks in main.rs.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 240-245: Update the CARDWIRE_FORCE_GPU branch in the
environment-selection logic so any successfully parsed value requests the forced
path, rather than setting the boolean from value == 1; preserve the parsed value
as the GPU map ID and leave CARDWIRE_FORCE_DGPU behavior unchanged. Ensure
spawn_exec_analyzer can insert non-1 forced GPU IDs, with separate validation
only if required by existing ID rules.
In `@crates/cardwire-ebpf-userspace/src/lib.rs`:
- Around line 196-203: Update allow_comm so the generated 16-byte key always
reserves the final byte for the kernel’s terminating NUL: cap the copied command
length at 15 bytes, or reject inputs longer than 15 bytes while preserving the
existing result behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4f25f4c3-8b53-4bc4-8bf7-f5105a00ce88
📒 Files selected for processing (7)
crates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/models.rscrates/cardwire-ebpf-userspace/src/lib.rscrates/cardwire-ebpf/Cargo.tomlcrates/cardwire-ebpf/src/helpers.rscrates/cardwire-ebpf/src/main.rscrates/cardwire-ebpf/src/maps.rs
1 participant
Description
Rewrite the ebpf program present inside cardwire-ebpf to be written in Rust using aya-ebpf
Why ?
Fixes # (issue)
TODO
Checklist: