Change vfs::open syscall to an openat-like syscall + Fixed a bug in Dentry::create_and_fetch_file where the returned dentry was the parent and not the child

Author: haoud

kernel/src/fs/ramfs/interface.rs

@@ -451,9 +451,10 @@ fn readdir(
     offset: vfs::file::Offset,
 ) -> Result<vfs::dirent::DirectoryEntry, vfs::file::ReaddirError> {
     let file_data = file
-        .inode
+        .dentry
         .as_ref()
-        .expect("Open file without inode")
+        .expect("Open file without dentry")
+        .inode()
         .data
         .downcast_ref::<Spinlock<InodeDirectory>>()
         .expect("Inode is not a ramfs inode");
@@ -471,22 +472,26 @@ fn write(
     buf: &[u8],
     offset: vfs::file::Offset,
 ) -> Result<usize, vfs::file::WriteError> {
-    let file_data = file
-        .inode
+    let inode = file
+        .dentry
         .as_ref()
-        .expect("Open file without inode")
+        .expect("Open file without dentry")
+        .inode();
+    let mut metadata = inode.state.lock();
+
+    let file_data = inode
         .data
         .downcast_ref::<Spinlock<InodeFile>>()
         .expect("Inode is not a ramfs inode");
+    let mut locked_file = file_data.lock();
 
     // Write the buffer to the file, and extend the file if necessary.
-    let mut locked_file = file_data.lock();
     let content = locked_file.content_mut();
     let offset = offset.0;
 
     if offset + buf.len() > content.len() {
         content.resize(offset + buf.len(), 0);
-        file.inode.as_ref().unwrap().state.lock().size = content.len();
+        metadata.size = content.len();
     }
 
     // Write the buffer to the file and return the written size
@@ -500,15 +505,18 @@ fn read(
     buf: &mut [u8],
     offset: vfs::file::Offset,
 ) -> Result<usize, vfs::file::ReadError> {
-    let file_data = file
-        .inode
+    let inode = file
+        .dentry
         .as_ref()
-        .expect("Open file without inode")
+        .expect("Open file without dentry")
+        .inode();
+
+    let file_data = inode
         .data
         .downcast_ref::<Spinlock<InodeFile>>()
         .expect("Inode is not a ramfs inode");
-
     let locked_file = file_data.lock();
+
     let content = locked_file.content();
 
     // Read the buffer from the file. If the read goes beyond the end of the
@@ -549,9 +557,10 @@ fn seek(
         }
         vfs::file::Whence::End => {
             let file_data = file
-                .inode
+                .dentry
                 .as_ref()
                 .expect("Open file without inode")
+                .inode()
                 .data
                 .downcast_ref::<Spinlock<InodeFile>>()
                 .expect("Inode is not a ramfs inode");

kernel/src/syscall/mod.rs

@@ -97,7 +97,7 @@ fn syscall(id: usize, a: usize, b: usize, c: usize, d: usize, e: usize) -> isize
         Some(Syscall::MmuMap) => mmu::map(a, b, c, d).map_err(Into::into),
         Some(Syscall::MmuUnmap) => mmu::unmap(a, b).map_err(Into::into),
         Some(Syscall::VideoFramebufferInfo) => video::framebuffer_info(a).map_err(Into::into),
-        Some(Syscall::VfsOpen) => vfs::open(a, b).map_err(Into::into),
+        Some(Syscall::VfsOpen) => vfs::open(a, b, c).map_err(Into::into),
         Some(Syscall::VfsClose) => vfs::close(a).map_err(Into::into),
         Some(Syscall::VfsRead) => vfs::read(a, b, c).map_err(Into::into),
         Some(Syscall::VfsWrite) => vfs::write(a, b, c).map_err(Into::into),

kernel/src/syscall/vfs.rs

@@ -19,10 +19,23 @@ use super::clock::Timespec;
 /// # Panics
 /// This function panics an inode does not have a corresponding superblock. This
 /// should never happen, and is a serious bug in the kernel if it does.
-pub fn open(path: usize, flags: usize) -> Result<usize, OpenError> {
+pub fn open(dir: usize, path: usize, flags: usize) -> Result<usize, OpenError> {
     let current_task = SCHEDULER.current_task();
     let root = current_task.root();
-    let cwd = current_task.cwd();
+
+    // This is the dentry pointed by the file descriptor `dir`. If `dir` is
+    // `AT_FDCWD`, the current working directory is used.
+    let cwd = match dir {
+        vfs::fd::Descriptor::AT_FDCWD => current_task.cwd(),
+        _ => current_task
+            .files()
+            .lock()
+            .get(vfs::fd::Descriptor(dir))
+            .ok_or(OpenError::BadFileDescriptor)?
+            .dentry
+            .clone()
+            .ok_or(OpenError::NotADirectory)?,
+    };
 
     let flags = vfs::file::OpenFlags::from_bits(flags).ok_or(OpenError::InvalidFlag)?;
     let ptr = user::Pointer::<SyscallString>::from_usize(path).ok_or(OpenError::BadAddress)?;
@@ -59,13 +72,16 @@ pub fn open(path: usize, flags: usize) -> Result<usize, OpenError> {
                 }
 
                 let name = path.as_name().ok_or(OpenError::NoSuchFile)?.clone();
-                Dentry::create_and_fetch_file(parent, name)?
+                Dentry::create_and_fetch_file(&parent, name)?
             } else {
                 return Err(OpenError::from(e));
             }
         }
     };
 
+    log::debug!("Opening file inode id {}", dentry.inode().id.0);
+    log::debug!("name: {:?}", dentry.name());
+
     let file = dentry.open(flags)?;
     let id = current_task
         .files()
@@ -85,6 +101,9 @@ pub enum OpenError {
     /// An invalid address was passed as an argument
     BadAddress,
 
+    /// An invalid file descriptor was passed as an argument
+    BadFileDescriptor,
+
     /// The path is invalid
     InvalidPath,
 
@@ -338,8 +357,8 @@ pub fn write(fd: usize, buf: usize, len: usize) -> Result<usize, WriteError> {
 
     // If the file is associated with an inode, mark it as dirty since the inode
     // may has been modified
-    if let Some(inode) = &file.inode {
-        inode.mark_dirty();
+    if let Some(dentry) = &file.dentry {
+        dentry.dirtying_inode();
     }
 
     state.offset = offset;

kernel/src/vfs/dentry.rs

@@ -57,10 +57,10 @@ impl Dentry {
     /// # Errors
     /// Currently, this function does not return any error. However, this may change
     /// in the future.
-    pub fn open(&self, flags: OpenFlags) -> Result<File, OpenError> {
+    pub fn open(self: &Arc<Self>, flags: OpenFlags) -> Result<File, OpenError> {
         Ok(file::File::new(FileCreateInfo {
             operation: self.inode.file_ops.clone(),
-            inode: Some(self.inode.clone()),
+            dentry: Some(self.clone()),
             open_flags: flags,
             data: Box::new(()),
         }))
@@ -190,12 +190,12 @@ impl Dentry {
     /// if this function fails to connect the created dentry to this dentry. This should
     /// never happen and is a serious kernel bug.
     pub fn create_and_fetch_file(
-        dentry: Arc<Self>,
+        dentry: &Arc<Self>,
         name: Name,
     ) -> Result<Arc<Self>, CreateFetchError> {
         // Search for the file in the dentry cache and in the underlying filesystem.
         // If a file with the same name already exists, return an error.
-        match Self::fetch(&dentry, &name) {
+        match Self::fetch(dentry, &name) {
             Err(FetchError::NotFound) => {}
             Err(FetchError::NotADirectory) => return Err(CreateFetchError::NotADirectory),
             Err(FetchError::IoError) => return Err(CreateFetchError::IoError),
@@ -219,10 +219,10 @@ impl Dentry {
 
         // If an entry with the same name was created in the meantime, we
         // simply return an error.
-        match Self::connect_child(&dentry, child) {
+        match Self::connect_child(dentry, Arc::clone(&child)) {
             Err(ConnectError::AlreadyConnected | ConnectError::NotADirectory) => unreachable!(),
             Err(ConnectError::AlreadyExists) => Err(CreateFetchError::AlreadyExists),
-            Ok(_) => Ok(dentry),
+            Ok(_) => Ok(child),
         }
     }
 
@@ -349,12 +349,6 @@ impl Dentry {
     }
 }
 
-impl Drop for Dentry {
-    fn drop(&mut self) {
-        log::debug!("Dentry dropped: {:?}", self.name());
-    }
-}
-
 /// Represents a dentry in the filesystem tree. This structure is used to
 /// contains fields that can be modified by the filesystem driver, like the
 /// children list or the dentr name. The [`Dentry`] structure contains fields

kernel/src/vfs/fd.rs

@@ -5,6 +5,21 @@ use super::file::File;
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Descriptor(pub usize);
 
+impl Descriptor {
+    pub const AT_FDCWD_DESCRIPTOR: Self = Self(Self::AT_FDCWD);
+    pub const AT_FDCWD: usize = usize::MAX;
+
+    /// Create a new file descriptor from the given file descriptor number. If
+    /// the number is `AT_FDCWD`, returns `None`, otherwise returns `Some`.
+    #[must_use]
+    pub const fn new(fd: usize) -> Option<Self> {
+        match fd {
+            Self::AT_FDCWD => None,
+            _ => Some(Self(fd)),
+        }
+    }
+}
+
 /// A table of opened files. This is a simple array of 32 elements, where each
 /// element is an optional reference to an opened file.
 ///

kernel/src/vfs/file.rs

@@ -1,10 +1,10 @@
-use super::{dirent::DirectoryEntry, inode::Inode};
+use super::{dentry::Dentry, dirent::DirectoryEntry};
 use core::any::Any;
 
 #[derive(Debug)]
 pub struct File {
     /// The inode opened by this file.
-    pub inode: Option<Arc<Inode>>,
+    pub dentry: Option<Arc<Dentry>>,
 
     /// The operation table for this file.
     pub operation: Operation,
@@ -26,7 +26,7 @@ impl File {
     pub fn new(info: FileCreateInfo) -> Self {
         let state = OpenFileState { offset: Offset(0) };
         Self {
-            inode: info.inode,
+            dentry: info.dentry,
             operation: info.operation,
             open_flags: info.open_flags,
             state: Spinlock::new(state),
@@ -53,7 +53,7 @@ impl File {
 
 #[derive(Debug)]
 pub struct FileCreateInfo {
-    pub inode: Option<Arc<Inode>>,
+    pub dentry: Option<Arc<Dentry>>,
     pub operation: Operation,
     pub open_flags: OpenFlags,
     pub data: Box<dyn Any + Send + Sync>,

kernel/src/vfs/mod.rs

@@ -78,6 +78,7 @@ fn fill_ramdisk() {
 
 bitflags::bitflags! {
     /// Flags to control the behavior of the lookup operation.
+    #[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
     pub struct LookupFlags: u32 {
         /// Return the parent of the last component of the path. If the path has
         /// only one component, return the parent of the current directory. If the
@@ -169,9 +170,10 @@ pub fn read_all(
         .map_err(|_| ReadAllError::OpenError)?;
 
     let len = file
-        .inode
+        .dentry
         .as_ref()
-        .expect("Regular open file without inode")
+        .expect("Regular open file without dentry")
+        .inode()
         .state
         .lock()
         .size;

kernel/src/vfs/pipe/mod.rs

@@ -124,14 +124,14 @@ fn create_pair() -> (Arc<file::File>, Arc<file::File>) {
         operation: file::Operation::File(&PIPE_FILE_OPS),
         open_flags: file::OpenFlags::READ,
         data: Box::new(reader),
-        inode: None,
+        dentry: None,
     }));
 
     let writer_file = Arc::new(file::File::new(file::FileCreateInfo {
         operation: file::Operation::File(&PIPE_FILE_OPS),
         open_flags: file::OpenFlags::WRITE,
         data: Box::new(writer),
-        inode: None,
+        dentry: None,
     }));
 
     (reader_file, writer_file)

user/shell/src/main.rs

@@ -1,3 +1,5 @@
+use syscall::vfs::FileDescriptor;
+
 fn main() {
     println!("Creating /test");
     syscall::vfs::mkdir("/test").expect("mkdir failed");
@@ -42,7 +44,13 @@ fn main() {
     syscall::vfs::mkdir("/test").expect("mkdir failed");
 
     println!("Adding /test/test.txt");
-    syscall::vfs::open("/test/test.txt", syscall::vfs::O_CREATE, 0).expect("open failed");
+    syscall::vfs::open(
+        FileDescriptor::AT_FDCWD,
+        "/test/test.txt",
+        syscall::vfs::O_CREATE,
+        0,
+    )
+    .expect("open failed");
 
     println!("Trying to removing /test");
     match syscall::vfs::rmdir("/test") {

user/syscall/src/vfs.rs

@@ -11,6 +11,13 @@ pub const O_EXCL: usize = 1 << 4;
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct FileDescriptor(usize);
 
+impl FileDescriptor {
+    pub const STDIN: Self = Self(0);
+    pub const STDOUT: Self = Self(1);
+    pub const STDERR: Self = Self(2);
+    pub const AT_FDCWD: Self = Self(usize::MAX);
+}
+
 pub enum Whence {
     Current(isize),
     Start(isize),
@@ -70,6 +77,9 @@ pub enum OpenError {
     /// An invalid address was passed as an argument
     BadAddress,
 
+    /// An invalid file descriptor was passed as an argument
+    BadFileDescriptor,
+
     /// The path is invalid
     InvalidPath,
 
@@ -562,16 +572,22 @@ impl From<Errno> for UnlinkError {
 ///
 /// # Errors
 /// See `OpenError` for a list of possible errors.
-pub fn open(path: &str, flags: usize, _mode: usize) -> Result<FileDescriptor, OpenError> {
+pub fn open(
+    dir: FileDescriptor,
+    path: &str,
+    flags: usize,
+    _mode: usize,
+) -> Result<FileDescriptor, OpenError> {
     let str = SyscallString::from(path);
     let ret;
 
     unsafe {
         core::arch::asm!(
             "syscall",
             in("rax") Syscall::VfsOpen as u64,
-            in("rsi") &str as *const _ as u64,
-            in("rdx") flags as u64,
+            in("rsi") dir.0,
+            in("rdx") &str as *const _ as u64,
+            in("r10") flags,
             lateout("rax") ret,
         );
     }