fix(fs): 修复 sys_rename 逻辑并支持 RENAME_NOREPLACE by kaleidoscope416 · Pull Request #1393 · DragonOS-Community/DragonOS

kaleidoscope416 · 2025-11-20T11:56:30Z

逻辑修复：
· 修正覆盖现有文件/目录时的逻辑，确保目标被正确截断和删除。
· 增加文件类型检查，禁止文件与目录之间的非法覆盖。
· 增加祖先关系检查，防止将目录移动到其子目录下。
· 修复同名重命名及非空目录覆盖的边界情况。
· 拦截源或目标路径以 . 或 .. 结尾的非法操作。

功能增强：
引入RenameFlags支持，实现了 RENAME_NOREPLACE 语义。

Pull Request Overview

This PR fixes critical logic bugs in the sys_rename implementation and adds support for the RENAME_NOREPLACE flag. The changes enhance rename operation safety by adding proper validation for file type compatibility, ancestor relationship checks, and special path component handling.

Key changes:

Introduced RenameFlags bitflags structure with support for NOREPLACE, EXCHANGE, and WHITEOUT flags
Added ancestor relationship checking to prevent moving directories into their own subdirectories
Fixed file/directory overwrite logic with proper type checking and page cache truncation
Added validation to reject operations on "." and ".." path components

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
user/apps/tests/syscall/gvisor/whitelist.txt	Enables rename_test in gvisor test suite
user/apps/tests/syscall/gvisor/blocklists/rename_test	Defines blocklist for unimplemented rename test cases
kernel/src/filesystem/vfs/utils.rs	Adds `is_ancestor_except_self` helper for directory hierarchy validation
kernel/src/filesystem/vfs/syscall/rename_utils.rs	Implements NOREPLACE flag support and path validation logic
kernel/src/filesystem/vfs/syscall/mod.rs	Defines RenameFlags bitflags structure
kernel/src/filesystem/vfs/mount.rs	Updates MountFSInode to pass flags to move_to
kernel/src/filesystem/vfs/mod.rs	Updates IndexNode trait to include flags parameter
kernel/src/filesystem/ramfs/mod.rs	Updates ramfs move_to signature to accept flags
kernel/src/filesystem/procfs/mod.rs	Updates procfs move_to signature to accept flags
kernel/src/filesystem/kernfs/mod.rs	Updates kernfs move_to signature to accept flags
kernel/src/filesystem/fat/fs.rs	Implements rename logic with EXCHANGE flag handling and type checking
kernel/src/filesystem/fat/entry.rs	Implements file type validation, overwrite logic, and page cache truncation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T12:00:16Z

kernel/src/filesystem/vfs/syscall/rename_utils.rs

