fix(ipc): 修复部分 shm_test 错误的测试用例

[mistcoversmyeyes]

主要改动

• 克隆地址空间时，正确增加 KernelShm 的 map_count
• 补充 ShmFlags::PERM_MASK 用于获取 POSIX 权限位
  • 该掩码用于屏蔽掉非权限位的一次性标志，如 IPC_CREAT、IPC_EXCL 等。
• 使 KernelShm 能正确创建并初始化
  • 正确初始化 uid/gid/cuid/cgid，并使用 PERM_MASK 正确获取权限位。
  • shm_lpid 初始化为 0（因为 lpid 只记录最后一个执行 attach/detach 操作的进程 ID，而在创建时没有这样的进程）。
• 使 shm_stat 能够正确获取共享内存状态信息
  • 使 shm_stat 正确获取 shm_perm.mode。
  • 使 shm_stat 将 shm_{a,d,c}time 以秒为单位输出，
• 完善 sys_shmget 创建一个 key 已经存在的共享内存段时的错误处理
  • 添加了对 size 参数的检查，确保如果 size 大于已存在段的大小，则返回错误。

[mistcoversmyeyes]

在合入之前需要清理多余的提交

[fslongjin]

skills的相关的commit要从这个pr里面drop掉

[fslongjin]

@claude review

[dragonos-community-ai-pr-reviewer]

Claude finished @fslongjin's task in 6m 12s —— View job

---

Claude's Review of PR #1674

I've reviewed the code changes in this PR. Here's my analysis:

Todo List:

• Create initial GitHub comment with todo list
• Check for CLAUDE.md file for repo-specific instructions
• Examine PR changes and commits
• Review code changes for bugs, security issues, performance
• Check for extra commits that need to be dropped
• Provide comprehensive review feedback

Summary of Changes

This PR fixes SysV shared memory test cases (ShmTest) by aligning implementation with Linux behavior. The changes include:

1. kernel/src/ipc/shm.rs (+61/-20):

   • Add PERM_MASK constant for permission bits
   • Populate uid/gid/cuid/cgid from current process credentials
   • Implement detach_shm() to handle destruction when map_count == 0 and SHM_DEST flag is set
   • Report timestamps in seconds (tv_sec) instead of nanoseconds
   • Initialize shm_lprid to 0 (matching Linux)
   • Fix permission bit handling in copy_from() to only copy permission bits
   • Remove unsafe frame allocator call in ipc_rmid()

2. kernel/src/ipc/syscall/sys_shmat.rs (+1/-0):

   • Store attachment base address (shm_attach) in VMA to identify VMA fragments from same shmat call

3. kernel/src/ipc/syscall/sys_shmdt.rs (+38/-4):

   • Track all VMAs with same shm_id and shm_attach and unmap them together
   • Call detach_shm() after unmapping to update map_count and handle destruction
   • Validate shm_id and shm_attach before unmapping

4. kernel/src/ipc/syscall/sys_shmget.rs (+7/-0):

   • Reject shmget with size larger than existing segment (matching Linux shm_more_checks)

5. kernel/src/mm/syscall/mod.rs (+1/-1):

   • Initialize shmat VM flags with VM_SHARED | VM_MAYSHARE by default

6. kernel/src/mm/ucontext.rs (+47/-27):

   • Increase map_count when forking SHM mappings
   • Handle SHM detachment in unmap_all() when process exits
   • Remove duplicate SHM cleanup in VMA unmapping
   • Add shm_attach field to VMA struct

7. config/app-blocklist.toml (+1/-1):

   • Uncomment gvisor syscall tests line (unrelated to SHM fixes)

Code Review Feedback

Positive Aspects

• The changes align well with Linux SysV shared memory semantics
• Proper credential handling (uid/gid/cuid/cgid) improves security
• The shm_attach tracking correctly handles VMA splitting scenarios
• Test coverage improvements mentioned in commit messages are promising

Potential Issues to Verify

1. Map count management in fork (kernel/src/mm/ucontext.rs:318):

   • The increase_count() is called when forking SHM mappings, which is correct
   • Need to ensure map_count is increased only once per attachment, not per VMA fragment (already handled via shm_attach grouping)

