Merge #352

352: Add `UserMemory` type to exploit invariant of user process memory r=efenniht a=Medowhill

유저 프로세스 메모리를 관리하는 `UserMemory` 타입을 추가했습니다. `UserMemory`는 다음과 같은 invariant를 가집니다.

(For brevity, `pt` := `page_table`, and we treat `pt` as a function from `va` to `pa`.)

* `TRAMPOLINE` ∈ dom(`pt`) ∧ `pt(TRAMPOLINE)` = `trampoline`.
* `TRAPFRAME` ∈ dom(`pt`) ∧ `pt(TRAPFRAM)E` = `trap_frame`.
* If `va` ∈ dom(`pt`), `va` mod `PGSIZE` = 0 ∧ `pt(va)` mod `PGSIZE` = 0.
* If `va` ∈ dom(`pt`) where `va` ∉ { `TRAMPOLINE`, `TRAPFRAME` }, then `Page::from_usize(pt(va))` succeeds without breaking the invariant of `Page`.
* If `va` ∈ dom(`pt`) where `va` ∉ { 0, `TRAMPOLINE`, `TRAPFRAME` }, then `va - PGSIZE` ∈ dom(`pt`).
* `pgroundup(size)` ∉ dom(`pt`).
* If `size` > 0, then `pgroundup(size) - PGSIZE` ∈ dom(`pt`).
* `Page::from_usize(trap_frame)` succeeds without breaking the invariant of `Page`.

이제 `PageTable`은 단순히 `VAddr`에서 `PAddr`로 가는 맵 자료 구조로 취급됩니다.

Closes #338

Co-authored-by: Jaemin Hong <[email protected]>

kernel-rs/src/exec.rs

@@ -1,16 +1,15 @@
 #![allow(clippy::unit_arg)]
 
 use crate::{
-    fs::{InodeGuard, Path},
+    fs::Path,
     kernel::Kernel,
     page::Page,
     param::MAXARG,
-    proc::{myproc, Proc},
+    proc::myproc,
     riscv::{pgroundup, PGSIZE},
-    vm::{KVAddr, PageTable, UVAddr, VAddr},
+    vm::{KVAddr, PAddr, UVAddr, UserMemory, VAddr},
 };
 use core::{cmp, mem};
-use cstr_core::CStr;
 
 /// "\x7FELF" in little endian
 const ELF_MAGIC: u32 = 0x464c457f;