+        return Err(SystemError::EBUSY);
+    }
+
+    if !flags.is_empty() {


The NOREPLACE flag check is performed, but then at line 78-81, there's a check that returns EINVAL for any non-empty flags. This means after the NOREPLACE handling (lines 65-72), if NOREPLACE is set, the flags will still be non-empty and will trigger the EINVAL error at line 80.

The check at lines 78-81 should exclude NOREPLACE from consideration, or be moved before the NOREPLACE handling. Consider:

if flags.intersects(RenameFlags::EXCHANGE | RenameFlags::WHITEOUT) { log::warn!("renameat2 flags {flags:?} not supported yet"); return Err(SystemError::EINVAL); }

Suggested change

if !flags.is_empty() {

if flags.intersects(RenameFlags::EXCHANGE | RenameFlags::WHITEOUT) {

Copilot · 2025-11-20T12:00:16Z

kernel/src/filesystem/fat/entry.rs

+                if let Some(page_cache) = new_inode.unwrap().page_cache().clone() {
+                    truncate_inode_pages(page_cache, 0);


Calling .unwrap() on new_inode will panic if new_inode is None. This can happen when the target doesn't exist (ShortName case at line 870).

Use pattern matching or safe handling:

if let Some(inode) = &new_inode { if let Some(page_cache) = inode.page_cache().clone() { truncate_inode_pages(page_cache, 0); } }

Suggested change

if let Some(page_cache) = new_inode.unwrap().page_cache().clone() {

truncate_inode_pages(page_cache, 0);

if let Some(inode) = &new_inode {

if let Some(page_cache) = inode.page_cache().clone() {

truncate_inode_pages(page_cache, 0);

}

在target存在时才执行这个unwrap，不会解到None

Copilot · 2025-11-20T12:00:17Z

kernel/src/filesystem/fat/fs.rs

        let mut guard = self.0.lock();
        let old_inode = guard.find(old_name)?;
+        let new_inode = guard.find(new_name).ok();
+        // do_rename2_at has checked NOREPLACE


Typo in comment: "do_rename2_at" should be "do_renameat2" to match the actual function name.

Suggested change

// do_rename2_at has checked NOREPLACE

// do_renameat2 has checked NOREPLACE

Copilot · 2025-11-20T12:00:17Z

kernel/src/filesystem/fat/fs.rs

+        let mut new_guard = other.0.lock();
        let old_inode: Arc<LockedFATInode> = old_guard.find(old_name)?;
+        let new_inode = new_guard.find(new_name);
+        // do_rename2_at has checked NOREPLACE


Typo in comment: "do_rename2_at" should be "do_renameat2" to match the actual function name.

Suggested change

// do_rename2_at has checked NOREPLACE

// do_renameat2 has checked NOREPLACE

Copilot · 2025-11-20T12:00:18Z

kernel/src/filesystem/fat/entry.rs

+                // 目录不能覆盖文件
+                if old_is_dir && !new_is_dir {
+                    return Err(SystemError::ENOTDIR);
+                }
+                // 文件不能覆盖目录
+                if !old_is_dir && new_is_dir {
+                    return Err(SystemError::EISDIR);
+                }


This file type checking logic is duplicated between the rename function (lines 876-883) and rename_across function (lines 940-947). Consider extracting this into a helper function to reduce code duplication and improve maintainability:

fn check_rename_type_compatibility(old_is_dir: bool, new_is_dir: bool) -> Result<(), SystemError> { if old_is_dir && !new_is_dir { return Err(SystemError::ENOTDIR); } if !old_is_dir && new_is_dir { return Err(SystemError::EISDIR); } Ok(()) }

Copilot · 2025-11-20T12:00:18Z

kernel/src/filesystem/vfs/syscall/rename_utils.rs

+        if old_meta.inode_id == new_parent_meta.inode_id
+            || is_ancestor_except_self(&old_inode, &new_parent_inode)


The logic here checks if old_inode (source directory) equals new_parent_inode OR if old_inode is an ancestor of new_parent_inode. However, the first condition is incorrect: when old_meta.inode_id == new_parent_meta.inode_id, it means the source directory IS the destination parent directory, which is a valid rename within the same directory (e.g., renaming dir1/file to dir1/newfile).

The correct check should only prevent moving a directory into its own subdirectory:

if is_ancestor_except_self(&old_inode, &new_parent_inode) { return Err(SystemError::EINVAL); }

Remove the old_meta.inode_id == new_parent_meta.inode_id condition.

Suggested change

if old_meta.inode_id == new_parent_meta.inode_id

|| is_ancestor_except_self(&old_inode, &new_parent_inode)

if is_ancestor_except_self(&old_inode, &new_parent_inode)

Copilot · 2025-11-20T12:00:18Z

kernel/src/filesystem/fat/entry.rs

+                if let Some(page_cache) = new_inode.unwrap().page_cache().clone() {
+                    truncate_inode_pages(page_cache, 0);


Calling .unwrap() on new_inode will panic if it contains an Err value. The parameter type is Result<Arc<LockedFATInode>, SystemError>, so when the target doesn't exist, this will be an Err.

Use pattern matching or safe handling:

if let Ok(inode) = &new_inode { if let Some(page_cache) = inode.page_cache().clone() { truncate_inode_pages(page_cache, 0); } }

Suggested change

if let Some(page_cache) = new_inode.unwrap().page_cache().clone() {

truncate_inode_pages(page_cache, 0);

if let Ok(inode) = &new_inode {

if let Some(page_cache) = inode.page_cache().clone() {

truncate_inode_pages(page_cache, 0);

}

和上面一样

Copilot · 2025-11-20T12:00:18Z

kernel/src/filesystem/fat/entry.rs

+                let old_is_dir = old_dentry.is_dir();
+                let new_is_dir = e.is_dir();
+
+                // 目录不能覆盖文件
+                if old_is_dir && !new_is_dir {
+                    return Err(SystemError::ENOTDIR);
+                }
+                // 文件不能覆盖目录
+                if !old_is_dir && new_is_dir {
+                    return Err(SystemError::EISDIR);
+                }
+                if e.is_dir() && !(e.to_dir().unwrap().is_empty(fs.clone())) {
+                    return Err(SystemError::ENOTEMPTY);
+                }


This non-empty directory check logic is duplicated in both rename (line 884-886) and rename_across (lines 948-950). Consider extracting it into a helper function or consolidating with the file type checking to reduce duplication:

fn validate_rename_target(old_entry: &FATDirEntry, new_entry: &FATDirEntry, fs: Arc<FATFileSystem>) -> Result<(), SystemError> { let old_is_dir = old_entry.is_dir(); let new_is_dir = new_entry.is_dir(); if old_is_dir && !new_is_dir { return Err(SystemError::ENOTDIR); } if !old_is_dir && new_is_dir { return Err(SystemError::EISDIR); } if new_entry.is_dir() && !(new_entry.to_dir().unwrap().is_empty(fs)) { return Err(SystemError::ENOTEMPTY); } Ok(()) }

Suggested change

let old_is_dir = old_dentry.is_dir();

let new_is_dir = e.is_dir();

// 目录不能覆盖文件

if old_is_dir && !new_is_dir {

return Err(SystemError::ENOTDIR);

}

// 文件不能覆盖目录

if !old_is_dir && new_is_dir {

return Err(SystemError::EISDIR);

}

if e.is_dir() && !(e.to_dir().unwrap().is_empty(fs.clone())) {

return Err(SystemError::ENOTEMPTY);

}

validate_rename_target(&old_dentry, &e, fs.clone())?;

Copilot · 2025-11-20T12:00:19Z

kernel/src/filesystem/vfs/syscall/rename_utils.rs

+        if new_filename == "." || new_filename == ".." {
+            return Err(SystemError::EEXIST);
+        }


When NOREPLACE flag is set and new_filename is "." or "..", the function returns EEXIST. However, this check is redundant because these special filenames are already blocked at lines 74-76 with EBUSY error for all rename operations. The EEXIST check here should only apply when the target file actually exists, not for special path components. Consider removing this check or moving it after the general "." and ".." validation.

Suggested change

if new_filename == "." || new_filename == ".." {

return Err(SystemError::EEXIST);

}

这里情况不一样，加这个为了通过测试

fslongjin · 2025-11-21T16:49:00Z

测例没过。

kernel/src/filesystem/vfs/mount.rs

kaleidoscope416 added 9 commits November 18, 2025 17:28

加入判断祖先关系的函数；正确覆盖重命名相同名字情况；正确覆盖目标目录为非空目录的情况

f904d19

rename替换时，删除路径的文件

97ee20a

拒绝文件类型不同的覆盖

42f17a2

在remove目录项前加入截断

416d420

使覆盖逻辑正确

0446e0f

拦截源或目标的末尾路径分量是 "." 或 ".."的情况

76341b2

fmt, 增加测试

6c057ec

fmt

b050443

修改blocklist

6d9a70c

Copilot AI review requested due to automatic review settings November 20, 2025 11:56

github-actions bot added the Bug fix A bug is fixed in this pull request label Nov 20, 2025

Copilot started reviewing on behalf of kaleidoscope416 November 20, 2025 11:56 View session

Copilot finished reviewing on behalf of kaleidoscope416 November 20, 2025 11:58

Copilot AI reviewed Nov 20, 2025

View reviewed changes

kaleidoscope416 added 4 commits November 20, 2025 20:17

copilot advise & clippy

f0e3409

continue copilot advise

94cd876

clippy

66fb8c4

fix

ac64bb6

kaleidoscope416 force-pushed the fix_rename branch from ef90258 to edeaaf3 Compare November 21, 2025 12:54

kaleidoscope416 added 2 commits November 22, 2025 14:47

fmt

3f17317

fix test

ce10794

kaleidoscope416 force-pushed the fix_rename branch from edeaaf3 to ce10794 Compare November 22, 2025 06:49

Merge branch 'master' into fix_rename

c144a34

fslongjin reviewed Nov 24, 2025

View reviewed changes

kernel/src/filesystem/vfs/mount.rs Outdated Show resolved Hide resolved

1

762bfc3

fslongjin approved these changes Nov 24, 2025

View reviewed changes

fslongjin merged commit 54c2a45 into DragonOS-Community:master Nov 24, 2025
13 of 14 checks passed

	if !flags.is_empty() {
	if flags.intersects(RenameFlags::EXCHANGE \| RenameFlags::WHITEOUT) {

		if let Some(page_cache) = new_inode.unwrap().page_cache().clone() {
		truncate_inode_pages(page_cache, 0);

	// do_rename2_at has checked NOREPLACE
	// do_renameat2 has checked NOREPLACE

		if old_meta.inode_id == new_parent_meta.inode_id
		\|\| is_ancestor_except_self(&old_inode, &new_parent_inode)

	if old_meta.inode_id == new_parent_meta.inode_id
	\|\| is_ancestor_except_self(&old_inode, &new_parent_inode)
	if is_ancestor_except_self(&old_inode, &new_parent_inode)

	if new_filename == "." \|\| new_filename == ".." {
	return Err(SystemError::EEXIST);
	}

Uh oh!

Conversation

kaleidoscope416 commented Nov 20, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

kaleidoscope416 Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

kaleidoscope416 Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

kaleidoscope416 Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

fslongjin commented Nov 21, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaleidoscope416 commented Nov 20, 2025 •

edited by cursor bot

Loading