Fix signal ordering wrt syscall return.

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

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()

src/context/signal.rs

@@ -118,7 +118,7 @@ pub fn signal_handler() {
                     }
                 }
 
-                unsafe { switch() };
+                switch();
             }
             _ => {
                 // println!("Exit {}", sig);

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<Context>,
     _next_guard: ArcRwSpinlockWriteGuard<Context>,
 }
@@ -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<Option<SwitchResult>>,
+    switch_result: Cell<Option<SwitchResultInner>>,
     pit_ticks: Cell<usize>,
 
     /// Unique ID of the currently running context.

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

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))?;
     }

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?

src/sync/wait_condition.rs

@@ -57,9 +57,7 @@ impl WaitCondition {
             drop(guard);
         }
 
-        unsafe {
-            context::switch();
-        }
+        context::switch();
 
         let mut waited = true;
 

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 = {

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() {

src/syscall/process.rs

@@ -247,7 +247,7 @@ pub fn kill(pid: ContextId, sig: usize) -> Result<usize> {
         Err(Error::new(EPERM))
     } else {
         // Switch to ensure delivery to self
-        unsafe { context::switch(); }
+        context::switch();
 
         Ok(0)
     }

src/syscall/time.rs

@@ -40,7 +40,7 @@ pub fn nanosleep(req_buf: UserSliceRo, rem_buf_opt: Option<UserSliceWo>) -> 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<UserSliceWo>) -> Resu
 }
 
 pub fn sched_yield() -> Result<()> {
-    unsafe {
-        context::switch();
-    }
+    context::switch();
     Ok(())
 }