@@ -89,8 +88,8 @@ impl ProgHdr {
 }
 
 impl Kernel {
-    pub unsafe fn exec(&self, path: &Path, argv: &[Page]) -> Result<usize, ()> {
-        if argv.len() > MAXARG {
+    pub fn exec(&self, path: &Path, args: &[Page]) -> Result<usize, ()> {
+        if args.len() > MAXARG {
             return Err(());
         }
 
@@ -115,12 +114,12 @@ impl Kernel {
             return Err(());
         }
 
-        let p: *mut Proc = myproc();
-        let mut data = &mut *(*p).data.get();
-        let mut pt = PageTable::<UVAddr>::new(data.trapframe).ok_or(())?;
+        let p = unsafe { &mut *myproc() };
+        let mut data = unsafe { &mut *p.data.get() };
+        let trap_frame = PAddr::new(data.trap_frame() as *const _ as _);
+        let mut mem = UserMemory::new(trap_frame, None).ok_or(())?;
 
         // Load program into memory.
-        let mut sz = 0;
         for i in 0..elf.phnum as usize {
             let off = elf.phoff + i * mem::size_of::<ProgHdr>();
 
@@ -137,31 +136,29 @@ impl Kernel {
                 if ph.memsz < ph.filesz || ph.vaddr % PGSIZE != 0 {
                     return Err(());
                 }
-                sz = pt.alloc(sz, ph.vaddr.checked_add(ph.memsz).ok_or(())?)?;
-                loadseg(
-                    &mut pt,
-                    UVAddr::new(ph.vaddr),
-                    &mut ip,
-                    ph.off as _,
-                    ph.filesz as _,
-                )?;
+                mem.alloc(ph.vaddr.checked_add(ph.memsz).ok_or(())?)?;
+                mem.read_file(UVAddr::new(ph.vaddr), &mut ip, ph.off as _, ph.filesz as _)?;
             }
         }
         drop(ip);
         drop(tx);
 
         // Allocate two pages at the next page boundary.
         // Use the second as the user stack.
-        sz = pgroundup(sz);
-        sz = pt.alloc(sz, sz + 2 * PGSIZE)?;
-        pt.clear(UVAddr::new(sz - 2 * PGSIZE));
+        let mut sz = pgroundup(mem.size());
+        sz = mem.alloc(sz + 2 * PGSIZE)?;
+        mem.clear(UVAddr::new(sz - 2 * PGSIZE));
         let mut sp: usize = sz;
         let stackbase: usize = sp - PGSIZE;
 
         // Push argument strings, prepare rest of stack in ustack.
         let mut ustack = [0usize; MAXARG + 1];
-        for (arg, stack) in izip!(argv, &mut ustack) {
-            let bytes = CStr::from_ptr(arg.as_ptr()).to_bytes_with_nul();
+        for (arg, stack) in izip!(args, &mut ustack) {
+            let null_idx = arg
+                .iter()
+                .position(|c| *c == 0)
+                .expect("exec: no null char found");
+            let bytes = &arg[..null_idx + 1];
             sp -= bytes.len();
 
             // riscv sp must be 16-byte aligned
@@ -170,10 +167,10 @@ impl Kernel {
                 return Err(());
             }
 
-            pt.copy_out(UVAddr::new(sp), bytes)?;
+            mem.copy_out(UVAddr::new(sp), bytes)?;
             *stack = sp;
         }
-        let argc: usize = argv.len();
+        let argc: usize = args.len();
         ustack[argc] = 0;
 
         // push the array of argv[] pointers.
@@ -183,12 +180,9 @@ impl Kernel {
         if sp < stackbase {
             return Err(());
         }
-        pt.copy_out(UVAddr::new(sp), &ustack.align_to::<u8>().1[..argv_size])?;
-
-        // arguments to user main(argc, argv)
-        // argc is returned via the system call return
-        // value, which goes in a0.
-        (*data.trapframe).a1 = sp;
+        // It is safe because any byte can be considered as a valid u8.
+        let (_, ustack, _) = unsafe { ustack.align_to::<u8>() };
+        mem.copy_out(UVAddr::new(sp), &ustack[..argv_size])?;
 
         // Save program name for debugging.
         let path_str = path.as_bytes();
@@ -197,55 +191,28 @@ impl Kernel {
             .rposition(|c| *c == b'/')
             .map(|i| &path_str[(i + 1)..])
             .unwrap_or(path_str);
-        let p_name = &mut (*p).name;
+        let p_name = &mut p.name;
         let len = cmp::min(p_name.len(), name.len());
         p_name[..len].copy_from_slice(&name[..len]);
         if len < p_name.len() {
             p_name[len] = 0;
         }
 
         // Commit to the user image.
-        data.pagetable = pt;
-        data.sz = sz;
+        data.memory = mem;
+
+        // arguments to user main(argc, argv)
+        // argc is returned via the system call return
+        // value, which goes in a0.
+        data.trap_frame_mut().a1 = sp;
 
         // initial program counter = main
-        (*data.trapframe).epc = elf.entry;
+        data.trap_frame_mut().epc = elf.entry;
 
         // initial stack pointer
-        (*data.trapframe).sp = sp;
+        data.trap_frame_mut().sp = sp;
 
         // this ends up in a0, the first argument to main(argc, argv)
         Ok(argc)
     }
 }
-
-/// Load a program segment into pagetable at virtual address va.
-/// va must be page-aligned
-/// and the pages from va to va+sz must already be mapped.
-///
-/// Returns `Ok(())` on success, `Err(())` on failure.
-unsafe fn loadseg(
-    pagetable: &mut PageTable<UVAddr>,
-    va: UVAddr,
-    ip: &mut InodeGuard<'_>,
-    offset: u32,
-    sz: u32,
-) -> Result<(), ()> {
-    assert!(va.is_page_aligned(), "loadseg: va must be page aligned");
-
-    for i in num_iter::range_step(0, sz, PGSIZE as _) {
-        let pa = pagetable
-            .walk_addr(va + i as usize)
-            .expect("loadseg: address should exist")
-            .into_usize();
-
-        let n = cmp::min(sz - i, PGSIZE as _);
-
-        let bytes_read = ip.read(KVAddr::new(pa), offset + i, n)?;
-        if bytes_read != n as _ {
-            return Err(());
-        }
-    }
-
-    Ok(())
-}

kernel-rs/src/file.rs

@@ -75,7 +75,7 @@ impl File {
         match &self.typ {
             FileType::Inode { ip, .. } | FileType::Device { ip, .. } => {
                 let mut st = ip.stat();
-                (*(*p).data.get()).pagetable.copy_out(
+                (*(*p).data.get()).memory.copy_out(
                     addr,
                     slice::from_raw_parts_mut(
                         &mut st as *mut Stat as *mut u8,

kernel-rs/src/kalloc.rs

@@ -4,7 +4,7 @@
 use crate::{
     memlayout::PHYSTOP,
     page::Page,
-    riscv::{pgroundup, PGSIZE},
+    riscv::{pgrounddown, pgroundup, PGSIZE},
 };
 
 use core::mem;
@@ -36,34 +36,40 @@ impl Kmem {
         }
     }
 
+    /// Create pages between `end` and `PHYSTOP`.
+    ///
     /// # Safety
     ///
-    /// Must be called only once. Create pages between `end` and `PHYSTOP` by
-    /// calling freerange.
+    /// There must be no existing pages. It implies that this method should be
+    /// called only once.
     pub unsafe fn init(&mut self) {
-        self.freerange(end.as_mut_ptr(), PHYSTOP as _);
+        // It is safe to acquire only the address of a static variable.
+        let pa_start = pgroundup(unsafe { end.as_ptr() as usize });
+        let pa_end = pgrounddown(PHYSTOP);
+        for pa in num_iter::range_step(pa_start, pa_end, PGSIZE) {
+            // It is safe because
+            // * pa_start is a multiple of PGSIZE, and pa is so
+            // * end <= pa < PHYSTOP
+            // * the safety condition of this method guarantees that the
+            //   created page does not overlap with existing pages
+            self.free(unsafe { Page::from_usize(pa) });
+        }
     }
 
     pub fn free(&mut self, pa: Page) {
-        let mut r = pa.into_usize() as *mut Run;
+        let pa = pa.into_usize();
+        debug_assert!(
+            // It is safe to acquire only the address of a static variable.
+            pa % PGSIZE == 0 && (unsafe { end.as_ptr() as usize }..PHYSTOP).contains(&pa),
+            "Kmem::free"
+        );
+        let mut r = pa as *mut Run;
         // By the invariant of Page, it does not create a cycle in this list and
         // thus is safe.
         unsafe { (*r).next = self.head };
         self.head = r;
     }
 
-    /// # Safety
-    ///
-    /// Create pages between `pa_start` and `pa_end`. Created pages must
-    /// not overwrap with any existing pages.
-    unsafe fn freerange(&mut self, pa_start: *mut u8, pa_end: *mut u8) {
-        let mut p = pgroundup(pa_start as _) as *mut u8;
-        while p.add(PGSIZE) <= pa_end {
-            self.free(Page::from_usize(p as _));
-            p = p.add(PGSIZE);
-        }
-    }
-
     pub fn alloc(&mut self) -> Option<Page> {
         if self.head.is_null() {
             return None;

kernel-rs/src/kernel.rs

@@ -1,25 +1,24 @@
 use core::fmt::{self, Write};
+use core::mem::MaybeUninit;
 use core::sync::atomic::{spin_loop_hint, AtomicBool, Ordering};
 
 use crate::{
     bio::Bcache,
     console::{consoleinit, Console, Printer},
     file::{Devsw, FileTable},
     fs::{FileSystem, Itable},
-    kalloc::{end, Kmem},
-    memlayout::PHYSTOP,
+    kalloc::Kmem,
     page::{Page, RawPage},
     param::{NCPU, NDEV},
     plic::{plicinit, plicinithart},
     println,
     proc::{cpuid, procinit, scheduler, Cpu, ProcessSystem},
-    riscv::PGSIZE,
     sleepablelock::Sleepablelock,
     spinlock::Spinlock,
     trap::{trapinit, trapinithart},
     uart::Uart,
     virtio_disk::virtio_disk_init,
-    vm::{KVAddr, PageTable},
+    vm::KernelMemory,
 };
 
 /// The kernel.
@@ -46,8 +45,8 @@ pub struct Kernel {
 
     kmem: Spinlock<Kmem>,
 
-    /// The kernel's page table.
-    pub page_table: PageTable<KVAddr>,
+    /// The kernel's memory manager.
+    memory: MaybeUninit<KernelMemory>,
 
     pub ticks: Sleepablelock<u32>,
 
@@ -82,7 +81,7 @@ impl Kernel {
             uart: Uart::new(),
             printer: Spinlock::new("PRINTLN", Printer::new()),
             kmem: Spinlock::new("KMEM", Kmem::new()),
-            page_table: unsafe { PageTable::zero() },
+            memory: MaybeUninit::uninit(),
             ticks: Sleepablelock::new("time", 0),
             procs: ProcessSystem::zero(),
             cpus: [Cpu::new(); NCPU],
@@ -111,12 +110,6 @@ impl Kernel {
     /// call to kernel().alloc().  (The exception is when
     /// initializing the allocator; see Kmem::init.)
     pub fn free(&self, mut page: Page) {
-        let pa = page.addr().into_usize();
-        debug_assert!(
-            pa % PGSIZE == 0 && (pa as *mut _) >= unsafe { end.as_mut_ptr() } && pa < PHYSTOP,
-            "[Kernel::free]"
-        );
-
         // Fill with junk to catch dangling refs.
         page.write_bytes(1);
 
@@ -198,11 +191,11 @@ pub unsafe fn kernel_main() -> ! {
         // Physical page allocator.
         KERNEL.kmem.get_mut().init();
 
-        // Create kernel page table.
-        KERNEL.page_table = PageTable::<KVAddr>::new().expect("PageTable::new failed");
+        // Create kernel memory manager.
+        let memory = KernelMemory::new().expect("PageTable::new failed");
 
         // Turn on paging.
-        KERNEL.page_table.init_hart();
+        KERNEL.memory.write(memory).init_hart();
 
         // Process system.
         procinit(&mut KERNEL.procs);
@@ -236,7 +229,7 @@ pub unsafe fn kernel_main() -> ! {
         println!("hart {} starting", cpuid());
 
         // Turn on paging.
-        KERNEL.page_table.init_hart();
+        KERNEL.memory.assume_init_mut().init_hart();
 
         // Install kernel trap vector.
         trapinithart();

kernel-rs/src/lib.rs

@@ -19,38 +19,52 @@
 #![feature(generic_associated_types)]
 #![feature(unsafe_block_in_unsafe_fn)]
 
+// TODO(rv6): We must apply #[deny(unsafe_op_in_unsafe_fn)] to every module.
 mod arena;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod bio;
 mod console;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod etrace;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod exec;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod fcntl;
 mod file;
 mod fs;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod kalloc;
 mod kernel;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod list;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod memlayout;
 mod page;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod param;
 mod pipe;
 mod plic;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod poweroff;
 mod proc;
 mod riscv;
 mod sleepablelock;
 mod sleeplock;
 mod spinlock;
 mod start;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod stat;
 mod syscall;
 mod sysfile;
 mod sysproc;
 mod trap;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod uart;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod utils;
 mod virtio;
 mod virtio_disk;
+#[deny(unsafe_op_in_unsafe_fn)]
 mod vm;
 
 #[macro_use]