From fd86797d55da3241075c930fff0e06f22a91bc80 Mon Sep 17 00:00:00 2001 From: jinwjinl <2532191107@qq.com> Date: Mon, 10 Aug 2026 21:33:08 +0800 Subject: [PATCH 1/5] fix lack of rename --- header/src/lib.rs | 1 + kernel/src/syscall_handlers/mod.rs | 9 +++ kernel/src/vfs/dcache.rs | 5 ++ kernel/src/vfs/fatfs.rs | 124 ++++++++++++++++++++++++++++- kernel/src/vfs/syscalls.rs | 48 +++++++++++ kernel/src/vfs/tmpfs.rs | 57 +++++++++++++ 6 files changed, 241 insertions(+), 3 deletions(-) diff --git a/header/src/lib.rs b/header/src/lib.rs index 7a53fb2d7..bac3be9cd 100644 --- a/header/src/lib.rs +++ b/header/src/lib.rs @@ -90,6 +90,7 @@ pub mod syscalls { TimerGetTime, TimerSetTime, TimerGetOverrun, + Rename, LastNR, } } diff --git a/kernel/src/syscall_handlers/mod.rs b/kernel/src/syscall_handlers/mod.rs index 1cd3e6254..d281cfbb3 100644 --- a/kernel/src/syscall_handlers/mod.rs +++ b/kernel/src/syscall_handlers/mod.rs @@ -69,6 +69,9 @@ mod vfs_syscalls { pub fn link(_oldpath: *const c_char, _newpath: *const c_char) -> i32 { -libc::ENOTSUP } + pub fn rename(_oldpath: *const c_char, _newpath: *const c_char) -> i32 { + -libc::ENOTSUP + } pub fn unlink(_path: *const c_char) -> i32 { -libc::ENOTSUP } @@ -541,6 +544,11 @@ define_syscall_handler!( vfs_syscalls::link(oldpath, newpath) } ); +define_syscall_handler!( + rename(oldpath: *const c_char, newpath: *const c_char) -> c_int { + vfs_syscalls::rename(oldpath, newpath) + } +); define_syscall_handler!( unlink(path: *const c_char) -> c_int { vfs_syscalls::unlink(path) @@ -868,6 +876,7 @@ syscall_table! { (Rmdir, rmdir), (Link, link), (Unlink, unlink), + (Rename, rename), (Fcntl, fcntl), (Stat, stat), (FStat, fstat), diff --git a/kernel/src/vfs/dcache.rs b/kernel/src/vfs/dcache.rs index 5093e52b0..45562bec1 100644 --- a/kernel/src/vfs/dcache.rs +++ b/kernel/src/vfs/dcache.rs @@ -368,6 +368,10 @@ impl Dcache { return Err(code::EBUSY); } + if ptr::addr_eq(self, Arc::as_ptr(new_dir)) && old_name == new_name { + return Ok(()); + } + // rename in the same directory if ptr::addr_eq(self, Arc::as_ptr(new_dir)) && old_name != new_name { if children.contains_key(new_name) { @@ -376,6 +380,7 @@ impl Dcache { } self.inode.rename(old_name, &self.inode, new_name)?; children.remove(old_name); + child.set_name_and_parent(new_name, self.this.clone()); if child.is_dcacheable() { children.insert(String::from(new_name), child); } diff --git a/kernel/src/vfs/fatfs.rs b/kernel/src/vfs/fatfs.rs index 171b54d9c..7095a33e5 100644 --- a/kernel/src/vfs/fatfs.rs +++ b/kernel/src/vfs/fatfs.rs @@ -322,11 +322,11 @@ impl core::fmt::Debug for FatFileData { struct FatFile { _parent: Weak, - internal_file: InternalFsLock, + internal_file: InternalFsLock>, } impl FatFile { - fn new(parent: &Weak, internal_file: InternalFsLock) -> Self { + fn new(parent: &Weak, internal_file: InternalFsLock>) -> Self { Self { _parent: parent.clone(), internal_file, @@ -400,7 +400,7 @@ impl FatInode { attr, data: FatFileData::File(FatFile::new( parent, - internal_fs_wrapper.wrap(internal_file), + internal_fs_wrapper.wrap(Some(internal_file)), )), }), this: weak_inode.clone(), @@ -562,10 +562,12 @@ impl InodeOps for FatInode { { let inner = self.inner.read(); let (file, _) = inner.as_file().unwrap().internal_file.get(); + let file = file.as_ref().unwrap(); assert_eq!(file.size().unwrap(), inner.attr.size.try_into().unwrap()); } let mut inner = self.inner.write(); let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); + let file = file.as_mut().unwrap(); let expected_read_size = buf.len(); let mut offset = offset; let mut total_read_size = 0; @@ -592,6 +594,7 @@ impl InodeOps for FatInode { let (write_size, new_size, extents) = { let mut inner = self.inner.write(); let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); + let file = file.as_mut().unwrap(); let mut offset = offset; let mut total_write_size = 0; let expected_write_size = buf.len(); @@ -677,6 +680,119 @@ impl InodeOps for FatInode { Ok(()) } + fn rename( + &self, + old_name: &str, + target: &Arc, + new_name: &str, + ) -> Result<(), Error> { + if old_name == "." || old_name == ".." || new_name == "." || new_name == ".." { + return Err(code::EINVAL); + } + let target = target.downcast_ref::().ok_or(code::EXDEV)?; + let source_fs = self.fs.upgrade().ok_or(code::EAGAIN)?; + let target_fs = target.fs.upgrade().ok_or(code::EAGAIN)?; + if !Arc::ptr_eq(&source_fs, &target_fs) { + return Err(code::EXDEV); + } + if self.type_() != InodeFileType::Directory || target.type_() != InodeFileType::Directory { + return Err(code::ENOTDIR); + } + if core::ptr::eq(self, target) { + let mut inner = self.inner.write(); + let dir = inner.as_dir_mut().unwrap(); + let child = dir.find(old_name).ok_or(code::ENOENT)?; + if old_name == new_name { + return Ok(()); + } + if dir.find(new_name).is_some() { + return Err(code::EEXIST); + } + let mut child_inner = child.inner.write(); + let is_file = child_inner.attr.type_() == InodeFileType::Regular; + if is_file { + let file = child_inner.as_file_mut().unwrap(); + let (slot, guard) = file.internal_file.get_mut(); + let old_file = slot.take().ok_or(code::EIO)?; + drop(old_file); + drop(guard); + } + + let (internal_dir, guard) = dir.internal_dir.get(); + if let Err(error) = internal_dir.rename(old_name, internal_dir, new_name) { + if is_file { + child_inner.as_file_mut().unwrap().internal_file.content = + Some(internal_dir.open_file(old_name)?); + } + return Err(error.into()); + } + if is_file { + match internal_dir.open_file(new_name) { + Ok(file) => { + child_inner.as_file_mut().unwrap().internal_file.content = Some(file); + } + Err(error) => { + let _ = internal_dir.rename(new_name, internal_dir, old_name); + child_inner.as_file_mut().unwrap().internal_file.content = + Some(internal_dir.open_file(old_name)?); + return Err(error.into()); + } + } + } + drop(guard); + dir.remove(old_name); + dir.insert(new_name, &child); + return Ok(()); + } + + let mut source_inner = self.inner.write(); + let mut target_inner = target.inner.write(); + let source_dir = source_inner.as_dir_mut().unwrap(); + let target_dir = target_inner.as_dir_mut().unwrap(); + if target_dir.find(new_name).is_some() { + return Err(code::EEXIST); + } + let child = source_dir.find(old_name).ok_or(code::ENOENT)?; + let mut child_inner = child.inner.write(); + let is_file = child_inner.attr.type_() == InodeFileType::Regular; + if is_file { + let file = child_inner.as_file_mut().unwrap(); + let (slot, guard) = file.internal_file.get_mut(); + let old_file = slot.take().ok_or(code::EIO)?; + drop(old_file); + drop(guard); + } + + let (source_internal, guard) = source_dir.internal_dir.get(); + let target_internal = &target_dir.internal_dir.content; + if let Err(error) = source_internal.rename(old_name, target_internal, new_name) { + if is_file { + child_inner.as_file_mut().unwrap().internal_file.content = + Some(source_internal.open_file(old_name)?); + } + return Err(error.into()); + } + if is_file { + match target_internal.open_file(new_name) { + Ok(file) => { + child_inner.as_file_mut().unwrap().internal_file.content = Some(file); + } + Err(error) => { + let _ = target_internal.rename(new_name, source_internal, old_name); + child_inner.as_file_mut().unwrap().internal_file.content = + Some(source_internal.open_file(old_name)?); + return Err(error.into()); + } + } + } else { + child_inner.as_dir_mut().unwrap().parent = target.this.clone(); + } + drop(guard); + source_dir.remove(old_name); + target_dir.insert(new_name, &child); + Ok(()) + } + fn getdents_at(&self, offset: usize, reader: &mut DirBufferReader) -> Result { if self.type_() != InodeFileType::Directory { error!("[FatInode] getdents_at: not a directory"); @@ -748,6 +864,7 @@ impl InodeOps for FatInode { let (new_size, extents) = { let mut inner = self.inner.write(); let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); + let file = file.as_mut().unwrap(); file.seek(SeekFrom::Start(size as u64))?; file.truncate()?; let new_size = file.size().unwrap() as usize; @@ -805,6 +922,7 @@ impl InodeOps for FatInode { } let mut inner = self.inner.write(); let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); + let file = file.as_mut().unwrap(); file.flush()?; Ok(()) } diff --git a/kernel/src/vfs/syscalls.rs b/kernel/src/vfs/syscalls.rs index ec9c96f99..0dd09c3d3 100644 --- a/kernel/src/vfs/syscalls.rs +++ b/kernel/src/vfs/syscalls.rs @@ -433,6 +433,36 @@ pub fn link(old_path: *const c_char, new_path: *const c_char) -> c_int { } } +pub fn rename(old_path: *const c_char, new_path: *const c_char) -> c_int { + if old_path.is_null() || new_path.is_null() { + return -libc::EINVAL; + } + + let old_path = match unsafe { CStr::from_ptr(old_path).to_str() } { + Ok(path) => path, + Err(_) => return -libc::EINVAL, + }; + let new_path = match unsafe { CStr::from_ptr(new_path).to_str() } { + Ok(path) => path, + Err(_) => return -libc::EINVAL, + }; + + let (old_dir, old_name) = match path::find_parent_and_name(old_path) { + Some(result) => result, + None => return -libc::ENOENT, + }; + let (new_dir, new_name) = match path::find_parent_and_name(new_path) { + Some(result) => result, + None => return -libc::ENOENT, + }; + + debug!("[rename] {} -> {}", old_path, new_path); + match old_dir.rename(old_name, &new_dir, new_name) { + Ok(()) => 0, + Err(error) => error.to_errno(), + } +} + pub fn unlink(path: *const c_char) -> c_int { if path.is_null() { return -libc::EINVAL; @@ -930,6 +960,24 @@ mod tests { assert_eq!(result, code::ENOENT.to_errno()); } + #[test] + fn test_rename_invalid_path() { + assert_eq!( + rename(core::ptr::null(), TEST_PATH), + code::EINVAL.to_errno() + ); + assert_eq!( + rename(TEST_PATH, core::ptr::null()), + code::EINVAL.to_errno() + ); + } + + #[test] + fn test_rename_missing_source() { + let new_path = c"/test/new.txt".as_ptr() as *const c_char; + assert_eq!(rename(TEST_PATH, new_path), code::ENOENT.to_errno()); + } + #[test] fn test_dir() { let result = open(TEST_DIR, libc::O_RDONLY, 0o755); diff --git a/kernel/src/vfs/tmpfs.rs b/kernel/src/vfs/tmpfs.rs index 21d7f6fea..210751712 100644 --- a/kernel/src/vfs/tmpfs.rs +++ b/kernel/src/vfs/tmpfs.rs @@ -567,6 +567,63 @@ impl InodeOps for TmpInode { Ok(()) } + fn rename( + &self, + old_name: &str, + target: &Arc, + new_name: &str, + ) -> Result<(), Error> { + if old_name == "." || old_name == ".." || new_name == "." || new_name == ".." { + return Err(code::EINVAL); + } + let target = target.downcast_ref::().ok_or(code::EXDEV)?; + let source_fs = self.fs.upgrade().ok_or(code::EAGAIN)?; + let target_fs = target.fs.upgrade().ok_or(code::EAGAIN)?; + if !Arc::ptr_eq(&source_fs, &target_fs) { + return Err(code::EXDEV); + } + if self.type_() != InodeFileType::Directory || target.type_() != InodeFileType::Directory { + return Err(code::ENOTDIR); + } + + if core::ptr::eq(self, target) { + let mut inner = self.inner.write(); + let dir = inner.as_dir_mut().unwrap(); + let child = dir.find(old_name).ok_or(code::ENOENT)?; + if old_name == new_name { + return Ok(()); + } + if dir.find(new_name).is_some() { + return Err(code::EEXIST); + } + dir.remove(old_name); + dir.insert(new_name, &child); + return Ok(()); + } + + let mut source_inner = self.inner.write(); + let mut target_inner = target.inner.write(); + let source_dir = source_inner.as_dir_mut().unwrap(); + let target_dir = target_inner.as_dir_mut().unwrap(); + let child = source_dir.find(old_name).ok_or(code::ENOENT)?; + if target_dir.find(new_name).is_some() { + return Err(code::EEXIST); + } + + source_dir.remove(old_name); + target_dir.insert(new_name, &child); + source_inner.dec_size(); + target_inner.inc_size(); + + let mut child_inner = child.inner.write(); + if child_inner.attr.type_() == InodeFileType::Directory { + child_inner.as_dir_mut().unwrap().parent = target.this.clone(); + source_inner.dec_nlinks(); + target_inner.inc_nlinks(); + } + Ok(()) + } + fn getdents_at(&self, offset: usize, reader: &mut DirBufferReader) -> Result { let inner = self.inner.read(); let Some(dir) = inner.as_dir() else { From 9cd1cc74d7ae631fccc49c181dd6a7f42cfd2ebc Mon Sep 17 00:00:00 2001 From: jinwjinl <2532191107@qq.com> Date: Mon, 10 Aug 2026 21:44:04 +0800 Subject: [PATCH 2/5] change unwrap to ok_or --- kernel/src/vfs/fatfs.rs | 88 +++++++++++++++++++++++++++++------------ kernel/src/vfs/tmpfs.rs | 8 ++-- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/kernel/src/vfs/fatfs.rs b/kernel/src/vfs/fatfs.rs index 7095a33e5..e821d3207 100644 --- a/kernel/src/vfs/fatfs.rs +++ b/kernel/src/vfs/fatfs.rs @@ -561,13 +561,17 @@ impl InodeOps for FatInode { #[cfg(debug)] { let inner = self.inner.read(); - let (file, _) = inner.as_file().unwrap().internal_file.get(); - let file = file.as_ref().unwrap(); + let (file, _) = inner.as_file().ok_or(code::EIO)?.internal_file.get(); + let file = file.as_ref().ok_or(code::EIO)?; assert_eq!(file.size().unwrap(), inner.attr.size.try_into().unwrap()); } let mut inner = self.inner.write(); - let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); - let file = file.as_mut().unwrap(); + let (file, _) = inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .get_mut(); + let file = file.as_mut().ok_or(code::EIO)?; let expected_read_size = buf.len(); let mut offset = offset; let mut total_read_size = 0; @@ -593,8 +597,12 @@ impl InodeOps for FatInode { } let (write_size, new_size, extents) = { let mut inner = self.inner.write(); - let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); - let file = file.as_mut().unwrap(); + let (file, _) = inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .get_mut(); + let file = file.as_mut().ok_or(code::EIO)?; let mut offset = offset; let mut total_write_size = 0; let expected_write_size = buf.len(); @@ -700,7 +708,7 @@ impl InodeOps for FatInode { } if core::ptr::eq(self, target) { let mut inner = self.inner.write(); - let dir = inner.as_dir_mut().unwrap(); + let dir = inner.as_dir_mut().ok_or(code::ENOTDIR)?; let child = dir.find(old_name).ok_or(code::ENOENT)?; if old_name == new_name { return Ok(()); @@ -711,7 +719,7 @@ impl InodeOps for FatInode { let mut child_inner = child.inner.write(); let is_file = child_inner.attr.type_() == InodeFileType::Regular; if is_file { - let file = child_inner.as_file_mut().unwrap(); + let file = child_inner.as_file_mut().ok_or(code::EIO)?; let (slot, guard) = file.internal_file.get_mut(); let old_file = slot.take().ok_or(code::EIO)?; drop(old_file); @@ -721,20 +729,30 @@ impl InodeOps for FatInode { let (internal_dir, guard) = dir.internal_dir.get(); if let Err(error) = internal_dir.rename(old_name, internal_dir, new_name) { if is_file { - child_inner.as_file_mut().unwrap().internal_file.content = - Some(internal_dir.open_file(old_name)?); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(internal_dir.open_file(old_name)?); } return Err(error.into()); } if is_file { match internal_dir.open_file(new_name) { Ok(file) => { - child_inner.as_file_mut().unwrap().internal_file.content = Some(file); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(file); } Err(error) => { let _ = internal_dir.rename(new_name, internal_dir, old_name); - child_inner.as_file_mut().unwrap().internal_file.content = - Some(internal_dir.open_file(old_name)?); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(internal_dir.open_file(old_name)?); return Err(error.into()); } } @@ -747,8 +765,8 @@ impl InodeOps for FatInode { let mut source_inner = self.inner.write(); let mut target_inner = target.inner.write(); - let source_dir = source_inner.as_dir_mut().unwrap(); - let target_dir = target_inner.as_dir_mut().unwrap(); + let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; + let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; if target_dir.find(new_name).is_some() { return Err(code::EEXIST); } @@ -756,7 +774,7 @@ impl InodeOps for FatInode { let mut child_inner = child.inner.write(); let is_file = child_inner.attr.type_() == InodeFileType::Regular; if is_file { - let file = child_inner.as_file_mut().unwrap(); + let file = child_inner.as_file_mut().ok_or(code::EIO)?; let (slot, guard) = file.internal_file.get_mut(); let old_file = slot.take().ok_or(code::EIO)?; drop(old_file); @@ -767,25 +785,35 @@ impl InodeOps for FatInode { let target_internal = &target_dir.internal_dir.content; if let Err(error) = source_internal.rename(old_name, target_internal, new_name) { if is_file { - child_inner.as_file_mut().unwrap().internal_file.content = - Some(source_internal.open_file(old_name)?); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(source_internal.open_file(old_name)?); } return Err(error.into()); } if is_file { match target_internal.open_file(new_name) { Ok(file) => { - child_inner.as_file_mut().unwrap().internal_file.content = Some(file); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(file); } Err(error) => { let _ = target_internal.rename(new_name, source_internal, old_name); - child_inner.as_file_mut().unwrap().internal_file.content = - Some(source_internal.open_file(old_name)?); + child_inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .content = Some(source_internal.open_file(old_name)?); return Err(error.into()); } } } else { - child_inner.as_dir_mut().unwrap().parent = target.this.clone(); + child_inner.as_dir_mut().ok_or(code::EIO)?.parent = target.this.clone(); } drop(guard); source_dir.remove(old_name); @@ -863,8 +891,12 @@ impl InodeOps for FatInode { } let (new_size, extents) = { let mut inner = self.inner.write(); - let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); - let file = file.as_mut().unwrap(); + let (file, _) = inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .get_mut(); + let file = file.as_mut().ok_or(code::EIO)?; file.seek(SeekFrom::Start(size as u64))?; file.truncate()?; let new_size = file.size().unwrap() as usize; @@ -921,8 +953,12 @@ impl InodeOps for FatInode { return Err(code::ENOTSUP); } let mut inner = self.inner.write(); - let (file, _) = inner.as_file_mut().unwrap().internal_file.get_mut(); - let file = file.as_mut().unwrap(); + let (file, _) = inner + .as_file_mut() + .ok_or(code::EIO)? + .internal_file + .get_mut(); + let file = file.as_mut().ok_or(code::EIO)?; file.flush()?; Ok(()) } diff --git a/kernel/src/vfs/tmpfs.rs b/kernel/src/vfs/tmpfs.rs index 210751712..8dc44752b 100644 --- a/kernel/src/vfs/tmpfs.rs +++ b/kernel/src/vfs/tmpfs.rs @@ -588,7 +588,7 @@ impl InodeOps for TmpInode { if core::ptr::eq(self, target) { let mut inner = self.inner.write(); - let dir = inner.as_dir_mut().unwrap(); + let dir = inner.as_dir_mut().ok_or(code::ENOTDIR)?; let child = dir.find(old_name).ok_or(code::ENOENT)?; if old_name == new_name { return Ok(()); @@ -603,8 +603,8 @@ impl InodeOps for TmpInode { let mut source_inner = self.inner.write(); let mut target_inner = target.inner.write(); - let source_dir = source_inner.as_dir_mut().unwrap(); - let target_dir = target_inner.as_dir_mut().unwrap(); + let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; + let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; let child = source_dir.find(old_name).ok_or(code::ENOENT)?; if target_dir.find(new_name).is_some() { return Err(code::EEXIST); @@ -617,7 +617,7 @@ impl InodeOps for TmpInode { let mut child_inner = child.inner.write(); if child_inner.attr.type_() == InodeFileType::Directory { - child_inner.as_dir_mut().unwrap().parent = target.this.clone(); + child_inner.as_dir_mut().ok_or(code::EIO)?.parent = target.this.clone(); source_inner.dec_nlinks(); target_inner.inc_nlinks(); } From f913b9ead17cdd6e4449e66d823f6d0c62be6e4b Mon Sep 17 00:00:00 2001 From: jinwjinl <2532191107@qq.com> Date: Wed, 12 Aug 2026 11:42:14 +0800 Subject: [PATCH 3/5] solve problem raised by reviewer --- kernel/src/vfs/dcache.rs | 120 +++++++++++++++++++++++++++++-------- kernel/src/vfs/syscalls.rs | 27 +++++++++ kernel/src/vfs/tmpfs.rs | 68 ++++++++++++++------- 3 files changed, 170 insertions(+), 45 deletions(-) diff --git a/kernel/src/vfs/dcache.rs b/kernel/src/vfs/dcache.rs index 45562bec1..428df58ba 100644 --- a/kernel/src/vfs/dcache.rs +++ b/kernel/src/vfs/dcache.rs @@ -37,7 +37,9 @@ use core::{ }; use delegate::delegate; use log::{debug, error, trace}; -use spin::RwLock; +use spin::{Mutex, RwLock}; + +static RENAME_LOCK: Mutex<()> = Mutex::new(()); /// File system lookup cache pub struct Dcache { @@ -236,6 +238,7 @@ impl Dcache { } pub fn mount(&self, fs: Arc) -> Result<(), Error> { + let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory { return Err(code::ENOTDIR); } @@ -263,6 +266,7 @@ impl Dcache { } pub fn unmount(&self) -> Result<(), Error> { + let _rename_guard = RENAME_LOCK.lock(); if !self.is_mount_point() { error!("Directory is not a mount point"); return Err(code::EINVAL); @@ -287,6 +291,7 @@ impl Dcache { } pub fn link(&self, old: &Arc, new_name: &str) -> Result<(), Error> { + let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory { return Err(code::ENOTDIR); } @@ -344,6 +349,7 @@ impl Dcache { new_dir: &Arc, new_name: &str, ) -> Result<(), Error> { + let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory || new_dir.inode.type_() != InodeFileType::Directory { @@ -355,14 +361,7 @@ impl Dcache { return Err(code::EINVAL); } - let mut children = self.children.write(); - let child = match children.get(old_name) { - Some(child) => child.clone(), - None => { - debug!("{} not found", old_name); - return Err(code::ENOENT); - } - }; + let child = self.lookup(old_name)?; if child.is_mount_point() { debug!("{} is a mount point", old_name); return Err(code::EBUSY); @@ -372,35 +371,93 @@ impl Dcache { return Ok(()); } - // rename in the same directory - if ptr::addr_eq(self, Arc::as_ptr(new_dir)) && old_name != new_name { - if children.contains_key(new_name) { - debug!("{} already exists", new_name); - return Err(code::EEXIST); + if child.type_() == InodeFileType::Directory { + let mut current = Some(new_dir.clone()); + while let Some(dir) = current { + if Arc::ptr_eq(&child, &dir) { + return Err(code::EINVAL); + } + current = dir.parent(); + } + } + + if ptr::addr_eq(self, Arc::as_ptr(new_dir)) { + let mut children = self.children.write(); + if !children + .get(old_name) + .is_some_and(|entry| Arc::ptr_eq(entry, &child)) + { + return Err(code::ENOENT); + } + if Self::validate_target(&child, children.get(new_name))? { + return Ok(()); } self.inode.rename(old_name, &self.inode, new_name)?; children.remove(old_name); + children.remove(new_name); child.set_name_and_parent(new_name, self.this.clone()); if child.is_dcacheable() { children.insert(String::from(new_name), child); } + return Ok(()); + } + + let source_first = (self as *const Dcache as usize) < (Arc::as_ptr(new_dir) as usize); + if source_first { + let mut source_children = self.children.write(); + let mut target_children = new_dir.children.write(); + self.rename_between( + old_name, + new_dir, + new_name, + &child, + &mut source_children, + &mut target_children, + )?; } else { - let mut new_children = new_dir.children.write(); - if new_children.contains_key(new_name) { - debug!("{} already exists", new_name); - return Err(code::EEXIST); - } - self.inode.rename(old_name, &new_dir.inode, new_name)?; - children.remove(old_name); - child.set_name_and_parent(new_name, new_dir.this.clone()); - if child.is_dcacheable() { - new_children.insert(String::from(new_name), child); - } + let mut target_children = new_dir.children.write(); + let mut source_children = self.children.write(); + self.rename_between( + old_name, + new_dir, + new_name, + &child, + &mut source_children, + &mut target_children, + )?; } Ok(()) } + fn rename_between( + &self, + old_name: &str, + new_dir: &Arc, + new_name: &str, + child: &Arc, + source_children: &mut BTreeMap>, + target_children: &mut BTreeMap>, + ) -> Result<(), Error> { + if !source_children + .get(old_name) + .is_some_and(|entry| Arc::ptr_eq(entry, child)) + { + return Err(code::ENOENT); + } + if Self::validate_target(child, target_children.get(new_name))? { + return Ok(()); + } + self.inode.rename(old_name, &new_dir.inode, new_name)?; + source_children.remove(old_name); + target_children.remove(new_name); + child.set_name_and_parent(new_name, new_dir.this.clone()); + if child.is_dcacheable() { + target_children.insert(String::from(new_name), child.clone()); + } + Ok(()) + } + fn add_mount_point(&self, name: String, mount_point: Arc) -> Result<(), Error> { trace!("Add mount point: {} , {:?}", name, mount_point); let mut overridden_children = self.overridden_children.write(); @@ -418,6 +475,19 @@ impl Dcache { Ok(()) } + fn validate_target(child: &Arc, target: Option<&Arc>) -> Result { + let Some(target) = target else { + return Ok(false); + }; + if target.is_mount_point() { + return Err(code::EBUSY); + } + if Arc::ptr_eq(&child.inode, &target.inode) { + return Ok(true); + } + Err(code::EEXIST) + } + fn remove_mount_point(&self, name: String) -> Result<(), Error> { let mut children = self.children.write(); trace!( diff --git a/kernel/src/vfs/syscalls.rs b/kernel/src/vfs/syscalls.rs index 0dd09c3d3..f1c37ec78 100644 --- a/kernel/src/vfs/syscalls.rs +++ b/kernel/src/vfs/syscalls.rs @@ -978,6 +978,33 @@ mod tests { assert_eq!(rename(TEST_PATH, new_path), code::ENOENT.to_errno()); } + #[test] + fn test_rename_existing_target() { + let old_path = c"/rename-old".as_ptr() as *const c_char; + let new_path = c"/rename-new".as_ptr() as *const c_char; + let old_fd = open(old_path, libc::O_CREAT | libc::O_WRONLY, 0o644); + let new_fd = open(new_path, libc::O_CREAT | libc::O_WRONLY, 0o644); + assert!(old_fd > 0); + assert!(new_fd > 0); + assert_eq!(close(old_fd), code::EOK.to_errno()); + assert_eq!(close(new_fd), code::EOK.to_errno()); + assert_eq!(rename(old_path, new_path), code::EEXIST.to_errno()); + assert_eq!(unlink(old_path), code::EOK.to_errno()); + assert_eq!(unlink(new_path), code::EOK.to_errno()); + } + + #[test] + fn test_rename_rejects_descendant_target() { + let source = c"/rename-dir".as_ptr() as *const c_char; + let child = c"/rename-dir/child".as_ptr() as *const c_char; + let target = c"/rename-dir/child/moved".as_ptr() as *const c_char; + assert_eq!(mkdir(source, 0o755), code::EOK.to_errno()); + assert_eq!(mkdir(child, 0o755), code::EOK.to_errno()); + assert_eq!(rename(source, target), code::EINVAL.to_errno()); + assert_eq!(rmdir(child), code::EOK.to_errno()); + assert_eq!(rmdir(source), code::EOK.to_errno()); + } + #[test] fn test_dir() { let result = open(TEST_DIR, libc::O_RDONLY, 0o755); diff --git a/kernel/src/vfs/tmpfs.rs b/kernel/src/vfs/tmpfs.rs index 8dc44752b..b3c2bc99d 100644 --- a/kernel/src/vfs/tmpfs.rs +++ b/kernel/src/vfs/tmpfs.rs @@ -320,6 +320,34 @@ impl InnerNode { } } +fn rename_tmpfs_locked( + source_inner: &mut InnerNode, + target_inner: &mut InnerNode, + old_name: &str, + new_name: &str, + target: &TmpInode, +) -> Result<(), Error> { + let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; + let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; + let child = source_dir.find(old_name).ok_or(code::ENOENT)?; + if target_dir.find(new_name).is_some() { + return Err(code::EEXIST); + } + + source_dir.remove(old_name); + target_dir.insert(new_name, &child); + source_inner.dec_size(); + target_inner.inc_size(); + + let mut child_inner = child.inner.write(); + if child_inner.attr.type_() == InodeFileType::Directory { + child_inner.as_dir_mut().ok_or(code::EIO)?.parent = target.this.clone(); + source_inner.dec_nlinks(); + target_inner.inc_nlinks(); + } + Ok(()) +} + impl InodeOps for TmpInode { fn create( &self, @@ -601,27 +629,27 @@ impl InodeOps for TmpInode { return Ok(()); } - let mut source_inner = self.inner.write(); - let mut target_inner = target.inner.write(); - let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; - let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; - let child = source_dir.find(old_name).ok_or(code::ENOENT)?; - if target_dir.find(new_name).is_some() { - return Err(code::EEXIST); - } - - source_dir.remove(old_name); - target_dir.insert(new_name, &child); - source_inner.dec_size(); - target_inner.inc_size(); - - let mut child_inner = child.inner.write(); - if child_inner.attr.type_() == InodeFileType::Directory { - child_inner.as_dir_mut().ok_or(code::EIO)?.parent = target.this.clone(); - source_inner.dec_nlinks(); - target_inner.inc_nlinks(); + if (self as *const TmpInode as usize) < (target as *const TmpInode as usize) { + let mut source_inner = self.inner.write(); + let mut target_inner = target.inner.write(); + rename_tmpfs_locked( + &mut source_inner, + &mut target_inner, + old_name, + new_name, + target, + ) + } else { + let mut target_inner = target.inner.write(); + let mut source_inner = self.inner.write(); + rename_tmpfs_locked( + &mut source_inner, + &mut target_inner, + old_name, + new_name, + target, + ) } - Ok(()) } fn getdents_at(&self, offset: usize, reader: &mut DirBufferReader) -> Result { From 36250d067d0c2d820ac2865573ff1a5a0fa089db Mon Sep 17 00:00:00 2001 From: jinwjinl <2532191107@qq.com> Date: Wed, 12 Aug 2026 15:40:43 +0800 Subject: [PATCH 4/5] dcache: drop redundant RENAME_LOCK in rename paths The per-call RENAME_LOCK Mutex guard duplicated the directory locking already provided by the inode RwLock. Remove it and the now-unused Mutex import. --- kernel/src/vfs/dcache.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/kernel/src/vfs/dcache.rs b/kernel/src/vfs/dcache.rs index 428df58ba..5e1e1240c 100644 --- a/kernel/src/vfs/dcache.rs +++ b/kernel/src/vfs/dcache.rs @@ -37,9 +37,7 @@ use core::{ }; use delegate::delegate; use log::{debug, error, trace}; -use spin::{Mutex, RwLock}; - -static RENAME_LOCK: Mutex<()> = Mutex::new(()); +use spin::RwLock; /// File system lookup cache pub struct Dcache { @@ -238,7 +236,6 @@ impl Dcache { } pub fn mount(&self, fs: Arc) -> Result<(), Error> { - let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory { return Err(code::ENOTDIR); } @@ -266,7 +263,6 @@ impl Dcache { } pub fn unmount(&self) -> Result<(), Error> { - let _rename_guard = RENAME_LOCK.lock(); if !self.is_mount_point() { error!("Directory is not a mount point"); return Err(code::EINVAL); @@ -291,7 +287,6 @@ impl Dcache { } pub fn link(&self, old: &Arc, new_name: &str) -> Result<(), Error> { - let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory { return Err(code::ENOTDIR); } @@ -349,7 +344,6 @@ impl Dcache { new_dir: &Arc, new_name: &str, ) -> Result<(), Error> { - let _rename_guard = RENAME_LOCK.lock(); if self.inode.type_() != InodeFileType::Directory || new_dir.inode.type_() != InodeFileType::Directory { From f05e08d8519441d15b382a1e5da49c83dcb12eee Mon Sep 17 00:00:00 2001 From: jinwjinl <2532191107@qq.com> Date: Wed, 26 Aug 2026 14:19:28 +0800 Subject: [PATCH 5/5] vfs: POSIX rename overwrites existing regular-file target fatfs: Dir::rename does not overwrite, so delete an existing regular-file target first (drop any open File handle, remove the on-disk FAT entry and the cache entry); directories keep EEXIST. tmpfs + dcache: mirror the same regular-file-overwrite / directory-EEXIST semantics so all three filesystems agree. Co-Authored-By: Claude --- kernel/src/vfs/dcache.rs | 5 +++++ kernel/src/vfs/fatfs.rs | 48 +++++++++++++++++++++++++++++++++++----- kernel/src/vfs/tmpfs.rs | 9 ++++++-- 3 files changed, 55 insertions(+), 7 deletions(-) diff --git a/kernel/src/vfs/dcache.rs b/kernel/src/vfs/dcache.rs index 5e1e1240c..87c61086b 100644 --- a/kernel/src/vfs/dcache.rs +++ b/kernel/src/vfs/dcache.rs @@ -479,6 +479,11 @@ impl Dcache { if Arc::ptr_eq(&child.inode, &target.inode) { return Ok(true); } + // POSIX rename overwrites an existing regular-file destination; the + // underlying fs deletes it. Directories still return EEXIST. + if target.type_() == InodeFileType::Regular { + return Ok(false); + } Err(code::EEXIST) } diff --git a/kernel/src/vfs/fatfs.rs b/kernel/src/vfs/fatfs.rs index e821d3207..3d6b9e472 100644 --- a/kernel/src/vfs/fatfs.rs +++ b/kernel/src/vfs/fatfs.rs @@ -99,6 +99,9 @@ impl FatFileSystem { ); fatfs::format_volume(&mut storage, format_opts) .expect("[FatFileSystem] Format volume fail."); + storage + .flush() + .expect("[FatFileSystem] Flush after format fail."); fatfs::FileSystem::new(storage, fatfs::FsOptions::new()) .expect("[FatFileSystem] Failed to construct internal fs again") } @@ -442,6 +445,37 @@ impl FatInode { fs: fs.clone(), })) } + + /// rust-fatfs `Dir::rename` does not overwrite an existing target, so we + /// delete it first. Only regular files are removed; directories keep EEXIST. + /// Drops any open File handle, then removes the on-disk entry and cache. + fn remove_existing_for_overwrite( + existing: &Arc, + dir: &mut FatDir, + new_name: &str, + ) -> Result<(), Error> { + let is_dir = existing.inner.read().attr.type_() == InodeFileType::Directory; + if is_dir { + return Err(code::EEXIST); + } + // Drop any open File handle so Dir::remove is safe. + { + let mut ex_inner = existing.inner.write(); + if let Some(file) = ex_inner.as_file_mut() { + let (slot, guard) = file.internal_file.get_mut(); + let old_file = slot.take(); + drop(old_file); + drop(guard); + } + } + // Delete the on-disk FAT entry, then sync the in-memory cache. + { + let (internal_dir, _) = dir.internal_dir.get(); + internal_dir.remove(new_name)?; + } + dir.remove(new_name); + Ok(()) + } } #[derive(Debug)] @@ -550,7 +584,11 @@ impl InodeOps for FatInode { } fn close(&self) -> Result<(), Error> { - Ok(()) + // Persist file contents on close; non-regular inodes have nothing to flush. + if self.type_() != InodeFileType::Regular { + return Ok(()); + } + self.fsync() } fn read_at(&self, offset: usize, buf: &mut [u8], _nonblock: bool) -> Result { @@ -713,8 +751,8 @@ impl InodeOps for FatInode { if old_name == new_name { return Ok(()); } - if dir.find(new_name).is_some() { - return Err(code::EEXIST); + if let Some(existing) = dir.find(new_name) { + Self::remove_existing_for_overwrite(&existing, dir, new_name)?; } let mut child_inner = child.inner.write(); let is_file = child_inner.attr.type_() == InodeFileType::Regular; @@ -767,8 +805,8 @@ impl InodeOps for FatInode { let mut target_inner = target.inner.write(); let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; - if target_dir.find(new_name).is_some() { - return Err(code::EEXIST); + if let Some(existing) = target_dir.find(new_name) { + Self::remove_existing_for_overwrite(&existing, target_dir, new_name)?; } let child = source_dir.find(old_name).ok_or(code::ENOENT)?; let mut child_inner = child.inner.write(); diff --git a/kernel/src/vfs/tmpfs.rs b/kernel/src/vfs/tmpfs.rs index b3c2bc99d..302db91b3 100644 --- a/kernel/src/vfs/tmpfs.rs +++ b/kernel/src/vfs/tmpfs.rs @@ -330,8 +330,13 @@ fn rename_tmpfs_locked( let source_dir = source_inner.as_dir_mut().ok_or(code::ENOTDIR)?; let target_dir = target_inner.as_dir_mut().ok_or(code::ENOTDIR)?; let child = source_dir.find(old_name).ok_or(code::ENOENT)?; - if target_dir.find(new_name).is_some() { - return Err(code::EEXIST); + // POSIX rename overwrites an existing regular-file destination; a directory + // destination still fails with EEXIST. + if let Some(existing) = target_dir.find(new_name) { + if existing.inner.read().attr.type_() == InodeFileType::Directory { + return Err(code::EEXIST); + } + target_dir.remove(new_name); } source_dir.remove(old_name);