Fix process and path targeting in disk failure injector#1095
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0112ff4 | Docs | Datadog PR Page | Give us feedback! |
2ef0bd5 to
1cc3b78
Compare
39459fe to
59a7ed6
Compare
Fixes several bugs in eBPF-based disk failure injection: - fmod_ret chaining and cwd index mask were broken, causing incorrect path resolution for relative paths - Separate chain size from depth to correctly compute the cwd index mask - Reject dotdot paths and use active PID namespace in disk failure eBPF - Reject pod-level disk failure on shared host PID namespace - Cap path writes and reject shared pod namespaces to prevent cross- container interference - Fail closed on truncated cwd depth and long relative paths - Fail closed when mount-crossing loop exhausts its budget - Remove stale CONFIG_BPF_KPROBE_OVERRIDE validation - Require CONFIG_FUNCTION_ERROR_INJECTION for fmod_ret - Root filter was disrupting all relative paths; fix cgroup comparison - Normalize cgroup scope to avoid false shared-namespace rejections - Check root filter before rejecting dirfd-relative opens - Skip root cgroup paths in container identity comparison Jira: CHAOSPLT-DISK
81a8614 to
5bfc170
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bfc170ec2
ℹ️ 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".
The CONFIG_FUNCTION_ERROR_INJECTION check was placed in GetRequiredSystemConfig(), which is shared by all eBPF users including network disruptions (TC classifier). Kernels supporting TC-based eBPF but lacking fmod_ret support would be incorrectly rejected for network disruptions. Move the check into a new ValidateDiskFailureRequiredConfig() method on ConfigInformer, called only from the disk failure injector after the common ValidateRequiredSystemConfig() check. Update tests and mock accordingly, and refresh docs/disk_failure.md to reflect the current fmod_ret hook and PID namespace targeting.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Motivation
The disk failure injector mis-targeted both processes and paths, causing it to either fail to disrupt the intended opens or disrupt far more than the targeted path:
ppid == target_pid), missing grandchildren deeper in the process tree.filter_pathprefix.openat()also accepts relative paths, which skipped the filter entirely and were disrupted unconditionally — failing nearly every open of the targeted process regardless of-path.fmod_retprogram read its argument viaPT_REGS_PARM1(offset 112), which the verifier rejects forfmod_ret, causing a permission-denied panic on load.Changes
Process targeting — PID namespace inode filtering
target_pid+ parent traversal; addtarget_pid_ns_inumvolatile read viaBPF_CORE_READchaintask→nsproxy→pid_ns_for_children→ns.inum. Every process in a container shares the same PID namespace inode, so matching on it catches the whole process tree regardless of depth.-processflag with-pid-ns-inum.syscall.Statat injection time; node-level disruptions passinum=0(match all).Path targeting — relative path resolution
AT_FDCWD-relative paths to an absolute path by walking the task'spwddentry chain across mount points (liked_path) into a per-CPU scratch buffer, then apply the same prefix filter used for absolute paths.Verifier fixes
BPF_PROGmacro for thefmod_retprogram so theopenatargument is read from BTF offset 0 instead ofPT_REGS_PARM1.CONFIG_FUNCTION_ERROR_INJECTIONforfmod_ret; remove staleCONFIG_BPF_KPROBE_OVERRIDEvalidation.bpf_probe_read_kernel_strat a masked offset, and cap cwd depth to keep verifier state from exploding (E2BIG).QA Instructions
fmod_retprogram loads without theinvalid bpf_context access off=112verifier error.-path /some/dirdisrupts absolute opens under that prefix and relative opens that resolve under it, while leaving unrelated paths untouched.Blast Radius
Limited to the eBPF disk failure injector (
ebpf/disk-failure/,injector/disk_failure.go). No changes to other disruption kinds or the controller reconciliation path.Documentation