2. Double detachment risk:

   • detach_shm() decreases map_count and may destroy SHM segment
   • Need to ensure detach_shm() isn't called multiple times for the same attachment (e.g., via both explicit shmdt and process exit)
   • The shm_attachments set in unmap_all() should prevent duplicate calls for same (shm_id, attach) pair

3. Error handling:

   • detach_shm() returns Result but callers in sys_shmdt.rs:94 and ucontext.rs:1218 use ? - good
   • Consider adding log messages for destruction events for debugging

4. Race conditions:

   • SHM operations use locks (shm.lock(), page_manager_lock()), which should prevent races
   • Verify lock ordering doesn't cause deadlocks (SHM lock before page manager lock appears consistent)

Suggestions

1. Add comments for the shm_attach field explaining its purpose: "Base address of the original shmat call, used to group VMA fragments"
2. Consider adding assertions for map_count invariants (e.g., map_count >= 0)
3. Test edge cases: partial unmaps, multiple attachments to same segment, fork/exit scenarios

Extra Commits That Should Be Dropped

The PR currently includes 5 unrelated commits that should be removed before merging:

1. 097797f - chore:启用 gvisor 集成 (uncomments gvisor tests in blocklist)
2. d7eecdf - feat(sysfs): sysfs 支持挂载 (#1667) (sysfs mount feature)
3. 1c0948f - feat: 添加IO性能测试工具test_ioperf (#1666) (IO performance test tool)
4. e7add27 - feat(procfs): procfs支持挂载 (#1639) (procfs mount feature)
5. 39c95d5 - fix(mm): clamp file page writeback length to one page (#1662) (writeback fix)

The formatting commit (ca5cd84) only touches SHM-related files and can stay.

Recommendation

Approve with changes: The SHM fixes look correct and improve Linux compatibility. However, the unrelated commits must be dropped first.

If you'd like me to help clean up the branch by dropping these extra commits, please let me know.

[fslongjin]

@claude review

[dragonos-community-ai-pr-reviewer]

Claude finished @fslongjin's task in 4m 48s —— View job

---

Claude's Review of PR #1674

I've reviewed the current state of this PR. Here's my analysis:

Summary of Changes

This PR fixes SysV shared memory test cases (ShmTest) by aligning implementation with Linux behavior. The main changes include:

1. kernel/src/ipc/shm.rs:

   • Add PERM_MASK constant for permission bits
   • Populate uid/gid/cuid/cgid from current process credentials
   • Initialize shm_lprid to 0 (matching Linux)
   • Report timestamps in seconds (tv_sec) instead of nanoseconds
   • Fix permission bit handling in copy_from() to only copy permission bits

2. kernel/src/ipc/syscall/sys_shmget.rs:

   • Reject shmget with size larger than existing segment (matching Linux shm_more_checks)

3. kernel/src/mm/syscall/mod.rs:

   • Initialize shmat VM flags with VM_SHARED | VM_MAYSHARE by default

4. kernel/src/mm/ucontext.rs:

   • Increase map_count when forking SHM mappings
   • Handle SHM detachment in unmap_all() when process exits
   • Remove duplicate SHM cleanup in VMA unmapping

Code Review Feedback

Positive Aspects

• The changes align well with Linux SysV shared memory semantics
• Proper credential handling (uid/gid/cuid/cgid) improves security
• Permission bit masking (PERM_MASK) correctly separates permission bits from control flags
• Timestamp reporting in seconds matches POSIX expectations

Issues Found

1. Missing comment for shm_lprid initialization (kernel/src/ipc/shm.rs:415):

   • The code initializes shm_lprid to 0, which matches Linux behavior (newseg sets shm_lprid = NULL)
   • However, there's no comment explaining why it's 0, as requested in the review discussion

2. Potential map_count management issue:

   • In ucontext.rs, increase_count() is called when forking SHM mappings (line 321)
   • Need to ensure map_count is increased only once per attachment, not per VMA fragment
   • The shm_attach grouping mechanism (mentioned in PR description) should prevent this, but verify

3. Extra commits not related to SHM fixes:

   • The branch contains multiple unrelated commits that must be dropped before merging
   • These include sysfs, procfs, IO performance tools, and other features

Suggestions

1. Add explanatory comment for shm_lprid initialization:

   shm_lprid: RawPid::new(0), // Linux newseg() initializes shm_lprid = NULL

2. Consider adding a convenience constructor for KernelShm:

   • As suggested by @fslongjin, the creation and credential filling could be encapsulated
   • This would simplify ShmManager::add() and improve code organization

3. Add assertions for map_count invariants:

   • Consider adding debug_assert!(self.map_count > 0) before decrease_count()
   • This would catch potential double-decrement bugs during development

Review Comments from fslongjin Addressed

1. config/app-blocklist.toml: ✅ Should be reverted when unrelated commits are dropped

   • The "gvisor syscall tests" line was uncommented in an unrelated commit
   • Dropping that commit will restore the commented state

2. KernelShm constructor encapsulation: ⚠️ Partially addressed

   • Current code creates KernIpcPerm in ShmManager::add() and passes to KernelShm::new()
   • Consider adding a KernelShm::new_with_key() that handles credential collection

3. shm_lprid initialization: ⚠️ Needs comment

   • Code correctly initializes to 0 (matching Linux)
   • Missing explanatory comment as promised by @mistcoversmyeyes

Extra Commits That Must Be Dropped

The PR branch currently includes 11 unrelated commits that should be removed:

1. d7eecdf - feat(sysfs): sysfs 支持挂载 (#1667)
2. 1c0948f - feat: 添加IO性能测试工具test_ioperf (#1666)
3. e7add27 - feat(procfs): procfs支持挂载 (#1639)
4. 39c95d5 - fix(mm): clamp file page writeback length to one page (#1662)
5. a3cbe90 - feat(block): 新增异步BIO框架并集成到virtio-blk驱动 (#1659)
6. dbb5a83 - chore(nix/qemu): 添加动态端口分配和VM状态目录配置 (#1660)
7. 7272045 - fix(net): start iface id at 1 to match Linux ifindex semantics (#1658)
8. 4d7478c - refactor(futex): 重构优先级继承 futex 实现 (#1656)
9. eda2e5d - fix(process): refine process exit state management and signal handling (#1657)
10. b8adc89 - fix(rwsem): 修复读写信号量升级过程中的唤醒和状态管理问题 (#1654)
11. 8463318 - refactor(mm): 将页面锁从读写锁替换为读写信号量 (#1653)

The formatting commit (e4e4a36) only touches SHM-related files and can stay.

Recommendation

Request changes: The SHM fixes are technically correct and improve Linux compatibility. However:

1. The PR must be cleaned up by dropping all unrelated commits
2. Add the missing comment for shm_lprid initialization
3. Consider the constructor refactoring suggestion

Once these issues are addressed, the PR will be ready for merging.

If you'd like me to help clean up the branch by dropping the extra commits, please let me know.

[mistcoversmyeyes]

我摘出来可用的提交以后，本地跑了一下全部启用的 gvisor test， 都能够通过，不知道为啥 CI 卡住了。刚刚我又手动跑了一下卡住的那个 signalfd_test，本地是可以正常通过的。

[fslongjin]

PR 审查报告：#1674

概览

PR 标题： fix(ipc): 修复部分 shm_test 错误的测试用例
修改文件： 4 个文件

• kernel/src/ipc/shm.rs
• kernel/src/ipc/syscall/sys_shmget.rs
• kernel/src/mm/syscall/mod.rs
• kernel/src/mm/ucontext.rs

代码变更： +203 行添加，-89 行删除

PR 目标： 修复 DragonOS 共享内存实现中的多个 POSIX 合规性问题，使 gvisor 的 shm_test 测试用例通过。

---

🚨 严重问题（合并前必须修复）

1. unwrap() 调用导致的内核崩溃风险

严重程度： 严重
影响范围： 拒绝服务攻击、内核崩溃

问题位置：

• kernel/src/ipc/shm.rs:156 - ID 分配
• kernel/src/ipc/shm.rs:167, 319 - 页数转换
• kernel/src/ipc/shm.rs:329 - 页管理器查找
• kernel/src/mm/ucontext.rs:1011 - VMA 提取

问题描述：

多处使用 .unwrap() 和 .expect() 调用，在错误发生时会直接导致整个内核崩溃，而不是向用户空间返回错误。恶意或有缺陷的程序可以通过触发这些 panic 造成拒绝服务攻击（例如：耗尽 IPC ID、请求接近 usize::MAX 的超大内存大小）。

当前代码示例：

// kernel/src/ipc/shm.rs:156
let id = self.id_allocator.alloc().expect("No more id to allocate.");

// kernel/src/ipc/shm.rs:167
let page_count = PageFrameCount::from_bytes(page_align_up(size)).unwrap();

// kernel/src/mm/ucontext.rs:1011
let split_result = cur_vma
    .extract(intersection, &self.user_mapper.utable)
    .unwrap();

修复方案：

// 使用 ok_or 替代 expect/unwrap
let id = self.id_allocator.alloc().ok_or(SystemError::ENOSPC)?;

let page_count = PageFrameCount::from_bytes(page_align_up(size))
    .ok_or(SystemError::EINVAL)?;

let split_result = cur_vma
    .extract(intersection, &self.user_mapper.utable)
    .ok_or(SystemError::EFAULT)?;

风险场景：

• 攻击者创建 4096 个共享内存段（达到 SHMMNI 限制）
• 第 4097 次 shmget 调用会导致整个内核 panic
• 结果：系统崩溃，所有进程终止

---

2. try_clone (fork) 中的竞态条件

严重程度： 高
影响范围： Use-after-free、内存损坏、内核崩溃

问题位置： kernel/src/mm/ucontext.rs:316-323

问题描述：

在释放 vma_guard（第 261 行）和获取 shm_manager_guard（第 318 行）之间，另一个线程可能通过 IPC_RMID 删除共享内存段。这创建了 TOCTOU（检查时-使用时）竞态条件，子进程会得到一个指向不存在段的 VMA。

当前代码：

// 第 261 行：释放 vma_guard
drop(vma_guard);

// 第 316-323 行：竞态窗口！
if let Some(shm_id) = shm_id {
    let ipcns = ProcessManager::current_ipcns();
    let mut shm_manager_guard = ipcns.shm.lock();
    if let Some(kernel_shm) = shm_manager_guard.get_mut(&shm_id) {
        // Forked SHM mappings count as new attachments.
        kernel_shm.increase_count();
    }
    // 如果 get_mut 返回 None，代码静默继续！
}

修复方案：

if let Some(shm_id) = shm_id {
    let ipcns = ProcessManager::current_ipcns();
    let mut shm_manager_guard = ipcns.shm.lock();

    match shm_manager_guard.get_mut(&shm_id) {
        Some(kernel_shm) => {
            kernel_shm.increase_count();
        }
        None => {
            // SHM 段在 fork 期间被删除
            warn!("Fork: SHM segment {:?} no longer exists, marking VMA as invalid", shm_id);
            // 标记 VMA 为无效或进行适当处理
            // new_vma.lock().shm_id = None;
        }
    }
}

攻击场景：

进程 A：fork() 开始
进程 B：IPC_RMID 删除共享内存段（map_count 降为 0）
进程 A：尝试增加 map_count → 段已不存在
结果：子进程的 VMA 指向已释放的物理内存

---

3. sys_shmget 大小验证中的静默失败

严重程度： 高
影响范围： Use-after-free、访问无效内存

问题位置： kernel/src/ipc/syscall/sys_shmget.rs:54-58

问题描述：

如果 get_mut(&id) 返回 None（段在检查之间被删除），大小验证被静默跳过，执行继续。这个竞态条件可能允许附加到无效段。

当前代码：

if let Some(kernel_shm) = shm_manager_guard.get_mut(&id) {
    if size > kernel_shm.size() {
        return Err(SystemError::EINVAL);
    }
}
// 如果 get_mut 返回 None，静默继续并返回 id！

修复方案：

let kernel_shm = shm_manager_guard.get_mut(&id)
    .ok_or(SystemError::EIDRM)?;  // 段已被删除

if size > 0 && size > kernel_shm.size() {
    return Err(SystemError::EINVAL);
}

---

4. 类型转换 Panic

严重程度： 中
影响范围： 64 位系统上大 PID 值导致的崩溃

问题位置： kernel/src/ipc/shm.rs:249, 264, 265, 666

问题描述：

多个 to_i32() 和 to_u32() 转换上的 .unwrap() 调用在 64 位系统上遇到非常大的 PID 值时会 panic。

当前代码：

let key = kern_ipc_perm.key.data().to_i32().unwrap();
let shm_cpid = kernel_shm.shm_cprid.data().to_u32().unwrap();
let shm_lpid = kernel_shm.shm_lprid.data().to_u32().unwrap();

修复方案：

let key = kern_ipc_perm.key.data().to_i32()
    .ok_or(SystemError::EOVERFLOW)?;
let shm_cpid = kernel_shm.shm_cprid.data().to_u32()
    .ok_or(SystemError::EOVERFLOW)?;
let shm_lpid = kernel_shm.shm_lprid.data().to_u32()
    .ok_or(SystemError::EOVERFLOW)?;

---

⚠️ 重要问题（应该修复）

5. 缺少锁顺序文档

严重程度： 中
影响范围： 潜在死锁

问题位置： kernel/src/mm/ucontext.rs:316-323

问题描述：

代码顺序获取多个锁（vma_guard → page_manager_guard → shm_manager_guard）但没有记录锁顺序策略。代码库中不一致的锁顺序可能导致死锁。

建议添加：

// 锁顺序：VMA 锁 → page_manager → shm_manager
// 始终在获取 shm_manager 之前释放 VMA 锁以防止死锁

---

6. 断言而非错误返回

严重程度： 中
影响范围： 逻辑错误导致内核崩溃

问题位置： kernel/src/ipc/shm.rs:475

问题描述：

decrease_count() 使用 assert!，在逻辑错误时 panic 而不是返回错误。

当前代码：

pub fn decrease_count(&mut self) {
    assert!(self.map_count > 0, "map_count is zero");
    self.map_count -= 1;
}

建议修改为：

pub fn decrease_count(&mut self) -> Result<(), SystemError> {
    if self.map_count == 0 {
        warn!("Attempted to decrease map_count when already zero");
        return Err(SystemError::EINVAL);
    }
    self.map_count -= 1;
    Ok(())
}

---

7. 关于序列号的不准确注释

严重程度： 低（文档问题）
影响范围： 误导性文档

问题位置： kernel/src/ipc/shm.rs:497-499

问题描述：

注释暗示 seq 字段用于 ID 重用检测，但它始终为 0，在代码库中从未递增。

当前注释：

/// 序列号：用于在 ShmId 被重用的时候进行区分
seq: usize,

建议修改为：

/// 序列号：预留用于未来实现 ShmId 重用区分机制
/// 当前版本中该字段始终为 0，尚无实际作用
/// TODO: 实现 ID 序列号机制
seq: usize,

---

💡 改进建议

类型设计评估

当前评分： 7/10

优点：

• ✅ 正确的 POSIX 语义（创建时 shm_lpid=0）
• ✅ 从进程上下文正确初始化凭证
• ✅ 通过 PERM_MASK 隔离权限位

弱点：

• ❌ 权限掩码没有编译时保证
• ❌ 需要在调用点手动强制执行
• ❌ 混合职责（ShmFlags 结合了权限和操作标志）

建议：

1. 将权限位和操作标志分离为不同的类型
2. 添加经过验证的构造函数/设置器
3. 通过类型结构使创建者 ID 不可变

---

注释和文档评估

当前评分： 8/10

优秀示例：

• VMA extract() 文档（kernel/src/mm/ucontext.rs:1736-1761）
• 边界条件解释
• VMA 分割的 ASCII 图表

需要改进：

1. 添加注释解释 VM_SHARED 标志初始化
2. 在 sys_shmget 中记录大小验证的基本原理
3. 解释时间戳单位修复（秒 vs 纳秒）
4. 在相关地方添加 POSIX 合规性参考

---

测试覆盖率评估

当前状态：

• ✅ 现有的 gvisor 测试提供了良好的集成覆盖
• ✅ 内部测试覆盖基本操作
• ❌ 没有针对此 PR 中修复的错误的特定测试

关键测试缺失：

1. Fork map_count 管理（严重性：9/10）

   • 无测试验证父进程 detach 不会销毁子进程的段
   • 建议测试： fork 后父子进程都能访问共享内存，父进程 detach 后子进程仍可访问

2. 凭证初始化（严重性：8/10）

   • 测试打印但不断言 uid/gid/cuid/cgid
   • 建议测试： 验证段所有权与创建进程匹配

3. 时间戳单位（严重性：8/10）

   • 无验证时间是秒（不是纳秒）
   • 建议测试： 验证时间戳在合理范围内（不是纳秒值）

4. 大小验证（严重性：8/10）

   • 无测试当请求更大大小时的 EINVAL
   • 建议测试： 验证请求大于现有段的大小会失败

建议： 在后续 PR 中添加针对这些特定行为的目标测试。

---

✅ 积极发现

此 PR 做出了重大改进：

1. 安全增强： 使用实际进程凭证而不是硬编码的 uid=0, gid=0
2. POSIX 合规性： 正确的 shm_lpid 初始化、时间戳单位、大小验证
3. 错误修复： VM_SHARED 标志初始化、权限位掩码
4. 文档良好的意图： 清晰的注释解释"操作者"概念
5. 正确的错误传播： 大多数路径正确使用 Result<T, SystemError>

---

📋 修复行动计划

合并前必须（严重）

1. ⚠️ 将所有 .unwrap() 和 .expect() 调用替换为正确的错误处理
2. ⚠️ 为 try_clone 中的竞态条件添加错误处理
3. ⚠️ 修复 sys_shmget 大小验证中的静默失败
4. ⚠️ 优雅地处理类型转换错误

合并后建议（重要）

1. 添加锁顺序文档
2. 将断言转换为错误返回
3. 更新不准确的注释
4. 为修复的行为添加针对性测试

未来改进（可选）

1. 实现或移除 seq 字段
2. 分离权限和标志类型
3. 为映射计数操作添加溢出检查
4. 添加调试用的验证方法

---

📊 问题统计

发现的问题：

• 🚨 严重问题：4 个
• ⚠️ 重要问题：3 个
• 💡 改进建议：7 个

需要修改的文件：

• kernel/src/ipc/shm.rs（7 处位置）
• kernel/src/ipc/syscall/sys_shmget.rs（1 处位置）
• kernel/src/mm/ucontext.rs（2 处位置）

---

📝 详细问题位置汇总

文件	行号	问题	严重程度
kernel/src/ipc/shm.rs	156	.expect() panic	严重
kernel/src/ipc/shm.rs	167	.unwrap() panic	严重
kernel/src/ipc/shm.rs	319	.unwrap() panic	严重
kernel/src/ipc/shm.rs	329	.get_unwrap() panic	严重
kernel/src/ipc/shm.rs	249, 264, 265, 666	类型转换 unwrap	中
kernel/src/ipc/shm.rs	475	assert! panic	中
kernel/src/ipc/shm.rs	497-499	不准确的注释	低
kernel/src/ipc/syscall/sys_shmget.rs	54-58	静默失败	高
kernel/src/mm/ucontext.rs	1011	.unwrap() panic	严重
kernel/src/mm/ucontext.rs	316-323	竞态条件	高
kernel/src/mm/ucontext.rs	316-323	缺少锁顺序文档	中

---

总结

整体评估： 此 PR 修复了合法的 POSIX 合规性错误，语义更改是正确的。然而，错误处理问题（特别是 panic 风险）必须在合并前解决，以防止内核不稳定和拒绝服务漏洞。

建议：

1. 修复所有严重问题后再合并 - 这些问题可能导致内核崩溃和安全漏洞
2. 添加目标测试 - 防止已修复行为的回归
3. 改进文档 - 锁顺序、权限掩码逻辑等

语义更改正确的方面：

• ✅ shm_lpid 初始化为 0（POSIX 合规）
• ✅ 使用进程凭证而非硬编码的 root 凭证
• ✅ 时间戳使用秒而非纳秒
• ✅ 大小验证符合 POSIX 要求
• ✅ VM_SHARED 标志修复共享内存语义
• ✅ PERM_MASK 防止权限位污染

需要立即关注的方面：

• ❌ 多处可能导致内核 panic 的 unwrap/expect
• ❌ 竞态条件可能导致 use-after-free
• ❌ 静默失败可能导致访问无效内存
• ❌ 类型转换可能在大值时 panic

[mistcoversmyeyes]

提到的7个问题修了 1、2、3、4、5。问题6感觉先放在那里先，这不是本次 PR 引入的，先不改先。问题 7 我看了一下没啥问题，注释听清楚的。CI 测试没通过，我正在本地跑。

[mistcoversmyeyes]

当前 CI 的错误我本地切换到 master 分支跑了一下，能够复现，应该不是我这个 PR 引入的问题