rewrite(signal_list): provide better encapsulation

feat(syscall): implement syscall `sigreturn`

src/kernel/constants.rs

@@ -13,6 +13,8 @@ pub const SIG_BLOCK: u32 = 0;
 pub const SIG_UNBLOCK: u32 = 1;
 pub const SIG_SETMASK: u32 = 2;
 
+pub const SA_SIGINFO: u32 = 4;
+
 pub const CLOCK_REALTIME: u32 = 0;
 pub const CLOCK_MONOTONIC: u32 = 1;
 

src/kernel/syscall/procops.rs

@@ -360,10 +360,41 @@ fn do_rt_sigprocmask(how: u32, set: *mut u64, oldset: *mut u64, sigsetsize: usiz
     Ok(())
 }
 
+#[repr(C, packed)]
+#[derive(Debug, Clone, Copy)]
+struct UserSignalAction {
+    sa_handler: u32,
+    sa_flags: u32,
+    sa_restorer: u32,
+    sa_mask: u64,
+}
+
+impl From<UserSignalAction> for SignalAction {
+    fn from(from: UserSignalAction) -> SignalAction {
+        SignalAction {
+            sa_handler: from.sa_handler as usize,
+            sa_flags: from.sa_flags as usize,
+            sa_mask: from.sa_mask as usize,
+            sa_restorer: from.sa_restorer as usize,
+        }
+    }
+}
+
+impl From<SignalAction> for UserSignalAction {
+    fn from(from: SignalAction) -> UserSignalAction {
+        UserSignalAction {
+            sa_handler: from.sa_handler as u32,
+            sa_flags: from.sa_flags as u32,
+            sa_mask: from.sa_mask as u64,
+            sa_restorer: from.sa_restorer as u32,
+        }
+    }
+}
+
 fn do_rt_sigaction(
     signum: u32,
-    act: *const SignalAction,
-    oldact: *mut SignalAction,
+    act: *const UserSignalAction,
+    oldact: *mut UserSignalAction,
     sigsetsize: usize,
 ) -> KResult<()> {
     let signal = Signal::try_from(signum)?;
@@ -373,14 +404,14 @@ fn do_rt_sigaction(
 
     let old_action = Thread::current().signal_list.get_handler(signal);
     if !oldact.is_null() {
-        UserPointerMut::new(oldact)?.write(old_action)?;
+        UserPointerMut::new(oldact)?.write(old_action.into())?;
     }
 
     if !act.is_null() {
         let new_action = UserPointer::new(act)?.read()?;
         Thread::current()
             .signal_list
-            .set_handler(signal, &new_action)?;
+            .set_handler(signal, &new_action.into())?;
     }
 
     Ok(())
@@ -413,7 +444,7 @@ define_syscall32!(sys_tkill, do_tkill, tid: u32, sig: u32);
 define_syscall32!(sys_rt_sigprocmask, do_rt_sigprocmask,
     how: u32, set: *mut u64, oldset: *mut u64, sigsetsize: usize);
 define_syscall32!(sys_rt_sigaction, do_rt_sigaction,
-    signum: u32, act: *const SignalAction, oldact: *mut SignalAction, sigsetsize: usize);
+    signum: u32, act: *const UserSignalAction, oldact: *mut UserSignalAction, sigsetsize: usize);
 
 extern "C" {
     fn ISR_stub_restore();
@@ -473,6 +504,18 @@ fn sys_fork(int_stack: &mut interrupt_stack, mmxregs: &mut mmx_registers) -> usi
     new_thread.process.pid as usize
 }
 
+fn sys_sigreturn(int_stack: &mut interrupt_stack, mmxregs: &mut mmx_registers) -> usize {
+    let result = Thread::current().signal_list.restore(int_stack, mmxregs);
+    match result {
+        Ok(ret) => ret,
+        Err(_) => {
+            println_warn!("`sigreturn` failed in thread {}!", Thread::current().tid);
+            Thread::current().raise(Signal::SIGSEGV);
+            0
+        }
+    }
+}
+
 pub(super) fn register() {
     register_syscall!(0x01, exit);
     register_syscall!(0x02, fork);
@@ -488,6 +531,7 @@ pub(super) fn register() {
     register_syscall!(0x40, getppid);
     register_syscall!(0x42, setsid);
     register_syscall!(0x72, wait4);
+    register_syscall!(0x77, sigreturn);
     register_syscall!(0x84, getpgid);
     register_syscall!(0x93, getsid);
     register_syscall!(0xac, prctl);

src/kernel/task/scheduler.rs

@@ -108,7 +108,7 @@ impl Scheduler {
         let mut state = thread.state.lock();
 
         match *state {
-            ThreadState::Running | ThreadState::USleep => return,
+            ThreadState::Ready | ThreadState::Running | ThreadState::USleep => return,
             ThreadState::ISleep => {
                 *state = ThreadState::Ready;
                 self.enqueue(&thread);

src/kernel/task/signal.rs

@@ -1,6 +1,13 @@
 use core::cmp::Reverse;
 
-use crate::{io::BufferFill, kernel::user::dataflow::UserBuffer, prelude::*};
+use crate::{
+    io::BufferFill,
+    kernel::{
+        constants::SA_SIGINFO,
+        user::{dataflow::UserBuffer, UserPointer},
+    },
+    prelude::*,
+};
 
 use alloc::collections::{binary_heap::BinaryHeap, btree_map::BTreeMap};
 use bindings::{
@@ -52,10 +59,10 @@ impl Signal {
 
 #[derive(Debug, Clone, Copy)]
 pub struct SignalAction {
-    sa_handler: usize,
-    sa_flags: usize,
-    sa_restorer: usize,
-    sa_mask: usize,
+    pub sa_handler: usize,
+    pub sa_flags: usize,
+    pub sa_restorer: usize,
+    pub sa_mask: usize,
 }
 
 #[derive(Debug, Clone)]
@@ -160,6 +167,7 @@ impl SignalAction {
     fn handle(
         &self,
         signum: u32,
+        old_mask: u64,
         int_stack: &mut interrupt_stack,
         mmxregs: &mut mmx_registers,
     ) -> KResult<()> {
@@ -169,19 +177,19 @@ impl SignalAction {
 
         const CONTEXT_SIZE: usize = size_of::<interrupt_stack>()
             + size_of::<mmx_registers>()
-            + 2 * size_of::<u32>() // Signum and address of sa_restorer
-            + size_of::<usize>(); // Original RSP
+            + size_of::<usize>() // old_mask
+            + size_of::<u32>(); // `sa_handler` argument: `signum`
 
         // Save current interrupt context to 128 bytes above current user stack
-        // and align to 16 bytes
+        // and align to 16 bytes. Then we push the return address of the restorer.
         // TODO!!!: Determine the size of the return address
-        let sp = (int_stack.rsp - (128 + CONTEXT_SIZE + size_of::<u32>())) & !0xf;
+        let sp = ((int_stack.rsp - 128 - CONTEXT_SIZE) & !0xf) - size_of::<u32>();
         let restorer_address: u32 = self.sa_restorer as u32;
-        let mut stack = UserBuffer::new(sp as *mut _, CONTEXT_SIZE)?;
+        let mut stack = UserBuffer::new(sp as *mut _, CONTEXT_SIZE + size_of::<u32>())?;
 
         stack.copy(&restorer_address)?.ok_or(EFAULT)?; // Restorer address
-        stack.copy(&signum)?.ok_or(EFAULT)?; // Signal number
-        stack.copy(&int_stack.rsp)?.ok_or(EFAULT)?; // Original RSP
+        stack.copy(&signum)?.ok_or(EFAULT)?; // Restorer address
+        stack.copy(&old_mask)?.ok_or(EFAULT)?; // Original signal mask
         stack.copy(mmxregs)?.ok_or(EFAULT)?; // MMX registers
         stack.copy(int_stack)?.ok_or(EFAULT)?; // Interrupt stack
 
@@ -233,7 +241,7 @@ impl SignalListInner {
         self.pending.push(Reverse(signal));
 
         if signal.is_stop() {
-            return RaiseResult::Finished;
+            return RaiseResult::ShouldIWakeUp;
         }
 
         // TODO!!!!!!: Fix this. SIGCONT could wake up USleep threads.
@@ -273,7 +281,7 @@ impl SignalList {
     }
 
     pub fn set_handler(&self, signal: Signal, action: &SignalAction) -> KResult<()> {
-        if signal.is_now() {
+        if signal.is_now() || action.sa_flags & SA_SIGINFO as usize != 0 {
             return Err(EINVAL);
         }
 
@@ -328,28 +336,40 @@ impl SignalList {
     pub fn handle(&self, int_stack: &mut interrupt_stack, mmxregs: &mut mmx_registers) {
         loop {
             let signal = {
-                let mut inner = self.inner.lock_irq();
-                let signal = match inner.pop() {
+                let signal = match self.inner.lock_irq().pop() {
                     Some(signal) => signal,
                     None => return,
                 };
 
-                if let Some(handler) = inner.handlers.get(&signal) {
+                let handler = self.inner.lock_irq().handlers.get(&signal).cloned();
+                if let Some(handler) = handler {
                     if !signal.is_now() {
-                        let result = handler.handle(signal.to_signum(), int_stack, mmxregs);
+                        let old_mask = {
+                            let mut inner = self.inner.lock_irq();
+                            let old_mask = inner.mask;
+                            inner.mask(handler.sa_mask as u64);
+                            old_mask
+                        };
+                        let result =
+                            handler.handle(signal.to_signum(), old_mask, int_stack, mmxregs);
+                        if result.is_err() {
+                            self.inner.lock_irq().set_mask(old_mask);
+                        }
                         match result {
-                            Err(EFAULT) => inner.raise(Signal::SIGSEGV),
-                            Err(_) => inner.raise(Signal::SIGSYS),
+                            Err(EFAULT) => self.inner.lock_irq().raise(Signal::SIGSEGV),
+                            Err(_) => self.inner.lock_irq().raise(Signal::SIGSYS),
                             Ok(()) => return,
                         };
                         continue;
                     }
                 }
 
+                // TODO: The default signal handling process should be atomic.
+
                 // Default actions include stopping the thread, continuing the thread and
                 // terminating the process. All these actions will block the thread or return
                 // to the thread immediately. So we can unmask these signals now.
-                inner.unmask(signal.to_mask());
+                self.inner.lock_irq().unmask(signal.to_mask());
                 signal
             };
 
@@ -370,4 +390,24 @@ impl SignalList {
             }
         }
     }
+
+    /// Load the signal mask, MMX registers and interrupt stack from the user stack.
+    /// We must be here because `sigreturn` is called. Se we return the value of the register
+    /// used to store the syscall return value to prevent the original value being clobbered.
+    pub fn restore(
+        &self,
+        int_stack: &mut interrupt_stack,
+        mmxregs: &mut mmx_registers,
+    ) -> KResult<usize> {
+        let old_mask_vaddr = int_stack.rsp;
+        let old_mmxregs_vaddr = old_mask_vaddr + size_of::<usize>();
+        let old_int_stack_vaddr = old_mmxregs_vaddr + size_of::<mmx_registers>();
+
+        let old_mask = UserPointer::<u64>::new_vaddr(old_mask_vaddr)?.read()?;
+        *mmxregs = UserPointer::<mmx_registers>::new_vaddr(old_mmxregs_vaddr)?.read()?;
+        *int_stack = UserPointer::<interrupt_stack>::new_vaddr(old_int_stack_vaddr)?.read()?;
+
+        self.inner.lock_irq().set_mask(old_mask);
+        Ok(int_stack.regs.rax as usize)
+    }
 }

src/kernel/task/thread.rs

@@ -16,7 +16,7 @@ use crate::{
         vfs::FsContext,
     },
     prelude::*,
-    sync::{preempt, CondVar},
+    sync::{preempt, CondVar, SpinGuard},
 };
 
 use alloc::{
@@ -134,6 +134,25 @@ struct ProcessInner {
 pub struct WaitList {
     wait_procs: Spin<VecDeque<WaitObject>>,
     cv_wait_procs: CondVar,
+    process: Weak<Process>,
+}
+
+pub struct NotifyBatch<'waitlist, 'cv, 'process> {
+    wait_procs: SpinGuard<'waitlist, VecDeque<WaitObject>>,
+    cv: &'cv CondVar,
+    process: &'process Weak<Process>,
+    needs_notify: bool,
+}
+
+pub struct Entry<'waitlist, 'cv> {
+    wait_procs: SpinGuard<'waitlist, VecDeque<WaitObject>>,
+    cv: &'cv CondVar,
+    want_stop: bool,
+    want_continue: bool,
+}
+
+pub struct DrainExited<'waitlist> {
+    wait_procs: SpinGuard<'waitlist, VecDeque<WaitObject>>,
 }
 
 #[derive(Debug)]
@@ -484,50 +503,35 @@ impl ProcessList {
         process.mm_list.clear_user();
 
         // Make children orphans (adopted by init)
-        let mut init_inner = self.init.inner.lock();
-
-        inner.children.retain(|_, child| {
-            let child = child.upgrade().unwrap();
-            let mut child_inner = child.process.inner.lock();
-            if child_inner.parent.as_ref().unwrap() == &self.init {
-                return false;
-            }
-
-            child_inner.parent = Some(self.init.clone());
-            init_inner.add_child(&child);
+        {
+            let mut init_inner = self.init.inner.lock();
 
-            false
-        });
+            inner.children.retain(|_, child| {
+                let child = child.upgrade().unwrap();
+                let mut child_inner = child.process.inner.lock();
+                if child_inner.parent.as_ref().unwrap() == &self.init {
+                    return false;
+                }
 
-        let has_waiting = {
-            let mut init_waits = self.init.wait_list.wait_procs.lock();
-            let mut waits = process.wait_list.wait_procs.lock();
+                child_inner.parent = Some(self.init.clone());
+                init_inner.add_child(&child);
 
-            let mut done_some_work = false;
-            waits.retain(|item| {
-                if item.stopped().is_none() && !item.is_continue() {
-                    init_waits.push_back(*item);
-                    done_some_work = true;
-                }
                 false
             });
-
-            done_some_work
-        };
-
-        if has_waiting {
-            self.init.wait_list.cv_wait_procs.notify_all();
         }
 
-        {
-            let parent_wait_list = &inner.parent.as_ref().unwrap().wait_list;
-            let mut parent_waits = parent_wait_list.wait_procs.lock();
-            parent_waits.push_back(WaitObject {
-                pid: process.pid,
-                code: status,
-            });
-            parent_wait_list.cv_wait_procs.notify_all();
-        }
+        let mut init_notify = self.init.wait_list.notify_batch();
+        process
+            .wait_list
+            .drain_exited()
+            .into_iter()
+            .for_each(|item| init_notify.notify(item));
+        init_notify.finish();
+
+        inner.parent.as_ref().unwrap().wait_list.notify(WaitObject {
+            pid: process.pid,
+            code: status,
+        });
 
         preempt::enable();
     }
@@ -572,9 +576,9 @@ impl Process {
     pub fn new_cloned(other: &Arc<Self>) -> Arc<Self> {
         let other_inner = other.inner.lock();
 
-        let process = Arc::new(Self {
+        let process = Arc::new_cyclic(|weak| Self {
             pid: Self::alloc_pid(),
-            wait_list: WaitList::new(),
+            wait_list: WaitList::new(weak.clone()),
             mm_list: MMList::new_cloned(&other.mm_list),
             inner: Spin::new(ProcessInner {
                 pgroup: other_inner.pgroup.clone(),
@@ -598,7 +602,7 @@ impl Process {
             session.add_member(&pgroup);
             Self {
                 pid,
-                wait_list: WaitList::new(),
+                wait_list: WaitList::new(weak.clone()),
                 mm_list: MMList::new(),
                 inner: Spin::new(ProcessInner {
                     parent,
@@ -637,36 +641,21 @@ impl Process {
         trace_stop: bool,
         trace_continue: bool,
     ) -> KResult<Option<WaitObject>> {
-        let mut wait_list = self.wait_list.wait_procs.lock();
+        let mut waits = self.wait_list.entry(trace_stop, trace_continue);
         let wait_object = loop {
-            if let Some(idx) = wait_list
-                .iter()
-                .enumerate()
-                .filter(|(_, item)| {
-                    if item.stopped().is_some() {
-                        trace_stop
-                    } else if item.is_continue() {
-                        trace_continue
-                    } else {
-                        true
-                    }
-                })
-                .map(|(idx, _)| idx)
-                .next()
-            {
-                break wait_list.remove(idx).unwrap();
+            if let Some(object) = waits.get() {
+                break object;
             }
 
             if self.inner.lock().children.is_empty() {
                 return Err(ECHILD);
             }
+
             if no_block {
                 return Ok(None);
             }
-            self.wait_list.cv_wait_procs.wait(&mut wait_list);
-            if Thread::current().signal_list.has_pending_signal() {
-                return Err(EINTR);
-            }
+
+            waits.wait()?;
         };
 
         if wait_object.stopped().is_some() || wait_object.is_continue() {
@@ -869,7 +858,10 @@ impl Thread {
 
     pub fn do_stop(self: &Arc<Self>, signal: Signal) {
         if let Some(parent) = self.process.parent() {
-            parent.wait_list.notify_stop(self.process.pid, signal);
+            parent.wait_list.notify(WaitObject {
+                pid: self.process.pid,
+                code: WaitType::Stopped(signal),
+            });
         }
 
         preempt::disable();
@@ -882,7 +874,10 @@ impl Thread {
 
     pub fn do_continue(self: &Arc<Self>) {
         if let Some(parent) = self.process.parent() {
-            parent.wait_list.notify_continue(self.process.pid);
+            parent.wait_list.notify(WaitObject {
+                pid: self.process.pid,
+                code: WaitType::Continued,
+            });
         }
     }
 
@@ -990,29 +985,117 @@ impl Thread {
 unsafe impl Sync for Thread {}
 
 impl WaitList {
-    pub fn new() -> Self {
+    pub fn new(process: Weak<Process>) -> Self {
         Self {
             wait_procs: Spin::new(VecDeque::new()),
             cv_wait_procs: CondVar::new(),
+            process,
         }
     }
 
-    pub fn notify_continue(&self, pid: u32) {
+    pub fn notify(&self, wait: WaitObject) {
         let mut wait_procs = self.wait_procs.lock();
-        wait_procs.push_back(WaitObject {
-            pid,
-            code: WaitType::Continued,
-        });
+        wait_procs.push_back(wait);
         self.cv_wait_procs.notify_all();
+
+        self.process
+            .upgrade()
+            .expect("`process` must be valid if we are using `WaitList`")
+            .raise(Signal::SIGCHLD);
     }
 
-    pub fn notify_stop(&self, pid: u32, signal: Signal) {
-        let mut wait_procs = self.wait_procs.lock();
-        wait_procs.push_back(WaitObject {
-            pid,
-            code: WaitType::Stopped(signal),
-        });
-        self.cv_wait_procs.notify_all();
+    /// Notify some processes in batch. The process is waken up if we have really notified
+    /// some processes.
+    ///
+    /// # Lock
+    /// This function locks the `wait_procs` and returns a `NotifyBatch` that
+    /// will unlock it on dropped.
+    pub fn notify_batch(&self) -> NotifyBatch {
+        NotifyBatch {
+            wait_procs: self.wait_procs.lock(),
+            cv: &self.cv_wait_procs,
+            needs_notify: false,
+            process: &self.process,
+        }
+    }
+
+    pub fn drain_exited(&self) -> DrainExited {
+        DrainExited {
+            wait_procs: self.wait_procs.lock(),
+        }
+    }
+
+    pub fn entry(&self, want_stop: bool, want_continue: bool) -> Entry {
+        Entry {
+            wait_procs: self.wait_procs.lock(),
+            cv: &self.cv_wait_procs,
+            want_stop,
+            want_continue,
+        }
+    }
+}
+
+impl Entry<'_, '_> {
+    pub fn get(&mut self) -> Option<WaitObject> {
+        if let Some(idx) = self
+            .wait_procs
+            .iter()
+            .enumerate()
+            .filter(|(_, item)| {
+                if item.stopped().is_some() {
+                    self.want_stop
+                } else if item.is_continue() {
+                    self.want_continue
+                } else {
+                    true
+                }
+            })
+            .map(|(idx, _)| idx)
+            .next()
+        {
+            Some(self.wait_procs.remove(idx).unwrap())
+        } else {
+            None
+        }
+    }
+
+    pub fn wait(&mut self) -> KResult<()> {
+        self.cv.wait(&mut self.wait_procs);
+        if Thread::current().signal_list.has_pending_signal() {
+            return Err(EINTR);
+        }
+        Ok(())
+    }
+}
+
+impl DrainExited<'_> {
+    pub fn into_iter(&mut self) -> impl Iterator<Item = WaitObject> + '_ {
+        // We don't propagate stop and continue to the new parent.
+        self.wait_procs
+            .drain(..)
+            .filter(|item| item.stopped().is_none() && !item.is_continue())
+    }
+}
+
+impl NotifyBatch<'_, '_, '_> {
+    pub fn notify(&mut self, wait: WaitObject) {
+        self.wait_procs.push_back(wait);
+    }
+
+    /// Finish the batch and notify all if we have notified some processes.
+    pub fn finish(self) {}
+}
+
+impl Drop for NotifyBatch<'_, '_, '_> {
+    fn drop(&mut self) {
+        if self.needs_notify {
+            self.cv.notify_all();
+
+            self.process
+                .upgrade()
+                .expect("`process` must be valid if we are using `WaitList`")
+                .raise(Signal::SIGCHLD);
+        }
     }
 }