From 239bd317e9e0f0878cf6cb3d38223b82940b9441 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Tue, 5 Mar 2024 18:10:24 +0100 Subject: [PATCH] Fix signal ordering wrt syscall return. --- src/arch/x86_64/interrupt/syscall.rs | 6 +-- src/context/memory.rs | 8 +--- src/context/signal.rs | 2 +- src/context/switch.rs | 58 +++++++++++++++++----------- src/main.rs | 58 +++++++++++++--------------- src/scheme/proc.rs | 4 +- src/scheme/user.rs | 5 +-- src/sync/wait_condition.rs | 4 +- src/syscall/futex.rs | 4 +- src/syscall/mod.rs | 12 +++++- src/syscall/process.rs | 2 +- src/syscall/time.rs | 6 +-- 12 files changed, 84 insertions(+), 85 deletions(-) diff --git a/src/arch/x86_64/interrupt/syscall.rs b/src/arch/x86_64/interrupt/syscall.rs index 365bd19102..1bdce36db5 100644 --- a/src/arch/x86_64/interrupt/syscall.rs +++ b/src/arch/x86_64/interrupt/syscall.rs @@ -35,9 +35,9 @@ pub unsafe fn init() { // TF needs to be cleared, as enabling userspace-rflags-controlled singlestep in the kernel // would be a bad idea. // - // AC is not currently used, but when SMAP is enabled, it should always be cleared when - // entering the kernel (and never be set except in usercopy functions), if for some reason AC - // was set before entering userspace (AC can only be modified by kernel code). + // AC it should always be cleared when entering the kernel (and never be set except in usercopy + // functions), if for some reason AC was set before entering userspace (AC can only be modified + // by kernel code). // // The other flags could indeed be preserved and excluded from FMASK, but since they are not // used to pass data to the kernel, they might as well be masked with *marginal* security diff --git a/src/context/memory.rs b/src/context/memory.rs index cddd71fddb..4aca42644b 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -1674,9 +1674,7 @@ impl Grant { let Some((old_flags, phys, flush)) = mapper.remap_with(page.start_address(), |_| flags) else { continue; }; - unsafe { - flush.ignore(); - } + flush.ignore(); //log::info!("Remapped page {:?} (frame {:?})", page, Frame::containing_address(mapper.translate(page.start_address()).unwrap().0)); flusher.queue(Frame::containing_address(phys), None, TlbShootdownActions::change_of_flags(old_flags, flags)); } @@ -2540,9 +2538,7 @@ fn correct_inner<'l>( .write() .hard_block(HardBlockedReason::AwaitingMmap { file_ref }); - unsafe { - super::switch(); - } + super::switch(); let frame = context_lock .write() diff --git a/src/context/signal.rs b/src/context/signal.rs index 9848a49f1c..c23397f711 100644 --- a/src/context/signal.rs +++ b/src/context/signal.rs @@ -118,7 +118,7 @@ pub fn signal_handler() { } } - unsafe { switch() }; + switch(); } _ => { // println!("Exit {}", sig); diff --git a/src/context/switch.rs b/src/context/switch.rs index d783e3f73d..6ccc8f4f3a 100644 --- a/src/context/switch.rs +++ b/src/context/switch.rs @@ -3,7 +3,7 @@ use core::{cell::Cell, mem, ops::Bound, sync::atomic::Ordering}; use spinning_top::guard::ArcRwSpinlockWriteGuard; use crate::{ - context::{self, arch, contexts, Context}, + context::{arch, contexts, Context}, cpu_set::LogicalCpuId, interrupt, percpu::PercpuBlock, @@ -65,7 +65,7 @@ unsafe fn update_runnable(context: &mut Context, cpu_id: LogicalCpuId) -> Update } } -struct SwitchResult { +struct SwitchResultInner { _prev_guard: ArcRwSpinlockWriteGuard, _next_guard: ArcRwSpinlockWriteGuard, } @@ -78,7 +78,12 @@ pub fn tick() { // Switch after 3 ticks (about 6.75 ms) if new_ticks >= 3 { - let _ = unsafe { switch() }; + match switch() { + SwitchResult::Switched { signal: true } => { + crate::context::signal::signal_handler(); + }, + _ => (), + } } } @@ -93,12 +98,16 @@ pub unsafe extern "C" fn switch_finish_hook() { arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst); } -/// Switch to the next context +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum SwitchResult { + Switched { signal: bool }, + AllContextsIdle, +} + +/// Switch to the next context, picked by the scheduler. /// -/// # Safety -/// -/// Do not call this while holding locks! -pub unsafe fn switch() -> bool { +/// This is not memory-unsafe to call, but do NOT call this while holding locks! +pub fn switch() -> SwitchResult { let percpu = PercpuBlock::current(); //set PIT Interrupt counter to 0, giving each process same amount of PIT ticks @@ -156,7 +165,7 @@ pub unsafe fn switch() -> bool { let mut next_context_guard = next_context_lock.write_arc(); // Update state of next context and check if runnable - if let UpdateResult::CanSwitch { signal } = update_runnable(&mut *next_context_guard, cpu_id) { + if let UpdateResult::CanSwitch { signal } = unsafe { update_runnable(&mut *next_context_guard, cpu_id) } { // Store locks for previous and next context switch_context_opt = Some((prev_context_guard, next_context_guard)); percpu.switch_internals.switch_signal.set(signal); @@ -169,6 +178,8 @@ pub unsafe fn switch() -> bool { // Switch process states, TSS stack pointer, and store new context ID if let Some((mut prev_context_guard, mut next_context_guard)) = switch_context_opt { + // TODO: Update timestamps in switch_to + // Set old context as not running and update CPU time let prev_context = &mut *prev_context_guard; prev_context.running = false; @@ -184,20 +195,24 @@ pub unsafe fn switch() -> bool { percpu.switch_internals.context_id.set(next_context.id); // FIXME set th switch result in arch::switch_to instead - let prev_context = - mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *prev_context_guard); - let next_context = - mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *next_context_guard); + let prev_context = unsafe { + mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *prev_context_guard) + }; + let next_context = unsafe { + mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *next_context_guard) + }; percpu .switch_internals .switch_result - .set(Some(SwitchResult { + .set(Some(SwitchResultInner { _prev_guard: prev_context_guard, _next_guard: next_context_guard, })); - arch::switch_to(prev_context, next_context); + unsafe { + arch::switch_to(prev_context, next_context); + } // NOTE: After switch_to is called, the return address can even be different from the // current return address, meaning that we cannot use local variables here, and that we @@ -205,24 +220,21 @@ pub unsafe fn switch() -> bool { // contexts will return directly to the function pointer passed to context::spawn, and not // reach this code until the next context switch back. - // We can't reuse the `percpu` variable, since it's from the stack before the last - // context::switch, and can thus point to another CPU's percpu block. - if PercpuBlock::current().switch_internals.switch_signal.replace(false) { - context::signal::signal_handler(); - } + let new_percpu = PercpuBlock::current(); + // For the same reason, we obviously can't reuse the percpu block - true + SwitchResult::Switched { signal: new_percpu.switch_internals.switch_signal.get() } } else { // No target was found, unset global lock and return arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst); - false + SwitchResult::AllContextsIdle } } #[derive(Default)] pub struct ContextSwitchPercpu { - switch_result: Cell>, + switch_result: Cell>, pit_ticks: Cell, /// Unique ID of the currently running context. diff --git a/src/main.rs b/src/main.rs index c74ee3977b..d11f62a96d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -61,6 +61,7 @@ extern crate bitflags; use core::sync::atomic::{AtomicU32, Ordering}; +use crate::context::switch::SwitchResult; use crate::scheme::SchemeNamespace; use crate::consts::*; @@ -208,17 +209,7 @@ fn kmain(cpu_count: u32, bootstrap: Bootstrap) -> ! { } } - loop { - unsafe { - interrupt::disable(); - if context::switch() { - interrupt::enable_and_nop(); - } else { - // Enable interrupts, then halt CPU (to save power) until the next interrupt is actually fired. - interrupt::enable_and_halt(); - } - } - } + run_userspace() } /// This is the main kernel entry point for secondary CPUs @@ -227,27 +218,7 @@ fn kmain_ap(cpu_id: crate::cpu_set::LogicalCpuId) -> ! { #[cfg(feature = "profiling")] profiling::maybe_run_profiling_helper_forever(cpu_id); - if cfg!(feature = "multi_core") { - context::init(); - - let pid = syscall::getpid(); - info!("AP {}: {:?}", cpu_id, pid); - - #[cfg(feature = "profiling")] - profiling::ready_for_profiling(); - - loop { - unsafe { - interrupt::disable(); - if context::switch() { - interrupt::enable_and_nop(); - } else { - // Enable interrupts, then halt CPU (to save power) until the next interrupt is actually fired. - interrupt::enable_and_halt(); - } - } - } - } else { + if !cfg!(feature = "multi_core") { info!("AP {}: Disabled", cpu_id); loop { @@ -257,6 +228,29 @@ fn kmain_ap(cpu_id: crate::cpu_set::LogicalCpuId) -> ! { } } } + context::init(); + + let pid = syscall::getpid(); + info!("AP {}: {:?}", cpu_id, pid); + + #[cfg(feature = "profiling")] + profiling::ready_for_profiling(); + + run_userspace(); +} +fn run_userspace() -> ! { + loop { + unsafe { + interrupt::disable(); + match context::switch() { + SwitchResult::Switched { .. } => interrupt::enable_and_nop(), + SwitchResult::AllContextsIdle => { + // Enable interrupts, then halt CPU (to save power) until the next interrupt is actually fired. + interrupt::enable_and_halt(); + } + } + } + } } /// Allow exception handlers to send signal to arch-independent kernel diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index 5afef310c2..5f8b7c3c32 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -87,9 +87,7 @@ where // Wait until stopped while running { - unsafe { - context::switch(); - } + context::switch(); running = with_context(pid, |context| Ok(context.running))?; } diff --git a/src/scheme/user.rs b/src/scheme/user.rs index ad8ea3700f..bd40a82250 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -188,9 +188,7 @@ impl UserInner { event::trigger(self.root_id, self.handle_id, EVENT_READ); loop { - unsafe { - context::switch(); - } + context::switch(); let mut states = self.states.lock(); match states.entry(id) { @@ -204,7 +202,6 @@ impl UserInner { } // spurious wakeup State::Waiting { canceling: false, fd, context } => { - log::info!("EINTR"); *o.get_mut() = State::Waiting { canceling: true, fd, context }; // TODO: Is this too dangerous when the states lock is held? diff --git a/src/sync/wait_condition.rs b/src/sync/wait_condition.rs index 7ffedb676f..b6fb71b00e 100644 --- a/src/sync/wait_condition.rs +++ b/src/sync/wait_condition.rs @@ -57,9 +57,7 @@ impl WaitCondition { drop(guard); } - unsafe { - context::switch(); - } + context::switch(); let mut waited = true; diff --git a/src/syscall/futex.rs b/src/syscall/futex.rs index a609e1ddd6..1e6dd601c7 100644 --- a/src/syscall/futex.rs +++ b/src/syscall/futex.rs @@ -137,9 +137,7 @@ pub fn futex(addr: usize, op: usize, val: usize, val2: usize, addr2: usize) -> R drop(addr_space_guard); - unsafe { - context::switch(); - } + context::switch(); if timeout_opt.is_some() { let context_lock = { diff --git a/src/syscall/mod.rs b/src/syscall/mod.rs index 8b250fca5a..ef32c17624 100644 --- a/src/syscall/mod.rs +++ b/src/syscall/mod.rs @@ -14,16 +14,16 @@ pub use self::{ use self::{ data::{Map, SigAction, TimeSpec}, - error::{Error, Result, EOVERFLOW, ENOSYS}, + error::{Error, Result, EINTR, EOVERFLOW, ENOSYS}, flag::{EventFlags, MapFlags, WaitFlags}, number::*, + usercopy::UserSlice, }; use crate::{ context::{memory::AddrSpace, ContextId}, interrupt::InterruptStack, scheme::{memory::MemoryScheme, FileHandle, SchemeNamespace}, - syscall::usercopy::UserSlice, }; /// Debug @@ -285,6 +285,14 @@ pub fn syscall( let result = inner(a, b, c, d, e, f, stack); + if result == Err(Error::new(EINTR)) { + // Although it would be cleaner to simply run the signal trampoline right after switching + // back to any given context, where the signal set/queue is nonempty, syscalls need to + // complete *before* any signal is delivered. Otherwise the return value would probably be + // overwritten. + crate::context::signal::signal_handler(); + } + { let contexts = crate::context::contexts(); if let Some(context_lock) = contexts.current() { diff --git a/src/syscall/process.rs b/src/syscall/process.rs index d9a1007cf5..e8366b6a82 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -247,7 +247,7 @@ pub fn kill(pid: ContextId, sig: usize) -> Result { Err(Error::new(EPERM)) } else { // Switch to ensure delivery to self - unsafe { context::switch(); } + context::switch(); Ok(0) } diff --git a/src/syscall/time.rs b/src/syscall/time.rs index f8083d4c83..0160c02c59 100644 --- a/src/syscall/time.rs +++ b/src/syscall/time.rs @@ -40,7 +40,7 @@ pub fn nanosleep(req_buf: UserSliceRo, rem_buf_opt: Option) -> Resu // TODO: The previous wakeup reason was most likely signals, but is there any other possible // reason? - unsafe { context::switch(); } + context::switch(); if current_context.write().wake.take().is_some() { return Err(Error::new(EINTR)); @@ -67,8 +67,6 @@ pub fn nanosleep(req_buf: UserSliceRo, rem_buf_opt: Option) -> Resu } pub fn sched_yield() -> Result<()> { - unsafe { - context::switch(); - } + context::switch(); Ok(()) }