Remove trampoline code in handling user kernel

kernel/src/process/signal/mod.rs

@@ -17,7 +17,7 @@ use core::{mem, sync::atomic::Ordering};
 
 use align_ext::AlignExt;
 use c_types::{siginfo_t, ucontext_t};
-use constants::SIGKILL;
+use constants::SIGSEGV;
 pub use events::{SigEvents, SigEventsFilter};
 use ostd::{cpu::UserContext, user::UserContextApi};
 pub use pause::{with_signal_blocked, Pause};
@@ -119,7 +119,9 @@ pub fn handle_pending_signal(
                 signal.to_info(),
             ) {
                 debug!("Failed to handle user signal: {:?}", e);
-                do_exit_group(TermStatus::Killed(SIGKILL));
+                // If signal handling fails, the process should be terminated with SIGSEGV.
+                // Ref: <https://elixir.bootlin.com/linux/v6.13/source/kernel/signal.c#L3082>
+                do_exit_group(TermStatus::Killed(SIGSEGV));
             }
         }
         SigAction::Dfl => {
@@ -164,17 +166,17 @@ pub fn handle_user_signal(
     debug!("handler_addr = 0x{:x}", handler_addr);
     debug!("flags = {:?}", flags);
     debug!("restorer_addr = 0x{:x}", restorer_addr);
-    // FIXME: How to respect flags?
+
     if flags.contains_unsupported_flag() {
         warn!("Unsupported Signal flags: {:?}", flags);
     }
 
     if !flags.contains(SigActionFlags::SA_NODEFER) {
-        // add current signal to mask
+        // Add current signal to mask
         mask += sig_num;
     }
 
-    // block signals in sigmask when running signal handler
+    // Block signals in sigmask when running signal handler
     let old_mask = ctx.posix_thread.sig_mask().load(Ordering::Relaxed);
     ctx.posix_thread
         .sig_mask()
@@ -184,7 +186,7 @@ pub fn handle_user_signal(
     let mut stack_pointer = if let Some(sp) = use_alternate_signal_stack(ctx.thread_local) {
         sp as u64
     } else {
-        // just use user stack
+        // Just use user stack
         user_ctx.stack_pointer() as u64
     };
 
@@ -193,12 +195,12 @@ pub fn handle_user_signal(
 
     let user_space = ctx.user_space();
 
-    // 1. write siginfo_t
+    // 1. Write siginfo_t
     stack_pointer -= mem::size_of::<siginfo_t>() as u64;
     user_space.write_val(stack_pointer as _, &sig_info)?;
     let siginfo_addr = stack_pointer;
 
-    // 2. write ucontext_t.
+    // 2. Write ucontext_t.
     stack_pointer = alloc_aligned_in_user_stack(stack_pointer, mem::size_of::<ucontext_t>(), 16)?;
     let mut ucontext = ucontext_t {
         uc_sigmask: mask.into(),
@@ -223,30 +225,20 @@ pub fn handle_user_signal(
         .sig_context()
         .set(Some(ucontext_addr as Vaddr));
 
-    // 3. Set the address of the trampoline code.
+    // 3. Write the address of the restorer code.
     if flags.contains(SigActionFlags::SA_RESTORER) {
-        // If contains SA_RESTORER flag, trampoline code is provided by libc in restorer_addr.
-        // We just store restorer_addr on user stack to allow user code just to trampoline code.
+        // If the SA_RESTORER flag is present, the restorer code address is provided by the user.
         stack_pointer = write_u64_to_user_stack(stack_pointer, restorer_addr as u64)?;
-        trace!("After set restorer addr: user_rsp = 0x{:x}", stack_pointer);
-    } else {
-        // Otherwise we create a trampoline.
-        // FIXME: This may cause problems if we read old_context from rsp.
-        const TRAMPOLINE: &[u8] = &[
-            0xb8, 0x0f, 0x00, 0x00, 0x00, // mov eax, 15(syscall number of rt_sigreturn)
-            0x0f, 0x05, // syscall (call rt_sigreturn)
-            0x90, // nop (for alignment)
-        ];
-        stack_pointer -= TRAMPOLINE.len() as u64;
-        let trampoline_rip = stack_pointer;
-        user_space.write_bytes(stack_pointer as Vaddr, &mut VmReader::from(TRAMPOLINE))?;
-        stack_pointer = write_u64_to_user_stack(stack_pointer, trampoline_rip)?;
+        trace!(
+            "After writing restorer addr: user_rsp = 0x{:x}",
+            stack_pointer
+        );
     }
 
     // 4. Set correct register values
     user_ctx.set_instruction_pointer(handler_addr as _);
     user_ctx.set_stack_pointer(stack_pointer as usize);
-    // parameters of signal handler
+    // Parameters of signal handler
     if flags.contains(SigActionFlags::SA_SIGINFO) {
         user_ctx.set_arguments(sig_num, siginfo_addr as usize, ucontext_addr as usize);
     } else {

kernel/src/process/signal/sig_disposition.rs

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MPL-2.0
 
 use super::{constants::*, sig_action::SigAction, sig_num::SigNum};
+use crate::{prelude::*, process::signal::sig_action::SigActionFlags};
 
 #[derive(Copy, Clone)]
 pub struct SigDispositions {
@@ -26,9 +27,10 @@ impl SigDispositions {
         self.map[idx]
     }
 
-    pub fn set(&mut self, num: SigNum, sa: SigAction) -> SigAction {
+    pub fn set(&mut self, num: SigNum, sa: SigAction) -> Result<SigAction> {
+        check_sigaction(&sa)?;
         let idx = Self::num_to_idx(num);
-        core::mem::replace(&mut self.map[idx], sa)
+        Ok(core::mem::replace(&mut self.map[idx], sa))
     }
 
     pub fn set_default(&mut self, num: SigNum) {
@@ -52,3 +54,36 @@ impl SigDispositions {
         (num.as_u8() - MIN_STD_SIG_NUM) as usize
     }
 }
+
+fn check_sigaction(sig_action: &SigAction) -> Result<()> {
+    // Here we only check if the SA_RESTORER flag is set and restorer_addr is not 0.
+    // Note: Linux checks the SA_RESTORER flag when initializing the signal stack,
+    // whereas we have moved this check forward to prevent this action from being set.
+    // This may result in some differences from the behavior of Linux.
+
+    let SigAction::User {
+        flags,
+        restorer_addr,
+        ..
+    } = sig_action
+    else {
+        return Ok(());
+    };
+
+    if flags.contains(SigActionFlags::SA_RESTORER) && *restorer_addr != 0 {
+        return Ok(());
+    }
+
+    cfg_if::cfg_if! {
+        if #[cfg(target_arch = "x86_64")] {
+            // On x86-64, `SA_RESTORER` is mandatory and cannot be omitted.
+            // Ref: <https://elixir.bootlin.com/linux/v6.13/source/arch/x86/kernel/signal_64.c#L172>
+            return_errno_with_message!(Errno::EINVAL, "x86-64 should always use SA_RESTORER");
+        } else {
+            // FIXME: The support for user-provided signal handlers
+            // without `SA_RESTORER` is arch-dependent.
+            // Other archs may need to handle scenarios where `SA_RESTORER` is omitted.
+            return_errno_with_message!(Errno::EINVAL, "TODO: properly deal with SA_RESTORER");
+        }
+    }
+}