ext2: non-empty directory check when unlinking

kernel/driver/fs/ext2/src/dir.rs

@@ -3,6 +3,7 @@ use core::{
     any::Any,
     mem::{self, MaybeUninit},
     str::FromStr,
+    sync::atomic::{AtomicBool, Ordering},
 };
 
 use alloc::sync::Arc;
@@ -21,6 +22,7 @@ use crate::{data::FsRequiredFeatures, file::RegularNode, inode::InodeAccess, Dir
 
 pub struct DirectoryNode {
     fs: Arc<Ext2Fs>,
+    locked: AtomicBool,
     pub(crate) inode: InodeAccess,
 }
 
@@ -260,6 +262,7 @@ impl DirectoryNode {
         Node::directory(
             Self {
                 fs: fs.clone(),
+                locked: AtomicBool::new(false),
                 inode,
             },
             NodeFlags::empty(),
@@ -276,6 +279,7 @@ impl DirectoryNode {
         let ino = inode.ino();
         let this = Self {
             fs: fs.clone(),
+            locked: AtomicBool::new(false),
             inode,
         };
 
@@ -291,6 +295,11 @@ impl DirectoryNode {
     async fn create_entry(&self, name: &str, ino: u32) -> Result<(), Error> {
         assert!(name.len() < 255);
 
+        // This directory is in the process of being unlinked and cannot be written to
+        if self.locked.load(Ordering::Acquire) {
+            return Err(Error::DoesNotExist);
+        }
+
         {
             let mut holder = self.inode.cache().get_mut(ino).await?;
             {
@@ -376,9 +385,55 @@ impl DirectoryNode {
         Ok(())
     }
 
-    async fn remove_entry(&self, name: &str) -> Result<(), Error> {
-        // TODO if unlinking a directory, check that it only contains . and ..
-        assert!(name.len() < 255);
+    async fn prepare_for_removal(&self) -> Result<(), Error> {
+        // Check that the directory doesn't have any children besides . and .. and set the locked
+        // flag to prevent anyone from adding entries while the directory is being unlinked
+        // TODO find a better way to do this
+        let mut holder = self.inode.get_mut().await?;
+        {
+            let inode = holder.write();
+            let n = inode.blocks(&self.fs);
+
+            for i in 0..n {
+                self.fs
+                    .with_inode_block(&inode, i as _, |block| {
+                        let iter = DirentIter::new(&self.fs, block, 0);
+
+                        for (_, name, _) in iter {
+                            if name != b"." && name != b".." {
+                                return Err(Error::IsADirectory);
+                            }
+                        }
+
+                        Ok(())
+                    })
+                    .await?;
+            }
+
+            self.locked.store(true, Ordering::Release);
+        }
+        holder.put().await?;
+        Ok(())
+    }
+
+    async fn remove_entry(&self, child: &Node, name: &str) -> Result<(), Error> {
+        if name.len() >= 255 || name.contains('/') || name.contains('\0') {
+            return Err(Error::InvalidArgument);
+        }
+        assert_eq!(
+            Arc::as_ptr(&child.filesystem().unwrap()).addr(),
+            Arc::as_ptr(&self.fs).addr()
+        );
+
+        let child_ino = match child.data_as_any() {
+            data if let Some(dir) = data.downcast_ref::<DirectoryNode>() => {
+                // Check that the directory is not empty
+                dir.prepare_for_removal().await?;
+                dir.inode.ino()
+            }
+            data if let Some(file) = data.downcast_ref::<RegularNode>() => file.inode.ino(),
+            _ => return Err(Error::InvalidOperation),
+        };
 
         let ino = {
             let mut holder = self.inode.get_mut().await?;
@@ -409,6 +464,7 @@ impl DirectoryNode {
         };
 
         let ino = ino.ok_or(Error::DoesNotExist)?;
+        assert_eq!(ino, child_ino);
         log::info!("ext2: unlinked ino #{ino}");
 
         // Decrement the refcount on the inode
@@ -582,11 +638,11 @@ impl DirectoryImpl for DirectoryNode {
         Ok(())
     }
 
-    fn unlink_node(&self, _parent: &NodeRef, name: &str) -> Result<(), Error> {
+    fn unlink_node(&self, _parent: &NodeRef, child: &NodeRef, name: &str) -> Result<(), Error> {
         if self.fs.force_readonly {
             return Err(Error::ReadOnly);
         }
-        block!(self.remove_entry(name).await)?
+        block!(self.remove_entry(child, name).await)?
     }
 
     fn read_entries(

kernel/libk/src/vfs/node/ops.rs

@@ -139,23 +139,27 @@ impl Node {
         let directory = self.as_directory()?;
         let child = self.lookup_or_load(name, check)?;
 
-        if child.is_directory() {
-            return Err(Error::IsADirectory);
-        }
-
         // Detach the node in the real filesystem
-        match directory.imp.unlink_node(self, name) {
-            Ok(_) | Err(Error::NotImplemented) => (),
+        // If the filesystem implements unlink_node(), then checking if it's okay to
+        // remove a directory node is done by the filesystem itself, if not, check the
+        // cache.
+        let need_nonempty_check = match directory.imp.unlink_node(self, &child, name) {
+            Ok(_) => false,
+            Err(Error::NotImplemented) => true,
             Err(err) => return Err(err),
+        };
+
+        if need_nonempty_check && let Ok(child_dir) = child.as_directory() {
+            let child_children = child_dir.children.lock();
+            if !child_children.is_empty() {
+                return Err(Error::IsADirectory);
+            }
+            directory.remove_child(name);
+        } else {
+            // Just unlink the node from the cache tree
+            directory.remove_child(name);
         }
 
-        // Detach the node in the tree cache
-        {
-            let mut children = directory.children.lock();
-            children.retain(|(name_, _)| name != name_);
-        }
-
-        // TODO child.destroy() or something?
         Ok(())
     }
 
@@ -177,7 +181,7 @@ impl Node {
 
         let node = source.lookup_or_load(source_name, access)?;
 
-        match source_dir.imp.unlink_node(source, source_name) {
+        match source_dir.imp.unlink_node(source, &node, source_name) {
             Ok(()) | Err(Error::NotImplemented) => (),
             Err(error) => {
                 log::warn!("Rename: unlink {source_name}: {error:?}");

kernel/libk/src/vfs/node/traits.rs

@@ -37,7 +37,6 @@ pub trait CommonImpl: Send + Sync {
 }
 
 /// Regular file interface
-#[allow(unused)]
 pub trait RegularImpl: CommonImpl {
     /// Opens the file for reading/writing and returns a `(start position, instance data)` tuple
     fn open(
@@ -82,7 +81,6 @@ pub trait RegularImpl: CommonImpl {
 }
 
 /// Directory implementation
-#[allow(unused)]
 pub trait DirectoryImpl: CommonImpl {
     /// Opens a directory for reading its entries. Returns [DirectoryOpenPosition] to specify the
     /// starting position.
@@ -111,7 +109,7 @@ pub trait DirectoryImpl: CommonImpl {
     }
 
     /// Removes an entry of the directory with given name
-    fn unlink_node(&self, parent: &NodeRef, name: &str) -> Result<(), Error> {
+    fn unlink_node(&self, parent: &NodeRef, child: &NodeRef, name: &str) -> Result<(), Error> {
         Err(Error::NotImplemented)
     }
 
@@ -127,7 +125,6 @@ pub trait DirectoryImpl: CommonImpl {
 }
 
 /// Symbolic link interface
-#[allow(unused)]
 pub trait SymlinkImpl: CommonImpl {
     // /// Returns the target node (if such is available directly) of the link
     // fn target(&self, node: &NodeRef) -> Result<NodeRef, Error> {