fix(ipc): 修复 ShmTest 错误的测试用例

[mistcoversmyeyes]

关联到:

• fix(gvisor):修复 shm_test 未通过测试用例 #1632

[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]

这个的创建、填写uid/gid的工作应该是可以封装成一个new函数里面做？

[fslongjin]

为什么是0

[mistcoversmyeyes]

Linux 中创建一个新的内存段的函数是 shm.c:new_seg ,这个函数会在创建的时候将 shm_lprid 的值设置为 NULL 。所以这里设置为 0 肯定是没问题的

// ./linux-6.6/ipc/shm.c:705
static int newseg(struct ipc_namespace *ns, struct ipc_params *params) {
        // ....
	shp->shm_cprid = get_pid(task_tgid(current));
	shp->shm_lprid = NULL;
	shp->shm_atim = shp->shm_dtim = 0;
	shp->shm_ctim = ktime_get_real_seconds();
	shp->shm_segsz = size;
	shp->shm_nattch = 0;
	shp->shm_file = file;
	shp->shm_creator = current;
        // ...
}

[mistcoversmyeyes]

我加个注释说明一下。