Skip to content

Conversation

@sparkzky
Copy link
Collaborator

@sparkzky sparkzky commented Feb 4, 2026

实现完整的 pivot_root(2) 系统调用,支持容器根文件系统切换。

主要功能:

  • 新增 sys_pivot_root.rs,实现 Linux 兼容的 pivot_root 语义
  • MountFS 增加 bind_target_root 支持,正确处理 bind mount 场景
  • 挂载命名空间增加旧根挂载点管理,支持 pivot_root 后卸载
  • umount2 增加对当前目录卸载的特殊处理

  实现完整的 pivot_root(2) 系统调用,支持容器根文件系统切换。

  主要功能:
  - 新增 sys_pivot_root.rs,实现 Linux 兼容的 pivot_root 语义
  - MountFS 增加 bind_target_root 支持,正确处理 bind mount 场景
  - 挂载命名空间增加旧根挂载点管理,支持 pivot_root 后卸载
  - umount2 增加对当前目录卸载的特殊处理

Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
@github-actions github-actions bot added the enhancement New feature or request label Feb 4, 2026
@sparkzky
Copy link
Collaborator Author

sparkzky commented Feb 4, 2026

@claude review

@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Feb 4, 2026

Claude finished @sparkzky's task in 7m 1s —— View job


PR Review: pivot_root system call implementation

I've completed my review of the PR implementing the pivot_root system call. Here's my comprehensive feedback:

Todo List:

  • Analyze the PR context and changed files
  • Read the new sys_pivot_root.rs implementation
  • Review modifications to mount.rs, sys_mount.rs, sys_umount2.rs
  • Review changes to namespace/mnt.rs and utils.rs
  • Check for potential bugs, security issues, and edge cases
  • Evaluate code style and consistency with existing codebase
  • Test compilation and look for any compilation errors
  • Provide comprehensive review feedback

Overall Assessment

This is a well-implemented feature that adds Linux-compatible pivot_root(2) system call support to DragonOS. The implementation correctly handles container root filesystem switching and addresses DragonOS's bind mount architecture differences.

Strengths

  1. Correct Linux semantics: The implementation follows Linux pivot_root semantics including:

    • new_root and put_old must be directories
    • put_old must be under new_root
    • Requires CAP_SYS_CHROOT capability
    • Current working directory cannot be in put_old
  2. Handles DragonOS architecture differences: The bind_target_root mechanism correctly addresses DragonOS's bind mount implementation differences vs Linux.

  3. Good error handling: Comprehensive error checking with appropriate error codes (ENOTDIR, EINVAL, EBUSY, EPERM, etc.).

  4. Defensive programming: Loop limits (MAX_ANCESTOR_TRAVERSAL_DEPTH = 1000) prevent infinite loops during directory traversal.

  5. Proper cleanup: umount2 enhancements handle old root unmounting after pivot_root.

Issues and Recommendations

1. Code Duplication - is_ancestor function

  • File: kernel/src/filesystem/vfs/syscall/sys_pivot_root.rs:371-425
  • Issue: Duplicates functionality from kernel/src/filesystem/vfs/utils.rs:86-120
  • Recommendation: Use the existing utils::is_ancestor() function or refactor to avoid duplication.

2. Complex MountFS Lookup Logic

  • File: sys_pivot_root.rs:263-369 (get_mountfs() method)
  • Issue: 70+ lines with multiple fallback paths; could match incorrect filesystems if names collide
  • Recommendation:
    • Break into helper functions
    • Add more precise matching criteria beyond filesystem name
    • Consider adding a direct mapping from inode to MountFS

3. Unsafe Operations Need Better Documentation

  • Files:
    • mount.rs:336-342 (set_mount_flags())
    • mnt.rs:99-116 (force_change_root_mountfs())
  • Issue: Safety comments are minimal; need to document why these are safe
  • Recommendation: Add detailed safety comments explaining:
    • Why data races are prevented
    • What invariants are maintained
    • Why these operations can't fail

4. Inconsistent Logging

  • Issue: Many log::info! calls are commented out
  • Recommendation: Use log::debug! for debug logging instead of commented log::info!

5. Potential Edge Cases

  • bind mount propagation: Need to verify bind mount propagation flags are handled correctly during pivot_root
  • Multiple namespaces: Implementation assumes single mount namespace; need to verify cross-namespace behavior

6. Error Propagation

  • Example: sys_pivot_root.rs:113-117 logs error but could provide more context
  • Recommendation: Include inode details in error messages for debugging

Security Assessment

  1. Capability check: Correctly requires CAP_SYS_CHROOT (line 74)
  2. Path validation: Validates paths are not empty and are directories
  3. Permission checks: Uses cred.inode_permission() for directory traversal
  4. User pointer validation: Uses vfs_check_and_clone_cstr() for safe user memory access

CI Status

Build check passes ✅, but one test failed:

  • Build Check: Passed
  • Claude Code Review: Passed
  • Test MM (Host): Failed (may be unrelated to this PR)
  • Test x86_64: In progress

The build success indicates the code compiles correctly.

Testing Recommendations

  1. Unit tests: Add tests for:

    • new_root and put_old same inode case
    • Invalid directory types
    • Permission denied scenarios
    • Nested mount point scenarios
  2. Integration tests: Test complete container workflow:

    • Bind mount rootfs
    • pivot_root to new root
    • umount2(".") to clean up old root

Conclusion

The implementation is functionally correct and follows Linux semantics. With the recommended improvements (particularly consolidating is_ancestor and documenting unsafe operations), this will be a robust addition to DragonOS's container support.

Recommended Action: Approve with suggestions for minor improvements.

View CI runs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant