introduce VMError to simplify EFAULT error handling

crate/memory/src/lib.rs

@@ -14,3 +14,9 @@ mod addr;
 pub mod no_mmu;
 
 pub use crate::addr::*;
+
+pub enum VMError {
+    InvalidPtr
+}
+
+pub type VMResult<T> = Result<T, VMError>;

crate/memory/src/memory_set/mod.rs

@@ -192,29 +192,37 @@ impl<T: InactivePageTable> MemorySet<T> {
         }
     }
     /// Check the pointer is within the readable memory
-    pub fn check_ptr<S>(&self, ptr: *const S) -> bool {
-        self.areas.iter().find(|area| area.check_ptr(ptr)).is_some()
+    pub fn check_ptr<S>(&self, ptr: *const S) -> VMResult<()> {
+        self.areas.iter()
+            .find(|area| area.check_ptr(ptr))
+            .map(|_|()).ok_or(VMError::InvalidPtr)
     }
     /// Check the pointer is within the writable memory
-    pub fn check_mut_ptr<S>(&self, ptr: *mut S) -> bool {
-        self.areas.iter().find(|area| area.check_mut_ptr(ptr)).is_some()
+    pub fn check_mut_ptr<S>(&self, ptr: *mut S) -> VMResult<()> {
+        self.areas.iter()
+            .find(|area| area.check_mut_ptr(ptr))
+            .map(|_|()).ok_or(VMError::InvalidPtr)
     }
     /// Check the array is within the readable memory
-    pub fn check_array<S>(&self, ptr: *const S, count: usize) -> bool {
-        self.areas.iter().find(|area| area.check_array(ptr, count)).is_some()
+    pub fn check_array<S>(&self, ptr: *const S, count: usize) -> VMResult<()> {
+        self.areas.iter()
+            .find(|area| area.check_array(ptr, count))
+            .map(|_|()).ok_or(VMError::InvalidPtr)
     }
     /// Check the array is within the writable memory
-    pub fn check_mut_array<S>(&self, ptr: *mut S, count: usize) -> bool {
-        self.areas.iter().find(|area| area.check_mut_array(ptr, count)).is_some()
+    pub fn check_mut_array<S>(&self, ptr: *mut S, count: usize) -> VMResult<()> {
+        self.areas.iter()
+            .find(|area| area.check_mut_array(ptr, count))
+            .map(|_|()).ok_or(VMError::InvalidPtr)
     }
     /// Check the null-end C string is within the readable memory, and is valid.
     /// If so, clone it to a String.
     ///
     /// Unsafe: the page table must be active.
-    pub unsafe fn check_and_clone_cstr(&self, ptr: *const u8) -> Option<String> {
+    pub unsafe fn check_and_clone_cstr(&self, ptr: *const u8) -> VMResult<String> {
         self.areas.iter()
             .filter_map(|area| area.check_and_clone_cstr(ptr))
-            .next()
+            .next().ok_or(VMError::InvalidPtr)
     }
     /// Find a free area with hint address `addr_hint` and length `len`.
     /// Return the start address of found free area.

kernel/src/syscall/fs.rs

@@ -11,9 +11,7 @@ use super::net::*;
 pub fn sys_read(fd: usize, base: *mut u8, len: usize) -> SysResult {
     info!("read: fd: {}, base: {:?}, len: {:#x}", fd, base, len);
     let mut proc = process();
-    if !proc.memory_set.check_mut_array(base, len) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_mut_array(base, len)?;
     let slice = unsafe { slice::from_raw_parts_mut(base, len) };
     let len = proc.get_file(fd)?.read(slice)?;
     Ok(len as isize)
@@ -22,9 +20,7 @@ pub fn sys_read(fd: usize, base: *mut u8, len: usize) -> SysResult {
 pub fn sys_write(fd: usize, base: *const u8, len: usize) -> SysResult {
     info!("write: fd: {}, base: {:?}, len: {:#x}", fd, base, len);
     let mut proc = process();
-    if !proc.memory_set.check_array(base, len) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_array(base, len)?;
     match proc.files.get(&fd) {
         Some(FileLike::File(_)) => sys_write_file(&mut proc, fd, base, len),
         Some(FileLike::Socket(_)) => sys_write_socket(&mut proc, fd, base, len),
@@ -65,8 +61,7 @@ pub fn sys_writev(fd: usize, iov_ptr: *const IoVec, iov_count: usize) -> SysResu
 
 pub fn sys_open(path: *const u8, flags: usize, mode: usize) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
     let flags = OpenFlags::from_bits_truncate(flags);
     info!("open: path: {:?}, flags: {:?}, mode: {:#o}", path, flags, mode);
 
@@ -115,9 +110,7 @@ pub fn sys_close(fd: usize) -> SysResult {
 pub fn sys_getcwd(buf: *mut u8, len: usize) -> SysResult {
     info!("getcwd: buf: {:?}, len: {:#x}", buf, len);
     let mut proc = process();
-    if !proc.memory_set.check_mut_array(buf, len) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_mut_array(buf, len)?;
     if proc.cwd.len() + 1 > len {
         return Err(SysError::ERANGE);
     }
@@ -135,9 +128,7 @@ pub fn sys_stat(path: *const u8, stat_ptr: *mut Stat) -> SysResult {
 pub fn sys_fstat(fd: usize, stat_ptr: *mut Stat) -> SysResult {
     info!("fstat: fd: {}", fd);
     let mut proc = process();
-    if !proc.memory_set.check_mut_ptr(stat_ptr) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_mut_ptr(stat_ptr)?;
     let file = proc.get_file(fd)?;
     let stat = Stat::from(file.metadata()?);
     // TODO: handle symlink
@@ -147,11 +138,8 @@ pub fn sys_fstat(fd: usize, stat_ptr: *mut Stat) -> SysResult {
 
 pub fn sys_lstat(path: *const u8, stat_ptr: *mut Stat) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
-    if !proc.memory_set.check_mut_ptr(stat_ptr) {
-        return Err(SysError::EFAULT);
-    }
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
+    proc.memory_set.check_mut_ptr(stat_ptr)?;
     info!("lstat: path: {}", path);
 
     let inode = proc.lookup_inode(&path)?;
@@ -189,8 +177,7 @@ pub fn sys_fdatasync(fd: usize) -> SysResult {
 
 pub fn sys_truncate(path: *const u8, len: usize) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
     info!("truncate: path: {:?}, len: {}", path, len);
     proc.lookup_inode(&path)?.resize(len)?;
     Ok(0)
@@ -205,9 +192,7 @@ pub fn sys_ftruncate(fd: usize, len: usize) -> SysResult {
 pub fn sys_getdents64(fd: usize, buf: *mut LinuxDirent64, buf_size: usize) -> SysResult {
     info!("getdents64: fd: {}, ptr: {:?}, buf_size: {}", fd, buf, buf_size);
     let mut proc = process();
-    if !proc.memory_set.check_mut_array(buf as *mut u8, buf_size) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_mut_array(buf as *mut u8, buf_size)?;
     let file = proc.get_file(fd)?;
     let info = file.metadata()?;
     if info.type_ != FileType::Dir {
@@ -239,8 +224,7 @@ pub fn sys_dup2(fd1: usize, fd2: usize) -> SysResult {
 
 pub fn sys_chdir(path: *const u8) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
     info!("chdir: path: {:?}", path);
 
     let inode = proc.lookup_inode(&path)?;
@@ -255,10 +239,8 @@ pub fn sys_chdir(path: *const u8) -> SysResult {
 
 pub fn sys_rename(oldpath: *const u8, newpath: *const u8) -> SysResult {
     let mut proc = process();
-    let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath) }
-        .ok_or(SysError::EFAULT)?;
-    let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath) }
-        .ok_or(SysError::EFAULT)?;
+    let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath)? };
+    let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath)? };
     info!("rename: oldpath: {:?}, newpath: {:?}", oldpath, newpath);
 
     let (old_dir_path, old_file_name) = split_path(&oldpath);
@@ -276,8 +258,7 @@ pub fn sys_rename(oldpath: *const u8, newpath: *const u8) -> SysResult {
 
 pub fn sys_mkdir(path: *const u8, mode: usize) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
     // TODO: check pathname
     info!("mkdir: path: {:?}, mode: {:#o}", path, mode);
 
@@ -292,10 +273,8 @@ pub fn sys_mkdir(path: *const u8, mode: usize) -> SysResult {
 
 pub fn sys_link(oldpath: *const u8, newpath: *const u8) -> SysResult {
     let mut proc = process();
-    let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath) }
-        .ok_or(SysError::EFAULT)?;
-    let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath) }
-        .ok_or(SysError::EFAULT)?;
+    let oldpath = unsafe { proc.memory_set.check_and_clone_cstr(oldpath)? };
+    let newpath = unsafe { proc.memory_set.check_and_clone_cstr(newpath)? };
     info!("link: oldpath: {:?}, newpath: {:?}", oldpath, newpath);
 
     let (new_dir_path, new_file_name) = split_path(&newpath);
@@ -307,8 +286,7 @@ pub fn sys_link(oldpath: *const u8, newpath: *const u8) -> SysResult {
 
 pub fn sys_unlink(path: *const u8) -> SysResult {
     let mut proc = process();
-    let path = unsafe { proc.memory_set.check_and_clone_cstr(path) }
-        .ok_or(SysError::EFAULT)?;
+    let path = unsafe { proc.memory_set.check_and_clone_cstr(path)? };
     info!("unlink: path: {:?}", path);
 
     let (dir_path, file_name) = split_path(&path);
@@ -597,15 +575,14 @@ struct IoVecs(Vec<&'static mut [u8]>);
 
 impl IoVecs {
     fn check_and_new(iov_ptr: *const IoVec, iov_count: usize, vm: &MemorySet, readv: bool) -> Result<Self, SysError> {
-        if !vm.check_array(iov_ptr, iov_count) {
-            return Err(SysError::EFAULT);
-        }
+        vm.check_array(iov_ptr, iov_count)?;
         let iovs = unsafe { slice::from_raw_parts(iov_ptr, iov_count) }.to_vec();
         // check all bufs in iov
         for iov in iovs.iter() {
-            if readv && !vm.check_mut_array(iov.base, iov.len as usize)
-                || !readv && !vm.check_array(iov.base, iov.len as usize) {
-                return Err(SysError::EFAULT);
+            if readv {
+                vm.check_mut_array(iov.base, iov.len as usize)?;
+            } else {
+                vm.check_array(iov.base, iov.len as usize)?;
             }
         }
         let slices = iovs.iter().map(|iov| unsafe { slice::from_raw_parts_mut(iov.base, iov.len as usize) }).collect();

kernel/src/syscall/mod.rs

@@ -4,6 +4,7 @@ use alloc::{string::String, sync::Arc, vec::Vec};
 use core::{slice, str, fmt};
 
 use bitflags::bitflags;
+use rcore_memory::VMError;
 use rcore_fs::vfs::{FileType, FsError, INode, Metadata};
 use spin::{Mutex, MutexGuard};
 
@@ -270,3 +271,9 @@ impl fmt::Display for SysError {
         )
     }
 }
+
+impl From<VMError> for SysError {
+    fn from(_: VMError) -> Self {
+        SysError::EFAULT
+    }
+}

kernel/src/syscall/net.rs

@@ -139,9 +139,7 @@ pub fn sys_connect(fd: usize, addr: *const u8, addrlen: usize) -> SysResult {
     );
 
     let mut proc = process();
-    if !proc.memory_set.check_ptr(addr) {
-        return Err(SysError::EFAULT);
-    }
+    proc.memory_set.check_ptr(addr)?;
 
     let mut dest = None;
     let mut port = 0;
@@ -234,13 +232,8 @@ pub fn sys_sendto(
         fd, buffer, len, addr, addr_len
     );
     let mut proc = process();
-    if !proc.memory_set.check_ptr(addr) {
-        return Err(SysError::EFAULT);
-    }
-
-    if !proc.memory_set.check_array(buffer, len) {
-        return Err(SysError::EINVAL);
-    }
+    proc.memory_set.check_ptr(addr)?;
+    proc.memory_set.check_array(buffer, len)?;
 
     // little hack: kick it forward
     let iface = &mut *NET_DRIVERS.lock()[0];

kernel/src/syscall/proc.rs

@@ -13,9 +13,7 @@ pub fn sys_fork(tf: &TrapFrame) -> SysResult {
 /// Wait the process exit.
 /// Return the PID. Store exit code to `code` if it's not null.
 pub fn sys_wait(pid: usize, code: *mut i32) -> SysResult {
-    if !process().memory_set.check_mut_ptr(code) {
-        return Err(SysError::EFAULT);
-    }
+    process().memory_set.check_mut_ptr(code)?;
     loop {
         use alloc::vec;
         let wait_procs = match pid {
@@ -53,8 +51,7 @@ pub fn sys_wait(pid: usize, code: *mut i32) -> SysResult {
 pub fn sys_exec(name: *const u8, argc: usize, argv: *const *const u8, tf: &mut TrapFrame) -> SysResult {
     let proc = process();
     let name = if name.is_null() { String::from("") } else {
-        unsafe { proc.memory_set.check_and_clone_cstr(name) }
-            .ok_or(SysError::EFAULT)?
+        unsafe { proc.memory_set.check_and_clone_cstr(name)? }
     };
     if argc <= 0 {
         return Err(SysError::EINVAL);
@@ -63,8 +60,7 @@ pub fn sys_exec(name: *const u8, argc: usize, argv: *const *const u8, tf: &mut T
     let mut args = Vec::new();
     unsafe {
         for &ptr in slice::from_raw_parts(argv, argc) {
-            let arg = proc.memory_set.check_and_clone_cstr(ptr)
-                .ok_or(SysError::EFAULT)?;
+            let arg = proc.memory_set.check_and_clone_cstr(ptr)?;
             args.push(arg);
         }
     }

kernel/src/syscall/time.rs

@@ -10,9 +10,7 @@ pub fn sys_time(time: *mut u64) -> SysResult {
     let t = unsafe { crate::trap::TICK };
     if time as usize != 0 {
         let mut proc = process();
-        if !proc.memory_set.check_mut_ptr(time) {
-            return Err(SysError::EFAULT);
-        }
+        proc.memory_set.check_mut_ptr(time)?;
         unsafe {
             time.write(t as u64);
         }