From 68aebe4175a2704c0b6e2126dd57d57f8edb51c1 Mon Sep 17 00:00:00 2001 From: LI Qing Date: Thu, 23 May 2024 13:50:33 +0800 Subject: [PATCH] Fix the potential deadlock issue of Ext2 --- kernel/aster-nix/src/fs/ext2/inode.rs | 379 +++++++++++++++++--------- 1 file changed, 249 insertions(+), 130 deletions(-) diff --git a/kernel/aster-nix/src/fs/ext2/inode.rs b/kernel/aster-nix/src/fs/ext2/inode.rs index d7d4eac0..1397f7a7 100644 --- a/kernel/aster-nix/src/fs/ext2/inode.rs +++ b/kernel/aster-nix/src/fs/ext2/inode.rs @@ -3,6 +3,8 @@ #![allow(dead_code)] #![allow(unused_variables)] +use alloc::rc::Rc; + use inherit_methods_macro::inherit_methods; use super::{ @@ -79,6 +81,10 @@ impl Inode { file_type: FileType, file_perm: FilePerm, ) -> Result> { + if name.len() > MAX_FNAME_LEN { + return_errno!(Errno::ENAMETOOLONG); + } + let inner = self.inner.upread(); if inner.file_type() != FileType::Dir { return_errno!(Errno::ENOTDIR); @@ -86,11 +92,7 @@ impl Inode { if inner.hard_links() == 0 { return_errno_with_message!(Errno::ENOENT, "dir removed"); } - if name.len() > MAX_FNAME_LEN { - return_errno!(Errno::ENAMETOOLONG); - } - - if inner.get_entry(name, 0).is_some() { + if inner.get_entry(name).is_some() { return_errno!(Errno::EEXIST); } @@ -105,7 +107,7 @@ impl Inode { let new_entry = DirEntry::new(inode.ino, name, file_type); let mut inner = inner.upgrade(); - if let Err(e) = inner.append_entry(new_entry, 0) { + if let Err(e) = inner.append_entry(new_entry) { self.fs().free_inode(inode.ino, is_dir).unwrap(); return Err(e); } @@ -113,6 +115,10 @@ impl Inode { } pub fn lookup(&self, name: &str) -> Result> { + if name.len() > MAX_FNAME_LEN { + return_errno!(Errno::ENAMETOOLONG); + } + let inner = self.inner.read(); if inner.file_type() != FileType::Dir { return_errno!(Errno::ENOTDIR); @@ -120,18 +126,17 @@ impl Inode { if inner.hard_links() == 0 { return_errno_with_message!(Errno::ENOENT, "dir removed"); } - if name.len() > MAX_FNAME_LEN { - return_errno!(Errno::ENAMETOOLONG); - } - let ino = inner - .get_entry_ino(name, 0) - .ok_or(Error::new(Errno::ENOENT))?; + let ino = inner.get_entry_ino(name).ok_or(Error::new(Errno::ENOENT))?; drop(inner); self.fs().lookup_inode(ino) } pub fn link(&self, inode: &Inode, name: &str) -> Result<()> { + if name.len() > MAX_FNAME_LEN { + return_errno!(Errno::ENAMETOOLONG); + } + let inner = self.inner.upread(); if inner.file_type() != FileType::Dir { return_errno!(Errno::ENOTDIR); @@ -139,21 +144,19 @@ impl Inode { if inner.hard_links() == 0 { return_errno_with_message!(Errno::ENOENT, "dir removed"); } - if name.len() > MAX_FNAME_LEN { - return_errno!(Errno::ENAMETOOLONG); - } + let inode_type = inode.file_type(); if inode_type == FileType::Dir { return_errno!(Errno::EPERM); } - if inner.get_entry(name, 0).is_some() { + if inner.get_entry(name).is_some() { return_errno!(Errno::EEXIST); } let new_entry = DirEntry::new(inode.ino, name, inode_type); let mut inner = inner.upgrade(); - inner.append_entry(new_entry, 0)?; + inner.append_entry(new_entry)?; drop(inner); inode.inner.write().inc_hard_links(); @@ -161,65 +164,71 @@ impl Inode { } pub fn unlink(&self, name: &str) -> Result<()> { - let inner = self.inner.upread(); - if inner.file_type() != FileType::Dir { - return_errno!(Errno::ENOTDIR); - } - if inner.hard_links() == 0 { - return_errno_with_message!(Errno::ENOENT, "dir removed"); - } if name == "." || name == ".." { return_errno!(Errno::EISDIR); } - if name.len() > MAX_FNAME_LEN { - return_errno!(Errno::ENAMETOOLONG); - } - let inode = { - let ino = inner - .get_entry_ino(name, 0) - .ok_or(Error::new(Errno::ENOENT))?; - self.fs().lookup_inode(ino)? - }; - if inode.file_type() == FileType::Dir { + let file = self.lookup(name)?; + if file.file_type() == FileType::Dir { return_errno!(Errno::EISDIR); } - let mut inner = inner.upgrade(); - inner.remove_entry(name, 0)?; - drop(inner); + let (mut self_inner, mut file_inner) = write_lock_two_inodes(self, &file); + // When we got the lock, the dir may have been modified by another thread + if self_inner.hard_links() == 0 { + return_errno_with_message!(Errno::ENOENT, "dir removed"); + } + let (offset, new_ino) = self_inner + .get_entry(name) + .map(|(offset, entry)| (offset, entry.ino())) + .ok_or(Error::new(Errno::ENOENT))?; + if file.ino != new_ino { + return_errno!(Errno::ENOENT); + } + let potential_new_file = self.fs().lookup_inode(file.ino)?; + if !Arc::ptr_eq(&file, &potential_new_file) { + return_errno!(Errno::ENOENT); + } - inode.inner.write().dec_hard_links(); + self_inner.remove_entry_at(name, offset)?; + file_inner.dec_hard_links(); Ok(()) } pub fn rmdir(&self, name: &str) -> Result<()> { - let self_inner = self.inner.upread(); - if self_inner.file_type() != FileType::Dir { - return_errno!(Errno::ENOTDIR); - } - if self_inner.hard_links() == 0 { - return_errno_with_message!(Errno::ENOENT, "dir removed"); - } if name == "." { return_errno_with_message!(Errno::EINVAL, "rmdir on ."); } if name == ".." { return_errno_with_message!(Errno::ENOTEMPTY, "rmdir on .."); } - if name.len() > MAX_FNAME_LEN { - return_errno!(Errno::ENAMETOOLONG); + + let dir_inode = self.lookup(name)?; + let dir_inner = dir_inode.inner.read(); + if dir_inner.file_type() != FileType::Dir { + return_errno!(Errno::ENOTDIR); } + if dir_inner.entry_count() > 2 { + return_errno!(Errno::ENOTEMPTY); + } + drop(dir_inner); - let dir_inode = { - let ino = self_inner - .get_entry_ino(name, 0) - .ok_or(Error::new(Errno::ENOENT))?; - self.fs().lookup_inode(ino)? - }; - - // FIXME: There may be a deadlock here. - let dir_inner = dir_inode.inner.upread(); + let (mut self_inner, mut dir_inner) = write_lock_two_inodes(self, &dir_inode); + // When we got the lock, the dir may have been modified by another thread + if self_inner.hard_links() == 0 { + return_errno_with_message!(Errno::ENOENT, "dir removed"); + } + let (offset, new_ino) = self_inner + .get_entry(name) + .map(|(offset, entry)| (offset, entry.ino())) + .ok_or(Error::new(Errno::ENOENT))?; + if dir_inode.ino != new_ino { + return_errno!(Errno::ENOENT); + } + let potential_new_dir = self.fs().lookup_inode(dir_inode.ino)?; + if !Arc::ptr_eq(&dir_inode, &potential_new_dir) { + return_errno!(Errno::ENOENT); + } if dir_inner.file_type() != FileType::Dir { return_errno!(Errno::ENOTDIR); } @@ -227,11 +236,7 @@ impl Inode { return_errno!(Errno::ENOTEMPTY); } - let mut self_inner = self_inner.upgrade(); - self_inner.remove_entry(name, 0)?; - drop(self_inner); - - let mut dir_inner = dir_inner.upgrade(); + self_inner.remove_entry_at(name, offset)?; dir_inner.dec_hard_links(); dir_inner.dec_hard_links(); // For "." Ok(()) @@ -247,29 +252,57 @@ impl Inode { return_errno_with_message!(Errno::ENOENT, "dir removed"); } - let (src_offset, src_inode) = { + let fs = self.fs(); + let (src_offset, src_inode, src_inode_typ) = { let (offset, entry) = self_inner - .get_entry(old_name, 0) + .get_entry(old_name) .ok_or(Error::new(Errno::ENOENT))?; - (offset, self.fs().lookup_inode(entry.ino())?) + (offset, fs.lookup_inode(entry.ino())?, entry.type_()) }; - let Some((dst_offset, dst_entry)) = self_inner.get_entry(new_name, 0) else { + let Some(dst_ino) = self_inner.get_entry_ino(new_name) else { let mut self_inner = self_inner.upgrade(); - self_inner.rename_entry(old_name, new_name, src_offset)?; + self_inner.rename_entry_at(old_name, new_name, src_offset)?; return Ok(()); }; - - if src_inode.ino == dst_entry.ino() { + if src_inode.ino == dst_ino { // Same inode, do nothing return Ok(()); } + let dst_inode = fs.lookup_inode(dst_ino)?; + drop(self_inner); - let dst_inode = self.fs().lookup_inode(dst_entry.ino())?; - // FIXME: There may be a deadlock here. - let dst_inner = dst_inode.inner.upread(); - let dst_inode_type = dst_inner.file_type(); - match (src_inode.file_type(), dst_inode_type) { + let (mut self_inner, mut dst_inner) = write_lock_two_inodes(self, &dst_inode); + // When we got the lock, the dir may have been modified by another thread + if self_inner.hard_links() == 0 { + return_errno_with_message!(Errno::ENOENT, "dir removed"); + } + + let (src_offset, new_src_ino) = self_inner + .get_entry(old_name) + .map(|(offset, entry)| (offset, entry.ino())) + .ok_or(Error::new(Errno::ENOENT))?; + if src_inode.ino != new_src_ino { + return_errno!(Errno::ENOENT); + } + let potential_new_src = fs.lookup_inode(src_inode.ino)?; + if !Arc::ptr_eq(&src_inode, &potential_new_src) { + return_errno!(Errno::ENOENT); + } + + let (dst_offset, new_dst_entry) = self_inner + .get_entry(new_name) + .ok_or(Error::new(Errno::ENOENT))?; + if dst_inode.ino != new_dst_entry.ino() { + return_errno!(Errno::ENOENT); + } + let potential_new_dst = fs.lookup_inode(dst_inode.ino)?; + if !Arc::ptr_eq(&dst_inode, &potential_new_dst) { + return_errno!(Errno::ENOENT); + } + + let dst_inode_typ = new_dst_entry.type_(); + match (src_inode_typ, dst_inode_typ) { (FileType::Dir, FileType::Dir) => { if dst_inner.entry_count() > 2 { return_errno!(Errno::ENOTEMPTY); @@ -283,16 +316,11 @@ impl Inode { } _ => {} } - let dst_is_dir = dst_inode_type == FileType::Dir; - let mut self_inner = self_inner.upgrade(); - self_inner.remove_entry(new_name, dst_offset)?; - self_inner.rename_entry(old_name, new_name, src_offset)?; - drop(self_inner); - - let mut dst_inner = dst_inner.upgrade(); + self_inner.remove_entry_at(new_name, dst_offset)?; + self_inner.rename_entry_at(old_name, new_name, src_offset)?; dst_inner.dec_hard_links(); - if dst_is_dir { + if dst_inode_typ == FileType::Dir { dst_inner.dec_hard_links(); // For "." } @@ -312,9 +340,7 @@ impl Inode { return self.rename_within(old_name, new_name); } - // FIXME: There may be a deadlock here. - let self_inner = self.inner.upread(); - let target_inner = target.inner.upread(); + let (self_inner, target_inner) = read_lock_two_inodes(self, target); if self_inner.file_type() != FileType::Dir || target_inner.file_type() != FileType::Dir { return_errno!(Errno::ENOTDIR); } @@ -322,49 +348,108 @@ impl Inode { return_errno_with_message!(Errno::ENOENT, "dir removed"); } - let (src_offset, src_inode) = { + let fs = self.fs(); + let (src_offset, src_inode, src_inode_typ) = { let (offset, entry) = self_inner - .get_entry(old_name, 0) + .get_entry(old_name) .ok_or(Error::new(Errno::ENOENT))?; - (offset, self.fs().lookup_inode(entry.ino())?) + (offset, fs.lookup_inode(entry.ino())?, entry.type_()) }; // Avoid renaming a directory to a subdirectory of itself if src_inode.ino == target.ino { return_errno!(Errno::EINVAL); } - let src_inode_type = src_inode.file_type(); - let is_dir = src_inode_type == FileType::Dir; + let is_dir = src_inode_typ == FileType::Dir; - let Some((dst_offset, dst_entry)) = target_inner.get_entry(new_name, 0) else { - let mut self_inner = self_inner.upgrade(); - let mut target_inner = target_inner.upgrade(); - self_inner.remove_entry(old_name, src_offset)?; - let new_entry = DirEntry::new(src_inode.ino, new_name, src_inode_type); - target_inner.append_entry(new_entry, 0)?; + let Some(dst_ino) = target_inner.get_entry_ino(new_name) else { drop(self_inner); drop(target_inner); + let mut write_guards = if is_dir { + write_lock_multiple_inodes(vec![&src_inode, target, self]) + } else { + write_lock_multiple_inodes(vec![target, self]) + }; + + // When we got the lock, the dir may have been modified by another thread + let mut self_inner = write_guards.pop().unwrap(); + let mut target_inner = write_guards.pop().unwrap(); + if self_inner.hard_links() == 0 || target_inner.hard_links() == 0 { + return_errno_with_message!(Errno::ENOENT, "dir removed"); + } + let (src_offset, new_src_ino) = self_inner + .get_entry(old_name) + .map(|(offset, entry)| (offset, entry.ino())) + .ok_or(Error::new(Errno::ENOENT))?; + if src_inode.ino != new_src_ino { + return_errno!(Errno::ENOENT); + } + let potential_new_src = fs.lookup_inode(src_inode.ino)?; + if !Arc::ptr_eq(&src_inode, &potential_new_src) { + return_errno!(Errno::ENOENT); + } + + self_inner.remove_entry_at(old_name, src_offset)?; + let new_entry = DirEntry::new(src_inode.ino, new_name, src_inode_typ); + target_inner.append_entry(new_entry)?; + if is_dir { - src_inode.inner.write().set_parent_ino(target.ino)?; + let mut src_inner = write_guards.pop().unwrap(); + src_inner.set_parent_ino(target.ino)?; } return Ok(()); }; - - if src_inode.ino == dst_entry.ino() { + if src_inode.ino == dst_ino { // Same inode, do nothing return Ok(()); } - // Avoid renaming a subdirectory to a directory. - if self.ino == dst_entry.ino() { + if self.ino == dst_ino { return_errno!(Errno::ENOTEMPTY); } + let dst_inode = fs.lookup_inode(dst_ino)?; + drop(self_inner); + drop(target_inner); - let dst_inode = self.fs().lookup_inode(dst_entry.ino())?; - // FIXME: There may be a deadlock here. - let dst_inner = dst_inode.inner.upread(); - let dst_inode_type = dst_inner.file_type(); - match (src_inode_type, dst_inode_type) { + let mut write_guards = if is_dir { + write_lock_multiple_inodes(vec![&src_inode, &dst_inode, target, self]) + } else { + write_lock_multiple_inodes(vec![&dst_inode, target, self]) + }; + + // When we got the lock, the dir may have been modified by another thread + let mut self_inner = write_guards.pop().unwrap(); + let mut target_inner = write_guards.pop().unwrap(); + if self_inner.hard_links() == 0 || target_inner.hard_links() == 0 { + return_errno_with_message!(Errno::ENOENT, "dir removed"); + } + + let (src_offset, new_src_ino) = self_inner + .get_entry(old_name) + .map(|(offset, entry)| (offset, entry.ino())) + .ok_or(Error::new(Errno::ENOENT))?; + if src_inode.ino != new_src_ino { + return_errno!(Errno::ENOENT); + } + let potential_new_src = fs.lookup_inode(src_inode.ino)?; + if !Arc::ptr_eq(&src_inode, &potential_new_src) { + return_errno!(Errno::ENOENT); + } + + let (dst_offset, new_dst_entry) = target_inner + .get_entry(new_name) + .ok_or(Error::new(Errno::ENOENT))?; + if dst_inode.ino != new_dst_entry.ino() { + return_errno!(Errno::ENOENT); + } + let potential_new_dst = fs.lookup_inode(dst_inode.ino)?; + if !Arc::ptr_eq(&dst_inode, &potential_new_dst) { + return_errno!(Errno::ENOENT); + } + + let mut dst_inner = write_guards.pop().unwrap(); + let dst_inode_typ = new_dst_entry.type_(); + match (src_inode_typ, dst_inode_typ) { (FileType::Dir, FileType::Dir) => { if dst_inner.entry_count() > 2 { return_errno!(Errno::ENOTEMPTY); @@ -378,24 +463,16 @@ impl Inode { } _ => {} } - let mut self_inner = self_inner.upgrade(); - let mut target_inner = target_inner.upgrade(); - self_inner.remove_entry(old_name, src_offset)?; - target_inner.remove_entry(new_name, dst_offset)?; - let new_entry = DirEntry::new(src_inode.ino, new_name, src_inode_type); - target_inner.append_entry(new_entry, 0)?; - drop(self_inner); - drop(target_inner); - let mut dst_inner = dst_inner.upgrade(); + self_inner.remove_entry_at(old_name, src_offset)?; + target_inner.remove_entry_at(new_name, dst_offset)?; + let new_entry = DirEntry::new(src_inode.ino, new_name, src_inode_typ); + target_inner.append_entry(new_entry)?; dst_inner.dec_hard_links(); if is_dir { dst_inner.dec_hard_links(); // For "." - } - drop(dst_inner); - - if is_dir { - src_inode.inner.write().set_parent_ino(target.ino)?; + let mut src_inner = write_guards.pop().unwrap(); + src_inner.set_parent_ino(target.ino)?; } Ok(()) @@ -583,6 +660,48 @@ impl Debug for Inode { } } +fn read_lock_two_inodes<'a>( + this: &'a Inode, + other: &'a Inode, +) -> (RwMutexReadGuard<'a, Inner>, RwMutexReadGuard<'a, Inner>) { + if this.ino < other.ino { + let this = this.inner.read(); + let other = other.inner.read(); + (this, other) + } else { + let other = other.inner.read(); + let this = this.inner.read(); + (this, other) + } +} + +fn write_lock_two_inodes<'a>( + this: &'a Inode, + other: &'a Inode, +) -> (RwMutexWriteGuard<'a, Inner>, RwMutexWriteGuard<'a, Inner>) { + let mut write_guards = write_lock_multiple_inodes(vec![this, other]); + let other_guard = write_guards.pop().unwrap(); + let this_guard = write_guards.pop().unwrap(); + (this_guard, other_guard) +} + +fn write_lock_multiple_inodes(inodes: Vec<&Inode>) -> Vec> { + // Record the index information of the input + let mut ordered_inodes: Vec<(usize, &Inode)> = inodes.into_iter().enumerate().collect(); + // Sort in ascending order of ino + ordered_inodes.sort_unstable_by_key(|&(_, inode)| inode.ino); + // Acquire the guards in order, and record by the input index. + // This ensures that the output order is consistent with the input. + let mut guards = vec![None; ordered_inodes.len()]; + for (original_idx, inode) in ordered_inodes { + guards[original_idx] = Some(Rc::new(inode.inner.write())); + } + guards + .into_iter() + .map(|guard| Rc::into_inner(guard.unwrap()).unwrap()) + .collect() +} + struct Inner { inode_impl: Arc, page_cache: PageCache, @@ -741,28 +860,28 @@ impl Inner { } fn init_dir(&mut self, self_ino: u32, parent_ino: u32) -> Result<()> { - self.append_entry(DirEntry::self_entry(self_ino), 0)?; - self.append_entry(DirEntry::parent_entry(parent_ino), 0)?; + self.append_entry(DirEntry::self_entry(self_ino))?; + self.append_entry(DirEntry::parent_entry(parent_ino))?; Ok(()) } - pub fn get_entry_ino(&self, name: &str, offset: usize) -> Option { - self.get_entry(name, offset).map(|(_, entry)| entry.ino()) + pub fn get_entry_ino(&self, name: &str) -> Option { + self.get_entry(name).map(|(_, entry)| entry.ino()) } - pub fn get_entry(&self, name: &str, offset: usize) -> Option<(usize, DirEntry)> { - DirEntryReader::new(&self.page_cache, offset).find(|(offset, entry)| entry.name() == name) + pub fn get_entry(&self, name: &str) -> Option<(usize, DirEntry)> { + DirEntryReader::new(&self.page_cache, 0).find(|(offset, entry)| entry.name() == name) } pub fn entry_count(&self) -> usize { DirEntryReader::new(&self.page_cache, 0).count() } - pub fn append_entry(&mut self, entry: DirEntry, offset: usize) -> Result<()> { + pub fn append_entry(&mut self, entry: DirEntry) -> Result<()> { let is_dir = entry.type_() == FileType::Dir; let is_parent = entry.name() == ".."; - DirEntryWriter::new(&self.page_cache, offset).append_entry(entry)?; + DirEntryWriter::new(&self.page_cache, 0).append_entry(entry)?; let file_size = self.inode_impl.file_size(); let page_cache_size = self.page_cache.pages().size(); if page_cache_size > file_size { @@ -774,7 +893,7 @@ impl Inner { Ok(()) } - pub fn remove_entry(&mut self, name: &str, offset: usize) -> Result<()> { + pub fn remove_entry_at(&mut self, name: &str, offset: usize) -> Result<()> { let entry = DirEntryWriter::new(&self.page_cache, offset).remove_entry(name)?; let is_dir = entry.type_() == FileType::Dir; let file_size = self.inode_impl.file_size(); @@ -788,7 +907,7 @@ impl Inner { Ok(()) } - pub fn rename_entry(&mut self, old_name: &str, new_name: &str, offset: usize) -> Result<()> { + pub fn rename_entry_at(&mut self, old_name: &str, new_name: &str, offset: usize) -> Result<()> { DirEntryWriter::new(&self.page_cache, offset).rename_entry(old_name, new_name)?; let file_size = self.inode_impl.file_size(); let page_cache_size = self.page_cache.pages().size(); @@ -799,7 +918,7 @@ impl Inner { } pub fn set_parent_ino(&mut self, parent_ino: u32) -> Result<()> { - let (offset, mut entry) = self.get_entry("..", 0).unwrap(); + let (offset, mut entry) = self.get_entry("..").unwrap(); entry.set_ino(parent_ino); DirEntryWriter::new(&self.page_cache, offset).write_entry(&entry)?; Ok(())