feat(tracepoint): add sched_process_exec tracepoint - #2154
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b1c50988b
ℹ️ 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".
| TP_system(sched), | ||
| TP_PROTO(comm: &str, pid: i32, old_pid: i32), | ||
| TP_STRUCT__entry { | ||
| comm: [u8; 16], |
There was a problem hiding this comment.
这里把事件 payload 定义成 comm 会让 /tracing/events/sched/sched_process_exec/format 暴露为 comm,pid,old_pid，而 Linux 6.6 的同名 tracepoint ABI 是 filename,pid,old_pid（filename 还是第一项，见 https://codebrowser.dev/linux/linux/include/trace/events/sched.h.html#431）。因此按 Linux format 编译或按 filename 过滤/解析的 eBPF/trace 工具会在 DragonOS 上找不到字段，raw offset 也会读错，恰好破坏本次为 agentsight 兼容该 tracepoint 的目的；即使暂时不能实现动态 __string，也应保留 filename 字段语义/名称并从 exec 参数填充。
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. Keeping comm as the first field is an intentional trade-off: DragonOS define_event_trace! does not support Linux __string (the dynamic string type filename requires), so the payload uses a fixed [u8;16] comm buffer filled from the exec basename instead. The field is documented as such in trace.rs. Switching to a filename field would either need __string support in the macro (a framework change) or a fixed-size filename buffer with a different semantic. Flagging for maintainer decision on whether to track the __string framework work separately; leaving this open for now.
|
感谢这两条 review，已逐条处理： ① trace.rs:14 — 保持 filename ABI（不采纳） 经 bug-hunter 多角色 fanout 审查（ABI 维度）独立确认：
因此「保留 ② execve.rs:226 — 先释放 basic 读锁再触发 tracepoint（采纳，已修复） 确认是真问题。 修复（commit 附带：fanout 的 EdgeCases 维度还发现我修锁时引入的一个回归——comm 截断在原始字节偏移，多字节 UTF-8 路径名（如 |
为 execve 成功路径添加 sched_process_exec tracepoint，供 ANOLISA agentsight 的 eBPF 程序追踪进程 exec 事件、构建 AI agent 进程树。 - 新增 kernel/src/process/trace.rs：声明 sched_process_exec tracepoint（comm/pid/old_pid 字段），TP_system(sched) - execve.rs：在 load_binary_file_with_context 之前捕获 old_pid（de_thread 会交换 pid）；trace 调用置于 arch_do_execve 成功后、is_ok() 分支内，对齐 Linux fs/exec.c:1803 - mod.rs：注册 process::trace 模块 tracepoint 注册、debugfs 导出、eBPF attach 全部由现有框架自动完成。static key 保证未启用时零开销。 Refs: DragonOS-Community#2149 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
新增 sched_tracepoint dunitest，验证 sched_process_exec tracepoint 的 debugfs 导出（format/enable/id 文件 + 字段）与 execve 触发行为。
- EventFilesExist: 验证 events/sched/sched_process_exec/{format,id,enable} 存在，format 含 comm/pid/old_pid/common_pid 字段
- FiresOnExecve: enable + 清空 trace 后 fork+execve(/proc/self/exe)，断言 trace 含 sched_process_exec 记录与 comm 字段
- whitelist 注册 normal/sched_tracepoint
Refs: DragonOS-Community#2149
Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
TracePointIdFile 的 id 由 global_init_events 的 AtomicUsize::new(0) fetch_add 分配，从 0 开始递增。原断言 EXPECT_GT(idval, 0) 错误假设 id 从 1 开始，导致 CI 中拿到 id=0 的 tracepoint 失败。改为 EXPECT_GE(idval, 0)。 Refs: DragonOS-Community#2149 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
…exec PR review 发现：trace_sched_process_exec(pcb.basic().name(), ...) 持有 basic 读锁 guard 直到 trace 调用返回，而 trace 默认回调内部的 trace_cmdline_push() 会再次获取同一把 basic 读锁。DragonOS RwLock 读锁不可重入，若此时另一 CPU 排队写锁，将导致 reader 等 writer、writer 等 reader 的 deadlock。 修复：先将 comm 复制到栈缓冲并在内层作用域释放读锁 guard，再调用 trace，消除锁重入。 Refs: DragonOS-Community#2149 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
bug-hunter fanout 发现：comm_len = bytes.len().min(15) 在原始字节偏移截断，当进程名第 15 字节落在多字节 UTF-8 字符中间时（如 Unicode 路径名），from_utf8 失败导致 comm 变空字符串。用 is_char_boundary 回退到字符边界修复。 同时加强测试：strtol 解析 id 后校验 end 指针，确认整个字符串都是数字而非仅前缀可解析。 Refs: DragonOS-Community#2149 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
make fmt / clippy: name.as_bytes().len() → name.len()（字符串可直接调 len()）。 Refs: DragonOS-Community#2149 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
5721983 to
441113f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 441113f3f4
ℹ️ 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".
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Requesting changes because the PR still does not implement the Linux sched_process_exec contract and introduces additional ordering, hot-path, security, and test-lifecycle problems.
The existing discussion on kernel/src/process/trace.rs:14 remains blocking: Linux 6.6 exposes __data_loc char[] filename, pid, and old_pid, while this PR publishes [u8; 16] comm, pid, and old_pid. This changes the raw-record layout, field offsets, and semantics. A program compiled against Linux's struct trace_event_raw_sched_process_exec will interpret the first four bytes of comm as a data-location descriptor and read the integer fields at the wrong offsets. The current macro's lack of dynamic-string support is an implementation limitation, not a valid reason to publish an incompatible ABI under a Linux event name. Please add reusable __data_loc/dynamic-field support first, or use a clearly DragonOS-specific event rather than redefining the Linux one.
The product requirement should also pin the AgentSight revision being supported. Current upstream ANOLISA proctrace.bpf.c and procmon.bpf.c use syscall execve tracepoints rather than sched_process_exec; if this PR is instead intended as Linux compatibility work, exact Linux ABI and timing are the acceptance criteria.
The inline comments below identify the remaining actionable blockers. The architectural fixes belong in the common tracepoint/perf layers where noted; event-local workarounds or tests that only validate the custom text format are not sufficient.
| comm_buf[..comm_len].copy_from_slice(&name.as_bytes()[..comm_len]); | ||
| } | ||
| let comm = core::str::from_utf8(&comm_buf[..comm_len]).unwrap_or(""); | ||
| trace_sched_process_exec(comm, pid, old_pid); |
There was a problem hiding this comment.
[P1] Establish a tracing permission boundary before exposing global exec metadata
This new source publishes every successful exec into the global tracepoint framework, but DragonOS's PERF_TYPE_TRACEPOINT path currently has no Linux-equivalent perf_event_paranoid, CAP_PERFMON/CAP_SYS_ADMIN, target-task access, or pid/cpu enforcement; TracepointPerfEvent stores but ignores the requested pid/cpu, and the shared trace files are world-readable once debugfs is mounted. This lets an unprivileged consumer enable and observe cross-user/cross-namespace exec metadata and potentially disable another consumer's event. The fix should be centralized in the perf/tracefs entry points before adding this data source, with capability, target visibility, namespace, and pid/cpu checks; hiding only this event would be a workaround.
There was a problem hiding this comment.
Agreed this is a real gap, but it is framework-wide rather than specific to sched_process_exec: DragonOS PERF_TYPE_TRACEPOINT currently lacks perf_event_paranoid / CAP_PERFMON|CAP_SYS_ADMIN enforcement, target-task visibility, and pid/cpu checks across all tracepoints, and the shared trace files are world-readable once debugfs is mounted. Centralizing the permission boundary in the perf/tracefs entry points is the right fix, but it touches every tracepoint/perf consumer and is out of scope for this single-event PR. Propose tracking it as a separate issue/PR so this event does not block on a framework-wide security rewrite. Leaving open for maintainer confirmation on scoping.
| << trace; | ||
|
|
||
| // 关闭事件并清理。 | ||
| write_file(enable_path, "0"); |
There was a problem hiding this comment.
[P1] Do not revoke another tracing consumer's enable state
Writing 0 here unconditionally resets a global boolean static key; it does not release a test-owned enable reference. If a perf fd or another debugfs user had already enabled this tracepoint, the test disables that consumer's event. The earlier assertion that the initial value must be 0 has the same isolation problem. TracePoint needs per-consumer enable ownership/reference counting (switching the static key only on 0→1 and 1→0), and this test should hold and release only its own token rather than overwrite global state.
There was a problem hiding this comment.
Agreed the test unconditionally writes the global static key. Note this mirrors Linux tracefs semantics: events///enable is itself a global boolean (writing 0 disables the static key for all consumers), and per-consumer enable ownership lives at the perf-event layer, not in the tracefs file. So the test behavior is Linux-compatible for the tracefs path. Per-consumer reference counting on the TracePoint itself (switching the static key only on 0->1 / 1->0 across perf fds + tracefs) would be a framework-level change affecting all tracepoints. Propose tracking it separately rather than in this single-event PR. Leaving open for maintainer scoping decision.
Add 3 targeted cases to sched_tracepoint.cc: - DefaultDisabledNoRecords: verify zero records when disabled (static-key gate / zero-overhead guarantee) - DisableStopsFiring: verify enable/disable state machine toggles the static-key and stops firing after disable - NonLeaderExecFiresWithDistinctOldPid: cover the de_thread raw_pid swap path with a multi-threaded non-leader execve, asserting old_pid != pid (FiresOnExecve only exercised single-threaded leader exec where old_pid == pid) All 5 cases pass on QEMU x86_64. Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
Address two P1 review items from fslongjin: - Thread: trace_sched_process_exec now runs after vfork_done complete_all(), not before. The prior order blocked the vfork parent behind every tracepoint callback after the child committed exec; Linux completes vfork from exec_mmap()/exec_mm_release() before trace_sched_process_exec() runs in the successful exec tail. - Thread: add a trace_<name>_enabled() read-only guard to define_event_trace! (GenericStaticKey::is_enabled, Relaxed atomic) and gate the whole comm field construction (basic read lock, UTF-8 boundary scan, copy) behind it in do_execve_internal. Rust evaluates call arguments before entering the static-key branch inside trace_<name>(), so the disabled path previously took the irqsave basic lock and scanned/copied the name on every exec. Now disabled execs pay zero trace overhead. Purely additive: all 12 existing tracepoints gain the guard, none change behavior. sched_tracepoint dunitest still passes 5/5 on QEMU x86_64. Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
Address one P2 and one P1 review item from fslongjin: - mount_debugfs returns AssertionResult now; callers use ASSERT_TRUE so a mount failure terminates the calling test instead of cascading against an ordinary directory. - Add a DebugfsMount RAII guard acquired right after mount (before enable) and armed after a successful enable write. Its destructor restores pre-test state on every exit path: disable (if armed) -> umount -> rmdir. Any mid-test ASSERT return can no longer leak a globally-enabled static key, pollute the shared ring buffer, or leak the mount point into later tests. Assertion semantics unchanged; 5/5 pass on QEMU x86_64. Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
2 participants
为 execve 成功路径添加
sched_process_exectracepoint，供 ANOLISA agentsight 的 eBPF 程序追踪进程 exec 事件、构建 AI agent 进程树。变更内容
kernel/src/process/trace.rs：声明sched_process_exectracepoint（comm/pid/old_pid字段），TP_system(sched)。字段参考 Linuxinclude/trace/events/sched.h。kernel/src/process/execve.rs：在load_binary_file_with_context之前捕获old_pid（de_thread会交换 pid）；trace 调用置于arch_do_execve成功后、is_ok()分支内，对齐 Linuxfs/exec.cexec_binprm()中trace_sched_process_exec的调用位置。kernel/src/process/mod.rs：注册process::trace模块。设计说明
define_event_trace!宏不支持 Linux 的__string动态字符串，且当前未实现bpf_get_current_comm()helper，故comm直接放入 payload（[u8; 16]，对齐TASK_COMM_LEN）。TP_printk在首个 NUL 字节处截断 comm，使用 Display 格式输出，输出格式对齐 Linux 的comm=... pid=... old_pid=...。Refs: #2149