Say why the fast path cannot serve an unprivileged caller (#36) - #41
Conversation
drmtap_grab_mapped_fast has no helper fallback: caching a persistent CPU mapping per framebuffer is its whole purpose, and a helper hands over a fresh fd per grab, so there is nothing stable to cache. Without CAP_SYS_ADMIN drmModeGetFB2 zeroes the GEM handles and it can only fail, on any GPU. It failed as -EACCES per frame behind a CACHE MISS line per frame, with the reason reachable only through drmtap_error(). That reads as a broken cache. It cost the reporter of #36 three rounds and cost me two wrong answers: I diagnosed a tiling/mmap problem that his fast path never reached, because it stopped at the privilege check before it. Now the first call says it once, in full, and names drmtap_grab_mapped() as the call that does fall back to the helper. A sticky flag answers every later call at the top of the function, so a capture loop running at frame rate produces no repeats. The flag is deliberately not cleared with the rest of the fast state: it records a property of the process, not of the plane or the framebuffer. Verified unprivileged on i915: one explanation, then silent -EACCES.
|
Warning Review limit reached
Next review available in: 46 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe fast persistent-mmap capture path now records missing CAP_SYS_ADMIN privilege in the context, preserves that state across cleanup, reports the fallback, and returns early with ChangesFast capture privilege handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FastGrab
participant DRM
participant Cleanup
Caller->>FastGrab: request fast capture
FastGrab->>DRM: drmModeGetFB2()
DRM-->>FastGrab: no exported framebuffer handle
FastGrab->>Cleanup: release cached fast resources
Cleanup-->>FastGrab: preserve fast_no_privilege
FastGrab-->>Caller: -EACCES and fallback guidance
Caller->>FastGrab: request fast capture again
FastGrab-->>Caller: immediate -EACCES
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@src/drm_grab.c`:
- Around line 1514-1530: After setting fast_no_privilege in the
privilege-failure path of drmtap_grab_mapped_fast, call drmtap_fast_cleanup(ctx)
to release cached fast-path resources. Apply this identical change at
src/drm_grab.c lines 1514-1530 and bindings/rust/libdrmtap-sys/csrc/drm_grab.c
lines 1514-1530.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5e704206-0354-4390-a925-a381a1449ada
📒 Files selected for processing (4)
bindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/drm_grab.csrc/drmtap_internal.h
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (rust)
- GitHub Check: Build & Test (Ubuntu 22.04)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation, no tabs, snake_case names, same-line braces, 100-character soft and 120-character hard line limits, and the specified comment conventions.
Every C source and header file must begin with the specified libdrmtap copyright, license, and file documentation header block.
Files:
src/drmtap_internal.hbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.h
📄 CodeRabbit inference engine (AGENTS.md)
Use
snake_case_tfor internal types,drmtap_*for public types,DRMTAP_UPPER_SNAKE_CASEfor macros/constants, and conventionalDRMTAP_MODULE_Hheader guards.
Files:
src/drmtap_internal.h
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Changes must compile with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
src/drmtap_internal.hbindings/rust/libdrmtap-sys/csrc/drmtap_internal.hsrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{c,h}: Check modifiers for tiling:DRM_FORMAT_MOD_LINEARrequires no deswizzle; tiled or compressed framebuffers usegpu_egl.cas the primary detile path, with CPU deswizzle only as a fallback.
For HDR output metadata with PQ, tone-map supported 10-bit and 16-bit scanouts to 8-bit SDR using PQ decoding, BT.2020-to-BT.709 mapping, tone mapping, and sRGB encoding in both CPU and EGL paths; do not claim P010 or HLG support.
Files:
src/drmtap_internal.hsrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Prefix public C API functions withdrmtap_; use negative errno values for errors, never abort, check allocations, and free resources during cleanup.
Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Use simple//comments for internal static functions.
On errors, return negative errno values, set a human-readable error withset_error, and clean up resources on every error path, preferably using agoto cleanuppattern.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper; use the Prime path rather than GEM_FLINK.
Files:
src/drm_grab.c
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()on every frame.
Files:
src/drm_grab.c
🪛 Clang (14.0.6)
src/drm_grab.c
[note] 1365-1365: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c
[note] 1365-1365: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
🔇 Additional comments (3)
src/drmtap_internal.h (1)
99-108: 🎯 Functional CorrectnessConfirm the latch’s ownership scope.
The field is embedded in each
drmtap_ctx, making the latch per-context rather than process-wide.
src/drmtap_internal.h#L99-L108: document the intended per-context behavior or use shared process state if the process-level contract is required.bindings/rust/libdrmtap-sys/csrc/drmtap_internal.h#L99-L108: keep the generated header synchronized with the chosen ownership model.bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
1303-1306: LGTM!Also applies to: 1361-1371
src/drm_grab.c (1)
1303-1306: LGTM!Also applies to: 1361-1371
The reporter of #36 called drmtap_grab() once for format info and then drmtap_grab_mapped_fast() for frames. Both halves of that were reasonable readings of the header, and both fail on his gpu: - drmtap_grab does no conversion, so on a scanout that cannot be CPU-mapped frame->data is NULL and the call still returns 0. The doc advertised the dma_buf_fd and never said there are no CPU pixels, so reading data after a success looked supported. It renders black on amdgpu GFX9+, discrete VRAM and nvidia. - drmtap_grab_mapped_fast needs CAP_SYS_ADMIN and has no helper fallback. The runtime now says so; the header said nothing at all. Both are documented now, with the working call named in each case. Also release the fast slots when latching fast_no_privilege, per review: an unprivileged process never populated one, but a process that captured with the capability and then dropped it leaves them live and unusable until drmtap_close.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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 (1)
bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
1514-1537: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftLatch privilege loss before cached-frame reuse.
The new privilege latch is reached only on cache misses; cached-frame paths can bypass it after
CAP_SYS_ADMINis dropped.
bindings/rust/libdrmtap-sys/csrc/drm_grab.c#L1514-L1537: check privilege before both cache-hit paths or invalidate cached slots when capability loss is detected.src/drm_grab.c#L1514-L1537: apply the same fix and add regression coverage for unchanged and previously cached framebuffers.🤖 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 `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c` around lines 1514 - 1537, Ensure privilege loss is detected before either cached-frame reuse path in drmtap_grab_mapped_fast, so CAP_SYS_ADMIN removal cannot bypass the fast_no_privilege latch; invalidate cached slots when detected or perform the privilege check before cache hits. Apply the same change in bindings/rust/libdrmtap-sys/csrc/drm_grab.c:1514-1537 and src/drm_grab.c:1514-1537, and add regression coverage for both unchanged and previously cached framebuffers.
🤖 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 `@bindings/rust/libdrmtap-sys/csrc/drmtap.h`:
- Around line 154-162: Update the drmtap_frame_info field documentation in
bindings/rust/libdrmtap-sys/csrc/drmtap.h lines 154-162 and include/drmtap.h
lines 154-162 to state that zero-copy frame data may be available but is not
guaranteed; keep both headers’ contracts identical and consistent with the
surrounding drmtap_grab documentation.
---
Outside diff comments:
In `@bindings/rust/libdrmtap-sys/csrc/drm_grab.c`:
- Around line 1514-1537: Ensure privilege loss is detected before either
cached-frame reuse path in drmtap_grab_mapped_fast, so CAP_SYS_ADMIN removal
cannot bypass the fast_no_privilege latch; invalidate cached slots when detected
or perform the privilege check before cache hits. Apply the same change in
bindings/rust/libdrmtap-sys/csrc/drm_grab.c:1514-1537 and
src/drm_grab.c:1514-1537, and add regression coverage for both unchanged and
previously cached framebuffers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6004176b-f741-40e6-923d-ad6be876a4ba
📒 Files selected for processing (4)
bindings/rust/libdrmtap-sys/csrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drmtap.hinclude/drmtap.hsrc/drm_grab.c
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Build & Test (Ubuntu 22.04)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{c,h}: Use C11 style with 4-space indentation, no tabs, snake_case names, same-line braces, 100-character soft and 120-character hard line limits, and the specified comment conventions.
Every C source and header file must begin with the specified libdrmtap copyright, license, and file documentation header block.
Files:
include/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
include/drmtap.h
📄 CodeRabbit inference engine (AGENTS.md)
Document public API functions with Doxygen comments, including behavior, parameters, return values, and relevant error codes.
Files:
include/drmtap.h
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Changes must compile with Meson using
-Wall -Wextra -Werrorand produce zero warnings.
Files:
include/drmtap.hbindings/rust/libdrmtap-sys/csrc/drmtap.hsrc/drm_grab.cbindings/rust/libdrmtap-sys/csrc/drm_grab.c
src/**/*.c
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.c: Prefix public C API functions withdrmtap_; use negative errno values for errors, never abort, check allocations, and free resources during cleanup.
Organize source files as grouped system/library/project includes, private types and constants, static functions, then public functions in header declaration order.
Use simple//comments for internal static functions.
On errors, return negative errno values, set a human-readable error withset_error, and clean up resources on every error path, preferably using agoto cleanuppattern.
Whenhandles[0] == 0, treat it as missingCAP_SYS_ADMINand trigger the privilege helper; use the Prime path rather than GEM_FLINK.
Files:
src/drm_grab.c
src/drm_grab.c
📄 CodeRabbit inference engine (AGENTS.md)
Never cache
fb_id; refresh it withdrmModeGetPlane()on every frame.
Files:
src/drm_grab.c
src/**/*.{c,h}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{c,h}: Check modifiers for tiling:DRM_FORMAT_MOD_LINEARrequires no deswizzle; tiled or compressed framebuffers usegpu_egl.cas the primary detile path, with CPU deswizzle only as a fallback.
For HDR output metadata with PQ, tone-map supported 10-bit and 16-bit scanouts to 8-bit SDR using PQ decoding, BT.2020-to-BT.709 mapping, tone mapping, and sRGB encoding in both CPU and EGL paths; do not claim P010 or HLG support.
Files:
src/drm_grab.c
🔇 Additional comments (4)
bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)
210-219: LGTM!include/drmtap.h (1)
210-219: LGTM!bindings/rust/libdrmtap-sys/csrc/drm_grab.c (1)
1303-1306: LGTM!Also applies to: 1361-1371
src/drm_grab.c (1)
1303-1306: LGTM!Also applies to: 1361-1371
The field comment said data is NULL on the zero-copy path. It is not: where the scanout can be CPU-mapped, drmtap_grab hands back that raw mapping, so data is non-NULL and still tiled. Only where no CPU mapping is possible is it NULL, and the call returns 0 either way. That difference is the whole of issue #36. On an Intel scanout the reporter would have got pixels from drmtap_grab and it would have looked supported; on his Vega he got NULL, so the API contract failure presented as a driver bug, and I went looking for one. Both headers now say data is not guaranteed on that path, that what is there is unconverted, and which call to use instead.
There was a problem hiding this comment.
fxd0h has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Closes the diagnosability half of #36.
drmtap_grab_mapped_fasthas no helper fallback, by design: caching a persistent CPU mapping per framebuffer is its whole purpose, and a helper hands over a fresh fd per grab, so there is nothing stable to cache. Without CAP_SYS_ADMIN,drmModeGetFB2zeroes the GEM handles and the entry point can only fail, on any GPU.The problem was how it said so. Per frame it logged
fast2: CACHE MISS fb=N slot=0 (cold start)and returned -EACCES, with the actual reason reachable only throughdrmtap_error(). That reads as a broken cache, which is exactly how it was reported. It cost the reporter three rounds, and it cost me two wrong answers: I diagnosed a tiling/mmap problem that his fast path never reached, because it stopped at the privilege check before it.Now the first call states it once and names
drmtap_grab_mapped()as the call that does fall back to the helper. A stickyfast_no_privilegeanswers every later call at the top of the function, so a capture loop at frame rate produces no repeats.The flag is deliberately not cleared alongside the rest of the fast state: it records a property of the process, not of the plane, the framebuffer or the device, so re-discovering it after a teardown would repeat the whole GetFB2 dance to reach the same verdict. There is a comment at the reset site saying so, since the omission otherwise reads as an oversight.
Verified unprivileged on i915 (uid 1000), three consecutive calls:
csrc/ is regenerated with tools/sync-crate.sh (--check clean).
Greptile Summary
This PR improves diagnosability for unprivileged callers of
drmtap_grab_mapped_fastby introducing a stickyfast_no_privilegeflag that fires a clear, one-time diagnostic log on the first failure and silences all subsequent per-frame noise, addressing the confusing "CACHE MISS cold start" spam that obscured the real cause in issue #36.fast_no_privilegetodrmtap_ctx; on first detection ofhandles[0]==0the flag is latched, a descriptivedrmtap_debug_logmessage is emitted once, and a helpful error string (namingdrmtap_grab_mapped()as the correct alternative) is set viadrmtap_set_error.drmtap_grab_mapped_fastanswers all subsequent calls immediately and silently, eliminating per-frame repetition without changing the-EACCESreturn code.drmtap_fast_cleanup(), with a comment explaining it records a process-level property rather than a device or framebuffer property; the Rustcsrc/copies are regenerated in lockstep.Confidence Score: 5/5
Safe to merge; all changes are purely diagnostic — no data paths, cache logic, or resource management are altered.
The only observable behavioral change is that repeated calls from an unprivileged process now return -EACCES immediately and silently instead of re-running the GetFB2 dance and emitting a misleading log line each time. The sticky flag follows the exact same pattern as the existing fast_no_cpu_map field, the cleanup comment clearly documents the intentional omission, and the Rust bindings are kept in sync. No new error paths, no resource allocation changes.
Files Needing Attention: No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[drmtap_grab_mapped_fast called] --> B{fast_no_privilege set?} B -->|yes| C[drmtap_set_error: needs CAP_SYS_ADMIN] C --> D[return -EACCES, no log output] B -->|no| E[Step 1: Find plane on first call] E --> F[Step 2: Get fb_id from plane] F --> G[Cache lookup by fb_id] G -->|HIT| H[Serve frame from cached slot] G -->|MISS| I[drmModeGetFB2] I --> J{handles zero?} J -->|yes| K[drmModeFreeFB2, set fast_no_privilege = 1] K --> L[drmtap_debug_log: one-time diagnostic] L --> M[drmtap_set_error] M --> N[return -EACCES] J -->|no| O[Populate cache slot and map DMA-BUF] O --> H H --> P[return 0]Reviews (1): Last reviewed commit: "Say why the fast path cannot serve an un..." | Re-trigger Greptile
Context used: