From 8107d0993c2a493f79fb63ceac4f2e2765aa735b Mon Sep 17 00:00:00 2001 From: gjz010 Date: Tue, 11 May 2021 01:57:34 +0800 Subject: [PATCH] bugfix for race demand-page access. --- crate/memory/src/memory_set/handler/delay.rs | 14 ++++- crate/memory/src/memory_set/handler/file.rs | 16 +++++- crate/memory/src/memory_set/handler/mod.rs | 57 ++++++++++++++++++- crate/memory/src/memory_set/handler/shared.rs | 2 +- crate/memory/src/memory_set/mod.rs | 9 +++ kernel/src/arch/riscv/interrupt/consts.rs | 9 +++ kernel/src/arch/riscv/interrupt/mod.rs | 25 +++++--- kernel/src/process/thread.rs | 38 ++++++++++--- 8 files changed, 148 insertions(+), 22 deletions(-) diff --git a/crate/memory/src/memory_set/handler/delay.rs b/crate/memory/src/memory_set/handler/delay.rs index 2eb8b341..fa66043a 100644 --- a/crate/memory/src/memory_set/handler/delay.rs +++ b/crate/memory/src/memory_set/handler/delay.rs @@ -49,10 +49,20 @@ impl MemoryHandler for Delay { } } - fn handle_page_fault(&self, pt: &mut dyn PageTable, addr: VirtAddr) -> bool { + fn handle_page_fault_ext( + &self, + pt: &mut dyn PageTable, + addr: VirtAddr, + access: super::AccessType, + ) -> bool { let entry = pt.get_entry(addr).expect("failed to get entry"); if entry.present() { - // not a delay case + // permission check. + if access.check_access(entry) { + return true; + } + // permisison check failed. + error!("Permission check failed at 0x{:x}.", addr); return false; } let frame = self.allocator.alloc().expect("failed to alloc frame"); diff --git a/crate/memory/src/memory_set/handler/file.rs b/crate/memory/src/memory_set/handler/file.rs index c3d1acd4..55871896 100644 --- a/crate/memory/src/memory_set/handler/file.rs +++ b/crate/memory/src/memory_set/handler/file.rs @@ -58,10 +58,24 @@ impl MemoryHandler for File { } } - fn handle_page_fault(&self, pt: &mut dyn PageTable, addr: usize) -> bool { + fn handle_page_fault_ext( + &self, + pt: &mut dyn PageTable, + addr: usize, + access: super::AccessType, + ) -> bool { let addr = addr & !(PAGE_SIZE - 1); let entry = pt.get_entry(addr).expect("failed to get entry"); if entry.present() { + // permission check. + if access.check_access(entry) { + return true; + } + // permisison check failed. + error!( + "Permission check failed at 0x{:x}, access = {:?}.", + addr, access + ); return false; } let execute = entry.execute(); diff --git a/crate/memory/src/memory_set/handler/mod.rs b/crate/memory/src/memory_set/handler/mod.rs index 79ee6460..a7b5d9fc 100644 --- a/crate/memory/src/memory_set/handler/mod.rs +++ b/crate/memory/src/memory_set/handler/mod.rs @@ -1,5 +1,45 @@ use super::*; - +#[derive(Copy, Clone, Debug)] +pub struct AccessType { + pub write: bool, + pub execute: bool, + pub user: bool, +} +impl AccessType { + pub fn unknown() -> Self { + AccessType { + write: true, + execute: true, + user: true, + } + } + pub fn read(user: bool) -> Self { + AccessType { + write: false, + execute: false, + user, + } + } + pub fn write(user: bool) -> Self { + AccessType { + write: true, + execute: false, + user, + } + } + pub fn execute(user: bool) -> Self { + AccessType { + write: false, + execute: true, + user, + } + } + pub fn check_access(self, entry: &dyn paging::Entry) -> bool { + ((!self.write) || entry.writable()) + && ((!self.execute) || entry.execute()) + && ((!self.user) || entry.user()) + } +} // here may be a interesting part for lab pub trait MemoryHandler: Debug + Send + Sync + 'static { fn box_clone(&self) -> Box; @@ -22,7 +62,20 @@ pub trait MemoryHandler: Debug + Send + Sync + 'static { /// Handle page fault on `addr` /// Return true if success, false if error - fn handle_page_fault(&self, pt: &mut dyn PageTable, addr: VirtAddr) -> bool; + fn handle_page_fault(&self, pt: &mut dyn PageTable, addr: VirtAddr) -> bool { + self.handle_page_fault_ext(pt, addr, AccessType::unknown()) + } + + /// Handle page fault on `addr` and access type `access` + /// Return true if success (or should-retry), false if error + fn handle_page_fault_ext( + &self, + pt: &mut dyn PageTable, + addr: VirtAddr, + _access: AccessType, + ) -> bool { + self.handle_page_fault(pt, addr) + } } impl Clone for Box { diff --git a/crate/memory/src/memory_set/handler/shared.rs b/crate/memory/src/memory_set/handler/shared.rs index f3deae13..e9ee61db 100644 --- a/crate/memory/src/memory_set/handler/shared.rs +++ b/crate/memory/src/memory_set/handler/shared.rs @@ -35,7 +35,7 @@ impl SharedGuard { self.target.insert(virt_addr, phys_addr); Some(phys_addr) } - + pub fn dealloc(&mut self, virt_addr: usize) { let phys_addr = self.target.get(&virt_addr).unwrap().clone(); self.allocator.dealloc(phys_addr); diff --git a/crate/memory/src/memory_set/mod.rs b/crate/memory/src/memory_set/mod.rs index f6324bb4..a4083317 100644 --- a/crate/memory/src/memory_set/mod.rs +++ b/crate/memory/src/memory_set/mod.rs @@ -376,6 +376,15 @@ impl MemorySet { &mut self.page_table } + pub fn handle_page_fault_ext(&mut self, addr: VirtAddr, access: handler::AccessType) -> bool { + let area = self.areas.iter().find(|area| area.contains(addr)); + match area { + Some(area) => area + .handler + .handle_page_fault_ext(&mut self.page_table, addr, access), + None => false, + } + } pub fn handle_page_fault(&mut self, addr: VirtAddr) -> bool { let area = self.areas.iter().find(|area| area.contains(addr)); match area { diff --git a/kernel/src/arch/riscv/interrupt/consts.rs b/kernel/src/arch/riscv/interrupt/consts.rs index 01e20bc9..bcfc197e 100644 --- a/kernel/src/arch/riscv/interrupt/consts.rs +++ b/kernel/src/arch/riscv/interrupt/consts.rs @@ -14,6 +14,15 @@ pub fn is_page_fault(trap: usize) -> bool { trap == InstructionPageFault || trap == LoadPageFault || trap == StorePageFault } +pub fn is_execute_page_fault(trap: usize) -> bool { + trap == InstructionPageFault +} +pub fn is_read_page_fault(trap: usize) -> bool { + trap == LoadPageFault +} +pub fn is_write_page_fault(trap: usize) -> bool { + trap == StorePageFault +} pub fn is_syscall(trap: usize) -> bool { trap == Syscall } diff --git a/kernel/src/arch/riscv/interrupt/mod.rs b/kernel/src/arch/riscv/interrupt/mod.rs index 1dc70338..cb320a6a 100644 --- a/kernel/src/arch/riscv/interrupt/mod.rs +++ b/kernel/src/arch/riscv/interrupt/mod.rs @@ -39,20 +39,27 @@ pub extern "C" fn trap_handler(tf: &mut TrapFrame) { trap_handler_no_frame(&mut tf.sepc); } +use crate::memory::AccessType; #[inline] pub fn trap_handler_no_frame(sepc: &mut usize) { use self::scause::{Exception as E, Interrupt as I, Trap}; let scause = scause::read(); let stval = stval::read(); + let is_user = false; trace!("Interrupt @ CPU{}: {:?} ", super::cpu::id(), scause.cause()); match scause.cause() { Trap::Interrupt(I::SupervisorExternal) => external(), Trap::Interrupt(I::SupervisorSoft) => ipi(), Trap::Interrupt(I::SupervisorTimer) => timer(), - Trap::Exception(E::LoadPageFault) => page_fault(stval, sepc), - Trap::Exception(E::StorePageFault) => page_fault(stval, sepc), - Trap::Exception(E::InstructionPageFault) => page_fault(stval, sepc), - _ => panic!("unhandled trap {:?}", scause.cause()), + Trap::Exception(E::LoadPageFault) => page_fault(stval, sepc, AccessType::read(is_user)), + Trap::Exception(E::StorePageFault) => page_fault(stval, sepc, AccessType::write(is_user)), + Trap::Exception(E::InstructionPageFault) => { + page_fault(stval, sepc, AccessType::execute(is_user)) + } + _ => { + let bits = scause.bits(); + panic!("unhandled trap {:?} ({})", scause.cause(), bits); + } } trace!("Interrupt end"); } @@ -62,7 +69,6 @@ fn external() { unsafe { super::board::handle_external_interrupt(); } - IRQ_MANAGER .read() .try_handle_interrupt(Some(SupervisorExternal)); @@ -78,11 +84,11 @@ pub fn timer() { crate::trap::timer(); } -fn page_fault(stval: usize, sepc: &mut usize) { +fn page_fault(stval: usize, sepc: &mut usize, access: AccessType) { let addr = stval; info!("\nEXCEPTION: Page Fault @ {:#x}", addr); - if crate::memory::handle_page_fault(addr) { + if crate::memory::handle_page_fault_ext(addr, access) { return; } extern "C" { @@ -94,6 +100,7 @@ fn page_fault(stval: usize, sepc: &mut usize) { *sepc = crate::memory::read_user_fixup as usize; return; } + error!("unhandled page fault {:#x} from {:#x}", addr, sepc); panic!("unhandled page fault"); } @@ -121,8 +128,8 @@ pub fn wait_for_interrupt() { } } -pub fn handle_user_page_fault(thread: &Arc, addr: usize) -> bool { - thread.vm.lock().handle_page_fault(addr) +pub fn handle_user_page_fault_ext(thread: &Arc, addr: usize, access: AccessType) -> bool { + thread.vm.lock().handle_page_fault_ext(addr, access) } pub fn handle_reserved_inst(tf: &mut UserContext) -> bool { diff --git a/kernel/src/process/thread.rs b/kernel/src/process/thread.rs index 3891aae2..adb598ee 100644 --- a/kernel/src/process/thread.rs +++ b/kernel/src/process/thread.rs @@ -5,7 +5,7 @@ use super::{ use crate::arch::interrupt::consts::{ is_intr, is_page_fault, is_reserved_inst, is_syscall, is_timer_intr, }; -use crate::arch::interrupt::{get_trap_num, handle_reserved_inst, handle_user_page_fault}; +use crate::arch::interrupt::{get_trap_num, handle_reserved_inst}; use crate::arch::{ cpu, fp::FpState, @@ -505,10 +505,8 @@ pub fn spawn(thread: Arc) { thread_context.fp.restore(); cx.run(); thread_context.fp.save(); - let trap_num = get_trap_num(&cx); trace!("back from user: {:#x?} trap_num {:#x}", cx, trap_num); - let mut exit = false; let mut do_yield = false; match trap_num { @@ -517,10 +515,36 @@ pub fn spawn(thread: Arc) { // page fault let addr = get_page_fault_addr(); info!("page fault from user @ {:#x}", addr); - - if !handle_user_page_fault(&thread, addr) { - // TODO: SIGSEGV - panic!("page fault handle failed"); + #[cfg(any(target_arch = "riscv32", target_arch = "riscv64"))] + { + use crate::arch::interrupt::consts::{ + is_execute_page_fault, is_read_page_fault, is_write_page_fault, + }; + use crate::arch::interrupt::handle_user_page_fault_ext; + let access_type = match trap_num { + _ if is_execute_page_fault(trap_num) => { + crate::memory::AccessType::execute(true) + } + _ if is_read_page_fault(trap_num) => { + crate::memory::AccessType::read(true) + } + _ if is_write_page_fault(trap_num) => { + crate::memory::AccessType::write(true) + } + _ => unreachable!(), + }; + if !handle_user_page_fault_ext(&thread, addr, access_type) { + // TODO: SIGSEGV + panic!("page fault handle failed"); + } + } + #[cfg(not(any(target_arch = "riscv32", target_arch = "riscv64")))] + { + use crate::arch::interrupt::handle_user_page_fault; + if !handle_user_page_fault(&thread, addr) { + // TODO: SIGSEGV + panic!("page fault handle failed"); + } } } _ if is_syscall(trap_num) => exit = handle_syscall(&thread, cx).await,