fix(cardwired): directly clean PID MAPS inside the bpf code - #158
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBehold: the daemon now exposes SmartPolicyInterface over D-Bus, shares PID maps through EbpfBlocker, removes policy entries during eBPF process lifecycle events, and processes execution events without close-event handling. ChangesSmart policy and PID lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DbusClient
participant SmartPolicyInterface
participant pid_map
participant forced_map
DbusClient->>SmartPolicyInterface: request_process_access(pid, policy, value)
SmartPolicyInterface->>pid_map: update or remove allow entry
SmartPolicyInterface->>forced_map: update or remove force entry
SmartPolicyInterface-->>DbusClient: policy result or D-Bus error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
41-50: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRestore expiry for
reported_pids.By the runes, removing close-event handling also removed the only cleanup for
reported_pids. The set now grows for the daemon lifetime. A reused PID also suppresses reports for an unrelated process.Keep process-exit cleanup, or replace this set with a bounded TTL cache.
🤖 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 41 - 50, Restore cleanup for reported_pids in CardwireAnalyzer so entries are removed when their processes exit, or replace the HashSet with a bounded TTL cache. Ensure reused PIDs can be reported again and the collection cannot grow for the daemon’s entire lifetime.
🤖 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 131-143: Update the execution-ring readiness handling in run so
errors from exec_ring.ready_mut(Interest::READABLE).await are not discarded: use
the same reporting pattern as report_logger, log the failure, and return the
error from run. Preserve the existing readable-event processing for successful
readiness checks.
In `@crates/cardwire-daemon/src/interface/smart.rs`:
- Around line 37-40: Update the policy-key construction and insertion flow
around the visible `/proc/<pid>` existence check to use a stable
process-lifetime identifier, such as the process start identity, rather than PID
alone. Ensure the identifier is obtained and validated before map insertion, and
key authorization so PID reuse cannot inherit the previous process policy; do
not rely solely on `Path::new(...).exists()`.
- Around line 43-86: Update the policy transition handler around the
`"Default"`, `"Allow_dGPU"`, `"Force_dGPU"`, and `"Force_GPU"` branches to treat
`AyaMapError::KeyNotFound` from removals as success. Make `"Default"` remove the
PID from both maps, and introduce/use one shared transition lock held across
each branch’s complete removal and insertion so concurrent requests cannot leave
the PID in both maps.
In `@crates/cardwire-ebpf/src/main.rs`:
- Around line 463-464: Move removal of CW_ALLOWED_PID and CW_FORCED_PID entries
in crates/cardwire-ebpf/src/main.rs#L463-L464 before the is_cardwired,
whitelist, Smart-mode, and ring-buffer-reservation early returns so cleanup is
independent of event delivery. In the main-thread exit handling at
crates/cardwire-ebpf/src/main.rs#L485-L501, remove both map entries regardless
of whether Smart mode is active.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/analyzer/models.rs`:
- Around line 41-50: Restore cleanup for reported_pids in CardwireAnalyzer so
entries are removed when their processes exit, or replace the HashSet with a
bounded TTL cache. Ensure reused PIDs can be reported again and the collection
cannot grow for the daemon’s entire lifetime.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 380d37bd-42d4-4c31-a511-82a1c9cc74bc
📒 Files selected for processing (8)
crates/cardwire-daemon/src/analyzer/models.rscrates/cardwire-daemon/src/daemon.rscrates/cardwire-daemon/src/interface/mod.rscrates/cardwire-daemon/src/interface/smart.rscrates/cardwire-daemon/src/models.rscrates/cardwire-ebpf-userspace/src/lib.rscrates/cardwire-ebpf/src/main.rscrates/cardwire-ebpf/src/maps.rs
💤 Files with no reviewable changes (1)
- crates/cardwire-ebpf/src/maps.rs
1 participant
Description
Please include a summary of the changes and if applicable, a related issue.
If this PR introduce a new feature, explain your motivations
Fixes # (issue)
TODO
Checklist: