Add sleepable program support and might_sleep helper gating#1136
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds program-level sleepable metadata and helper-level might_sleep flags, extends Linux section-prefix detection for ChangesSleepable eBPF Programs and Helper Gating
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 098061cb00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 (1)
src/linux/linux_platform.cpp (1)
161-163:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrefix ordering breaks sleepable detection.
The prefix list order causes incorrect matches. When the section is
fentry.s/foo, the loop checksfentry/first (line 190:section.find(prefix) == 0). Since"fentry.s/foo"starts with"fentry/", it matches and returns immediately withprefix = "fentry/". Line 192 then checks"fentry/".find(".s/"), which fails, sois_sleepableremainsfalse.All sleepable sections (fentry.s/, fexit.s/, fmod_ret.s/, iter.s/, tp_btf.s/, struct_ops.s/) will incorrectly match their non-sleepable variants first and be classified as non-sleepable, breaking the entire feature.
🐛 Reorder prefixes to check sleepable variants first
PTYPE("tracing", &g_tracing_descr, BPF_PROG_TYPE_TRACING, - {"fentry/" COMMA "fentry.s/" COMMA "fexit/" COMMA "fexit.s/" COMMA "fmod_ret/" COMMA "fmod_ret.s/" COMMA - "iter/" COMMA "iter.s/" COMMA "tp_btf/" COMMA "tp_btf.s/"}), + {"fentry.s/" COMMA "fentry/" COMMA "fexit.s/" COMMA "fexit/" COMMA "fmod_ret.s/" COMMA "fmod_ret/" COMMA + "iter.s/" COMMA "iter/" COMMA "tp_btf.s/" COMMA "tp_btf/"}), // struct_ops callbacks receive function arguments as u64 array, same as fentry/fexit. - PTYPE("struct_ops", &g_tracing_descr, BPF_PROG_TYPE_STRUCT_OPS, {"struct_ops/" COMMA "struct_ops.s/"}), - PTYPE("lsm", &g_tracing_descr, BPF_PROG_TYPE_LSM, {"lsm/" COMMA "lsm.s/"}), + PTYPE("struct_ops", &g_tracing_descr, BPF_PROG_TYPE_STRUCT_OPS, {"struct_ops.s/" COMMA "struct_ops/"}), + PTYPE("lsm", &g_tracing_descr, BPF_PROG_TYPE_LSM, {"lsm.s/" COMMA "lsm/"}),🤖 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 `@src/linux/linux_platform.cpp` around lines 161 - 163, The prefix list for tracing in the PTYPE initializer causes sleepable variants like "fentry.s/" to be shadowed by shorter prefixes ("fentry/"), so update the prefixes passed to PTYPE("tracing", &g_tracing_descr, ...) so that all ".s/" (sleepable) variants appear before their non-sleepable counterparts (e.g., "fentry.s/" before "fentry/", "fexit.s/" before "fexit/", "fmod_ret.s/" before "fmod_ret/", "iter.s/" before "iter/", "tp_btf.s/" before "tp_btf/", and "struct_ops.s/" before "struct_ops/"), or otherwise ensure matching uses longest-prefix-first; this fixes the section.find(prefix) == 0 logic and allows the subsequent is_sleepable check (the ".s/" substring detection) to work correctly.
🤖 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 `@src/linux/linux_platform.cpp`:
- Around line 161-163: The prefix list for tracing in the PTYPE initializer
causes sleepable variants like "fentry.s/" to be shadowed by shorter prefixes
("fentry/"), so update the prefixes passed to PTYPE("tracing", &g_tracing_descr,
...) so that all ".s/" (sleepable) variants appear before their non-sleepable
counterparts (e.g., "fentry.s/" before "fentry/", "fexit.s/" before "fexit/",
"fmod_ret.s/" before "fmod_ret/", "iter.s/" before "iter/", "tp_btf.s/" before
"tp_btf/", and "struct_ops.s/" before "struct_ops/"), or otherwise ensure
matching uses longest-prefix-first; this fixes the section.find(prefix) == 0
logic and allows the subsequent is_sleepable check (the ".s/" substring
detection) to work correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a307f9ea-e67b-4de8-b2b9-3bbaa96b3160
📒 Files selected for processing (1)
src/linux/linux_platform.cpp
|
Regarding the CodeRabbit "prefix ordering breaks sleepable detection" comment: this is a false positive. |
Add might_sleep flag to EbpfHelperPrototype and is_sleepable flag to EbpfProgramType. Helpers marked might_sleep are rejected in non-sleepable programs via is_helper_usable. Sleepable programs are detected by matching section prefixes ending in .s/ (e.g., fentry.s/, lsm.s/). Added .s/ variants for tracing and struct_ops program types. Marked 5 helpers as might_sleep: copy_from_user, copy_from_user_task, d_path, ima_inode_hash, ima_file_hash. Closes #1133 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
Syscall programs always run in sleepable context, so they should be able to call might_sleep helpers regardless of section suffix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
a97a487 to
44aaa29
Compare
Summary
might_sleepflag toEbpfHelperPrototype— marks helpers that may sleepis_sleepableflag toEbpfProgramType— set when the section prefix contains.s/is_helper_usable:might_sleephelpers are rejected in non-sleepable programs.s/section prefix variants for tracing (fentry.s/,fexit.s/,fmod_ret.s/,iter.s/,tp_btf.s/) andstruct_ops.s/might_sleep:copy_from_user,copy_from_user_task,d_path,ima_inode_hash,ima_file_hashTest plan
Closes #1133
Ref: #68
🤖 Generated with Claude Code