feat(vfs): implement pivot_root for mount namespaces

[sparkzky]

Add pivot_root(2) support for mount namespaces, including mount list
path rewrites, namespace-root handling, and rebinding of process fs root
and cwd after namespace switches.

Also fix mount-root path resolution and umount path lookup for moved
mount trees, and add C unit tests covering pivot_root behavior.

[sparkzky]

@claude review

[dragonos-community-ai-pr-reviewer]

Claude finished @sparkzky's task in 9m 58s —— View job

---

Claude's PR Review

I've reviewed the pivot_root implementation for mount namespaces. Overall, the implementation is comprehensive and follows Linux semantics closely. The code is well-structured, and the unit tests cover many important edge cases. Below are my detailed observations, suggestions, and potential issues.

Overall Assessment

• Correctness: The implementation correctly enforces pivot_root semantics (permission checks, mount point validation, path relationships, propagation restrictions).
• Completeness: Includes mount list path rewrites, namespace-root handling, and rebinding of process fs root/cwd after namespace switches.
• Testing: The C unit tests are thorough and cover success paths, error conditions, and special cases like dot-dot pivot.

Key Observations

1. Propagation Check Scope

In sys_pivot_root.rs lines 165–171, the code rejects the operation if any of the involved mounts (current root, new root, new root's parent, put_old) have shared propagation. According to the Linux man page, only the parent mount of new_root must not be shared (and new_root itself must not be shared). The current root and put_old mounts are not required to be private. However, this stricter check may be intentional for simplicity or safety. If you intend to match Linux exactly, consider narrowing the check.

2. Unsafe Pointer Casts for Mutability

In mnt.rs, both force_change_root_mountfs (lines 98–101) and pivot_root (lines 157–160) use unsafe pointer casts to mutate root_mountfs while holding only a &self reference. This pattern is risky because:

• root_mountfs is read without holding the inner lock (e.g., line 117 let old_root = self.root_mountfs.clone()).
• Concurrent reads and writes to root_mountfs could cause data races (assignment of an Arc is not atomic).
• The same pattern appears elsewhere (force_change_root_mountfs).

Suggestion: Move root_mountfs into InnerMntNamespace (protected by the same spinlock) or wrap it in a RwLock. This ensures atomic updates and eliminates unsafe.

3. Root Mount Self‑Mountpoint Check

The code requires new_root_mntfs.self_mountpoint() to be Some (line 161). This means new_root cannot be the root mount (the mount at /). While pivoting to the same root mount is already prevented by the EBUSY check (line 142–145), there is a theoretical case where new_root is a different mount that is also a root mount (i.e., an unmounted filesystem). This is probably intentional, but worth noting.

4. Path Rewriting Logic

The rewrite_pivot_path and normalize_pivot_path functions (in both sys_pivot_root.rs and mnt.rs) have duplicated logic. Consider extracting them to a common utility module to avoid divergence.

5. Task Iteration in repair_same_namespace_fs_refs

This function iterates over all processes in the system to find those sharing the target mount namespace. For systems with many processes, this could be expensive. Since pivot_root is a rare operation, this is likely acceptable, but you might want to maintain a per‑namespace task list for faster updates.

6. Missing Error Code for Non‑Directory put_old

The ensure_searchable_dir check ensures both new_root and put_old are directories. If put_old is not a directory, the error is ENOTDIR. This matches Linux behavior (verified).

7. Unused UmountFlag Parameter

In sys_umount2.rs, the _flag parameter of do_umount2 is unused. This is fine for now, but the flag handling should be implemented when needed.

8. Logging in sys_umount2

The log::warn! statements (lines 99–104, 110–115, 120) may produce noise in normal operation. Consider lowering them to debug! or making them conditional.

9. C Unit Tests

The test suite is excellent, covering:

• Success case with absolute and relative paths
• Dot‑dot pivot (. .)
• Propagation and detach scenarios
• Error cases (not a mount point, put_old outside new root, busy target, permission failure)

One missing test: new_root is a symlink (should be followed). The implementation uses lookup_follow_symlink, so it should work, but a test would be beneficial.

Potential Bugs

Race Condition in root_mountfs Access

As noted above, concurrent calls to pivot_root or force_change_root_mountfs could race with readers of root_mountfs. This could lead to torn reads or undefined behavior. This should be fixed.

Incorrect Rollback on EBUSY

In mnt.rs lines 141–152, if inserting old_root into put_old_parent.mountpoints() fails (returns Some), the code attempts to restore new_root to its parent. However, the new_root_parent.add_mount call might also fail, leaving the mount tree in an inconsistent state. Consider using a transaction‑like pattern that prepares all changes before committing.

Path Comparison in same_path_ref

The same_path_ref function (in both sys_pivot_root.rs and sys_umount2.rs) compares filesystem identity and inode IDs. This is correct, but note that it returns false if metadata lookup fails. This could hide legitimate errors; consider propagating the error instead of silently failing.

Suggestions for Improvement

1. Eliminate unsafe pointer casts by moving root_mountfs into InnerMntNamespace.
2. Deduplicate path‑rewriting helpers between sys_pivot_root.rs and mnt.rs.
3. Consider adding a per‑namespace task list for faster updates during pivot (optional).
4. Add a test for symlink handling in the C unit tests.
5. Review propagation checks against the Linux man page to decide whether to relax them.
6. Implement UmountFlag handling (future work).
7. Lower log level for umount2 warnings.

Conclusion

This is a solid implementation of a complex system call. The few issues identified are mostly about code safety and maintainability, not functional correctness. Addressing the race condition on root_mountfs is the highest priority.

Great work! 🚀

---

Next Steps: If you'd like me to help fix any of the issues above, please let me know which ones you'd like addressed first.

[fslongjin]

测例加到dunitest下面，这个会自动运行。