fix(starry): snapshot thread context in file syscalls#885
Conversation
There was a problem hiding this comment.
Review: fix(starry): snapshot thread context in file syscalls
变更概述
本 PR 在 sys_openat、sys_mkdirat、sys_mknodat 三个文件创建类 syscall 入口处,在 vm_load_string() 调用之前固定 current() / as_thread(),确保后续 umask() 和 cred() 读取使用 syscall 入口调用者的线程上下文。同时新增 qemu-smp4/test-openat-umask-smp 压力测试。
实现逻辑分析
根因正确性:vm_load_string() 内部通过 vm_load_until_nul() → access_user_memory() → user_copy() 访问用户空间,可能触发 page fault。在 SMP 可抢占环境中,page fault 路径可能导致当前任务被调度出去,之后恢复执行时 per-CPU current() 可能返回不同的 task(例如 kernel thread),导致 current().as_thread() panic。在 vm_load_string() 之前快照线程上下文,后续使用快照值而非重新读取 current(),语义上完全正确——文件创建的 umask/credential 本就应取 syscall 入口调用者属性。
修复范围:PR 聚焦于实际观测到 crash 的三个文件创建 syscall(fd_ops.rs:269 panic 路径),修复精准、最小化。
测试覆盖:新增的 test-openat-umask-smp 使用 CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND 创建 8 个共享地址空间和文件描述符表的 worker 线程,并发执行 umask() + openat(O_CREAT) 共 2400 次。测试直接命中修复所保护的并发路径,且有 stall 检测(100 次 poll 无进展则 FAIL)和完整 waitpid 校验。success_regex / fail_regex 设计合理,PASS 行需精确匹配数字,不会误匹配。
验证结果
cargo fmt --all --check:通过 ✅git diff --check origin/dev...HEAD:通过 ✅,无空白错误- 代码审阅:三处修改均为在
vm_load_string()前插入两行let curr/thread = ...,最小化变更,逻辑清晰 - 测试 C 代码
-Wall -Wextra -Werror编译选项正确
重复/重叠分析
- base 分支:
origin/dev上这三个 syscall 仍使用vm_load_string()之后二次current().as_thread()的模式,修复未存在于 base。 - 相关 open PR:搜索了全部 open PR(#886 eBPF userspace、#741 busybox_crond 等),无任何 PR 涉及 syscall 线程上下文快照或 openat/umask 并发修复。本 PR 无重复。
后续建议(非阻塞)
- 同类模式扩展:
dev分支上sys_fchownat(ctl.rs:460)、sys_fchmodat(ctl.rs:559)、sys_fstatat(stat.rs:166)等也存在vm_load_string()之后调用current().as_thread().cred()的同类模式。虽然当前 PR 聚焦于已观测到的 crash 路径是合理的,建议后续 PR 对这些位点做相同的入口快照处理。 - 架构覆盖:当前测试仅提供
qemu-riscv64.toml,bug 本身是架构无关的,如 x86_64 SMP 也可能触发可考虑补充。
总结
修复逻辑正确、范围精准、测试充分、无重复。APPROVE。
Powered by glm-5.1
|
|
||
| pub fn sys_mkdirat(dirfd: i32, path: *const c_char, mode: u32) -> AxResult<isize> { | ||
| let curr = current(); | ||
| let thread = curr.as_thread(); |
There was a problem hiding this comment.
建议后续 PR 同步处理 sys_fchownat(ctl.rs:460 let cred = current().as_thread().cred())和 sys_fchmodat 等同类位点——它们也在 vm_load_string() / resolve_at() 之后二次读取 current()。当前 PR 聚焦已观测 crash 路径是合理的。
| shell_prefix = "root@starry:" | ||
| shell_init_cmd = "/usr/bin/test-openat-umask-smp" | ||
| success_regex = ['(?m)^PASS: openat_umask_smp workers=\d+ iterations=\d+\s*$'] | ||
| fail_regex = ['(?i)\bpanic(?:ked)?\b', '(?m)^FAIL:'] |
There was a problem hiding this comment.
success_regex 要求 workers=\d+ iterations=\d+ 精确匹配,fail_regex 同时覆盖 panic 和 FAIL: 前缀,设计清晰。180s timeout 对 8×300 次 openat+write+close+unlink 循环在 QEMU TCG 下留有充足余量。
Summary
openat/mkdirat/mknodat入口固定当前 StarryOS 用户线程。vm_load_string()之后的umask/cred读取统一使用同一个入口线程,不再二次依赖 per-CPUcurrent()。qemu-smp4/test-openat-umask-smp,用多个CLONE_THREADworker 并发执行openat(O_CREAT)和umask压力路径。Root Cause
文件创建类 syscall 先从用户地址读取 pathname,然后才重新调用
current().as_thread()获取umask/ credential。这个路径在 SMP/preempt 和用户内存访问压力下不够稳:如果 usercopy/page-fault 或调度路径之后二次读取到的 current task 不是 Starry 用户线程,就会触发kernel taskpanic。这个问题是在 M6 多核 cargo 压力路径中暴露的,日志里看到
fd_ops.rs:269的current().as_thread()panic。需要注意,RISC-V QEMU MTTCG 本身存在 LR/SC 原子语义风险,所以该日志只能作为高压发现信号,不能单独当作正常硬件上的 correctness 证明。本 PR 的修复点是更保守的 syscall hardening:文件创建语义本就应该使用 syscall 入口调用者的线程上下文。Fix
sys_openat在入口保存curr/thread,后续创建 mode 使用thread.proc_data.umask(),credential 使用thread.cred()。sys_mkdirat/sys_mknodat同样在入口保存thread,后续 mode/perm 的 umask 计算使用同一个线程。qemu-smp4下并发创建文件,覆盖多线程共享umask+openat(O_CREAT)路径,确保不会因为上下文读取不一致而 panic 或停滞。Test plan
git diff --checkcargo fmt --all --checkcargo check -p starry-kernel --target riscv64gc-unknown-none-elfzig cc -target riscv64-linux-musl -Wall -Wextra -Werror -c test-suit/starryos/normal/qemu-smp4/test-openat-umask-smp/c/src/main.c -o /tmp/test-openat-umask-smp.ocargo xtask starry test qemu --arch riscv64 -g normal -c test-openat-umask-smp --list本地 macOS 缺
debugfs,Docker qemu 运行尝试中 Docker CLI 无输出卡住,因此先作为 draft PR 交给 GitHub Actions 容器环境跑完整 qemu case。