From 1d79d6b2df72f7153a67ece32b084247716209f8 Mon Sep 17 00:00:00 2001 From: Matthew Leach Date: Fri, 9 Jan 2026 06:46:47 +0000 Subject: [PATCH] process: refactor signal handling and fix action table sharing This commit refactors the signal handling subsystem to address a bug in process creation and to improve the separation of concerns between signal delivery and signal disposition. 1. Fix Action Table Sharing: Previously, the signal action table was wrapped in an `Arc`, causing forked child processes to inadvertently share signal dispositions with the parent. `SignalActionState` now contains a direct `SigActionSet` and implements Clone, ensuring that processes receive a private copy of the action table upon fork/clone. 2. Decouple Signal Selection from Execution: The logic for selecting a pending signal has been moved from the disposition state into a new `take_signal` method on `OwnedTask`. This decouples the "taking" of a signal (respecting masks and priorities) from the "actioning" of that signal. This is a prerequisite for implementing ptrace. 3. Various cleanup bits: - Renamed `SignalState` to `SignalActionState` to more accurately reflect that it manages signal dispositions. - Removed the `pending` signal set out of the action state and directly into the `ThreadGroup` and `OwnedTask`. - Renamed `set_pending` to `set_signal` for consistency. - Simplified the signal delivery loop in `dispatch_userspace_task` using the new `take_signal` API. --- src/process/exec.rs | 4 +- src/process/exit.rs | 5 +- src/process/owned.rs | 15 +- src/process/thread_group.rs | 4 +- src/process/thread_group/builder.rs | 8 +- src/process/thread_group/signal.rs | 102 ++++--------- src/process/thread_group/signal/kill.rs | 21 +-- src/process/thread_group/signal/sigaction.rs | 7 +- src/sched/uspc_ret.rs | 146 ++++++++++--------- 9 files changed, 144 insertions(+), 168 deletions(-) diff --git a/src/process/exec.rs b/src/process/exec.rs index 67299f1..f120cb7 100644 --- a/src/process/exec.rs +++ b/src/process/exec.rs @@ -8,7 +8,7 @@ use crate::{ page::ClaimedPage, uaccess::{copy_from_user, cstr::UserCStr}, }, - process::{ctx::Context, thread_group::signal::SignalState}, + process::{ctx::Context, thread_group::signal::SignalActionState}, sched::current::current_task, }; use alloc::{string::String, vec}; @@ -191,7 +191,7 @@ pub async fn kernel_exec( current_task.ctx = Context::from_user_ctx(user_ctx); *current_task.vm.lock_save_irq() = vm; - *current_task.process.signals.lock_save_irq() = SignalState::new_default(); + *current_task.process.signals.lock_save_irq() = SignalActionState::new_default(); Ok(()) } diff --git a/src/process/exit.rs b/src/process/exit.rs index a207664..d8ca768 100644 --- a/src/process/exit.rs +++ b/src/process/exit.rs @@ -80,7 +80,10 @@ pub fn do_exit_group(exit_code: ChildState) { parent.child_notifiers.child_update(process.tgid, exit_code); - parent.signals.lock_save_irq().set_pending(SigId::SIGCHLD); + parent + .pending_signals + .lock_save_irq() + .set_signal(SigId::SIGCHLD); // 5. This thread is now finished. *task.state.lock_save_irq() = TaskState::Finished; diff --git a/src/process/owned.rs b/src/process/owned.rs index e3782d0..db05b4f 100644 --- a/src/process/owned.rs +++ b/src/process/owned.rs @@ -8,7 +8,7 @@ use super::{ thread_group::{ Tgid, builder::ThreadGroupBuilder, - signal::{SigId, SigSet, SignalState}, + signal::{SigId, SigSet, SignalActionState}, }, threading::RobustListHead, }; @@ -62,7 +62,7 @@ impl OwnedTask { let thread_group_builder = ThreadGroupBuilder::new(Tgid::idle()) .with_priority(i8::MIN) - .with_sigstate(Arc::new(SpinLock::new(SignalState::new_ignore()))); + .with_sigstate(Arc::new(SpinLock::new(SignalActionState::new_ignore()))); let task = Task { tid: Tid::idle_for_cpu(), @@ -130,4 +130,15 @@ impl OwnedTask { pub fn raise_task_signal(&mut self, signal: SigId) { self.pending_signals.insert(signal.into()); } + + /// Take a pending signal from this task's pending signal queue, or the + /// process's pending signal queue, while repsecting the signal mask. + pub fn take_signal(&mut self) -> Option { + self.pending_signals.take_signal(self.sig_mask).or_else(|| { + self.process + .pending_signals + .lock_save_irq() + .take_signal(self.sig_mask) + }) + } } diff --git a/src/process/thread_group.rs b/src/process/thread_group.rs index cf2f93c..77f8444 100644 --- a/src/process/thread_group.rs +++ b/src/process/thread_group.rs @@ -12,7 +12,7 @@ use core::{ }; use pid::PidT; use rsrc_lim::ResourceLimits; -use signal::{SigSet, SignalState}; +use signal::{SigSet, SignalActionState}; use wait::ChildNotifiers; pub mod builder; @@ -96,7 +96,7 @@ pub struct ThreadGroup { pub parent: SpinLock>>, pub children: SpinLock>>, pub tasks: SpinLock>>, - pub signals: Arc>, + pub signals: Arc>, pub rsrc_lim: Arc>, pub pending_signals: SpinLock, pub priority: SpinLock, diff --git a/src/process/thread_group/builder.rs b/src/process/thread_group/builder.rs index a432921..49b3d41 100644 --- a/src/process/thread_group/builder.rs +++ b/src/process/thread_group/builder.rs @@ -7,7 +7,7 @@ use crate::sync::SpinLock; use super::{ Pgid, ProcessState, Sid, TG_LIST, Tgid, ThreadGroup, rsrc_lim::ResourceLimits, - signal::{SigSet, SignalState}, + signal::{SigSet, SignalActionState}, wait::ChildNotifiers, }; @@ -17,7 +17,7 @@ pub struct ThreadGroupBuilder { parent: Option>, umask: Option, pri: Option, - sigstate: Option>>, + sigstate: Option>>, rsrc_lim: Option>>, } @@ -41,7 +41,7 @@ impl ThreadGroupBuilder { } /// Sets the signal state of the thread group. - pub fn with_sigstate(mut self, sigstate: Arc>) -> Self { + pub fn with_sigstate(mut self, sigstate: Arc>) -> Self { self.sigstate = Some(sigstate); self } @@ -69,7 +69,7 @@ impl ThreadGroupBuilder { children: SpinLock::new(BTreeMap::new()), signals: self .sigstate - .unwrap_or_else(|| Arc::new(SpinLock::new(SignalState::new_default()))), + .unwrap_or_else(|| Arc::new(SpinLock::new(SignalActionState::new_default()))), rsrc_lim: self .rsrc_lim .unwrap_or_else(|| Arc::new(SpinLock::new(ResourceLimits::default()))), diff --git a/src/process/thread_group/signal.rs b/src/process/thread_group/signal.rs index 035b9b2..5aef4e2 100644 --- a/src/process/thread_group/signal.rs +++ b/src/process/thread_group/signal.rs @@ -8,9 +8,8 @@ use core::{ use bitflags::bitflags; use ksigaction::{KSignalAction, UserspaceSigAction}; use libkernel::memory::{address::UA, region::UserMemoryRegion}; -use ringbuf::Arc; -use crate::{memory::uaccess::UserCopyable, sync::SpinLock}; +use crate::memory::uaccess::UserCopyable; pub mod kill; pub mod ksigaction; @@ -81,6 +80,23 @@ impl From for SigId { } } +impl SigSet { + /// Set the signal with id `signal` to true in the set. + pub fn set_signal(&mut self, signal: SigId) { + *self = self.union(signal.into()); + } + + /// Remove a set signal from the set, setting it to false, while respecting + /// `mask`. Returns the ID of the removed signal. + pub fn take_signal(&mut self, mask: SigSet) -> Option { + let signal = self.difference(mask).iter().next()?; + + self.remove(signal); + + Some(signal.into()) + } +} + #[repr(u32)] #[derive(Clone, Copy, PartialEq, Eq, Debug)] #[allow(clippy::upper_case_acronyms)] @@ -196,91 +212,33 @@ impl AltSigStack { } } -pub struct SignalState { - action: Arc>, - pending: SigSet, +#[derive(Clone)] +pub struct SignalActionState { + action: SigActionSet, pub alt_stack: Option, } -impl Clone for SignalState { - fn clone(&self) -> Self { - Self { - action: self.action.clone(), - pending: SigSet::empty(), - alt_stack: None, - } - } -} - -impl SignalState { +impl SignalActionState { pub fn new_ignore() -> Self { Self { - action: Arc::new(SpinLock::new(SigActionSet([SigActionState::Ignore; 64]))), - pending: SigSet::empty(), + action: SigActionSet([SigActionState::Ignore; 64]), alt_stack: None, } } pub fn new_default() -> Self { Self { - action: Arc::new(SpinLock::new(SigActionSet([SigActionState::Default; 64]))), - pending: SigSet::empty(), + action: SigActionSet([SigActionState::Default; 64]), alt_stack: None, } } - pub fn clone_sharing_action_table(&self) -> Self { - Self { - action: self.action.clone(), - pending: SigSet::empty(), - alt_stack: None, - } - } - - pub fn clone_copying_action_table(&self) -> Self { - Self { - action: Arc::new(SpinLock::new(self.action.lock_save_irq().clone())), - pending: SigSet::empty(), - alt_stack: None, - } - } - - pub fn set_pending(&mut self, signal: SigId) { - self.pending.insert(signal.into()); - } - - pub fn action_signal( - &mut self, - mask: SigSet, - task_pending: &mut SigSet, - ) -> Option<(SigId, KSignalAction)> { - loop { - let signal = self - .pending - .union(*task_pending) - .difference(mask) - .iter() - .next()?; - - // Consume the signal we are about to action. - self.pending.remove(signal); - task_pending.remove(signal); - - let id: SigId = signal.into(); - - match self.action.lock_save_irq()[id] { - SigActionState::Ignore => continue, // look for another signal, - SigActionState::Default => { - let action = KSignalAction::default_action(id); - - if let Some(action) = action { - return Some((id, action)); - } - // Signal is ignored by default. Look for another signal. - } - SigActionState::Action(userspace_sig_action) => { - return Some((id, KSignalAction::Userspace(id, userspace_sig_action))); - } + pub fn action_signal(&self, id: SigId) -> Option { + match self.action[id] { + SigActionState::Ignore => None, // look for another signal, + SigActionState::Default => KSignalAction::default_action(id), + SigActionState::Action(userspace_sig_action) => { + Some(KSignalAction::Userspace(id, userspace_sig_action)) } } } diff --git a/src/process/thread_group/signal/kill.rs b/src/process/thread_group/signal/kill.rs index 4d1d9e7..d2de055 100644 --- a/src/process/thread_group/signal/kill.rs +++ b/src/process/thread_group/signal/kill.rs @@ -18,16 +18,16 @@ pub fn sys_kill(pid: PidT, signal: UserSigId) -> Result { if pid == current_task.process.tgid.value() as PidT { current_task .process - .signals + .pending_signals .lock_save_irq() - .set_pending(signal); + .set_signal(signal); return Ok(0); } match pid { p if p > 0 => { let target_tg = ThreadGroup::get(Tgid(p as _)).ok_or(KernelError::NoProcess)?; - target_tg.signals.lock_save_irq().set_pending(signal); + target_tg.pending_signals.lock_save_irq().set_signal(signal); } 0 => { @@ -41,7 +41,7 @@ pub fn sys_kill(pid: PidT, signal: UserSigId) -> Result { if let Some(tg) = tg_weak.upgrade() && *tg.pgid.lock_save_irq() == our_pgid { - tg.signals.lock_save_irq().set_pending(signal); + tg.pending_signals.lock_save_irq().set_signal(signal); } } } @@ -55,7 +55,7 @@ pub fn sys_kill(pid: PidT, signal: UserSigId) -> Result { if let Some(tg) = tg_weak.upgrade() && *tg.pgid.lock_save_irq() == target_pgid { - tg.signals.lock_save_irq().set_pending(signal); + tg.pending_signals.lock_save_irq().set_signal(signal); } } } @@ -76,9 +76,9 @@ pub fn sys_tkill(tid: PidT, signal: UserSigId) -> Result { if current_task.tid == target_tid { current_task .process - .signals + .pending_signals .lock_save_irq() - .set_pending(signal); + .set_signal(signal); } else { let task = current_task .process @@ -88,7 +88,10 @@ pub fn sys_tkill(tid: PidT, signal: UserSigId) -> Result { .and_then(|t| t.upgrade()) .ok_or(KernelError::NoProcess)?; - task.process.signals.lock_save_irq().set_pending(signal); + task.process + .pending_signals + .lock_save_irq() + .set_signal(signal); } Ok(0) @@ -99,7 +102,7 @@ pub fn send_signal_to_pg(pgid: Pgid, signal: SigId) { if let Some(tg) = tg_weak.upgrade() && *tg.pgid.lock_save_irq() == pgid { - tg.signals.lock_save_irq().set_pending(signal); + tg.pending_signals.lock_save_irq().set_signal(signal); } } } diff --git a/src/process/thread_group/signal/sigaction.rs b/src/process/thread_group/signal/sigaction.rs index fa2148b..1dbeba2 100644 --- a/src/process/thread_group/signal/sigaction.rs +++ b/src/process/thread_group/signal/sigaction.rs @@ -112,12 +112,11 @@ pub async fn sys_rt_sigaction( let old_action = { let task = current_task(); - let sigstate = task.process.signals.lock_save_irq(); - let mut action_table = sigstate.action.lock_save_irq(); - let old_action = action_table[sig]; + let mut sigstate = task.process.signals.lock_save_irq(); + let old_action = sigstate.action[sig]; if let Some(new_action) = new_act { - action_table[sig] = new_action.into(); + sigstate.action[sig] = new_action.into(); } old_action diff --git a/src/sched/uspc_ret.rs b/src/sched/uspc_ret.rs index 72d37fe..d998004 100644 --- a/src/sched/uspc_ret.rs +++ b/src/sched/uspc_ret.rs @@ -74,7 +74,7 @@ enum State { pub fn dispatch_userspace_task(ctx: *mut UserCtx) { let mut state = State::PickNewTask; - loop { + 'dispatch: loop { match state { State::PickNewTask => { // Pick a new task, potentially context switching to a new task. @@ -228,91 +228,93 @@ pub fn dispatch_userspace_task(ctx: *mut UserCtx) { continue; } - // See if there are any signals we need to action. - if let Some((id, action)) = { - let task = current_task(); - let mut pending_task_sigs = task.pending_signals; + { + let mut task = current_task(); - let mask = task.sig_mask; - task.process - .signals - .lock_save_irq() - .action_signal(mask, &mut pending_task_sigs) - } { - match action { - KSignalAction::Term | KSignalAction::Core => { - // Terminate the process, and find a new task. - kernel_exit_with_signal(id, false); + while let Some(signal) = task.take_signal() { + let sigaction = task.process.signals.lock_save_irq().action_signal(signal); - state = State::PickNewTask; - continue; - } - KSignalAction::Stop => { - let task = current_task(); + match sigaction { + // Signal ignored, look for another. + None => continue, + Some(KSignalAction::Term | KSignalAction::Core) => { + // Terminate the process, and find a new task. + kernel_exit_with_signal(signal, false); - // Default action: stop (suspend) the entire process. - let process = &task.process; - - // Notify the parent that we have stopped (SIGCHLD). - if let Some(parent) = process - .parent - .lock_save_irq() - .as_ref() - .and_then(|p| p.upgrade()) - { - parent - .child_notifiers - .child_update(process.tgid, ChildState::Stop { signal: id }); - parent.signals.lock_save_irq().set_pending(SigId::SIGCHLD); + state = State::PickNewTask; + continue 'dispatch; } + Some(KSignalAction::Stop) => { + // Default action: stop (suspend) the entire process. + let process = &task.process; - for thr_weak in process.tasks.lock_save_irq().values() { - if let Some(thr) = thr_weak.upgrade() { - *thr.state.lock_save_irq() = TaskState::Stopped; + // Notify the parent that we have stopped (SIGCHLD). + if let Some(parent) = process + .parent + .lock_save_irq() + .as_ref() + .and_then(|p| p.upgrade()) + { + parent + .child_notifiers + .child_update(process.tgid, ChildState::Stop { signal }); + + parent + .pending_signals + .lock_save_irq() + .set_signal(SigId::SIGCHLD); } - } - state = State::PickNewTask; - continue; - } - KSignalAction::Continue => { - let task = current_task(); - let process = &task.process; - - // Wake up all sleeping threads in the process. - for thr_weak in process.tasks.lock_save_irq().values() { - if let Some(thr) = thr_weak.upgrade() { - let mut st = thr.state.lock_save_irq(); - if *st == TaskState::Sleeping { - *st = TaskState::Runnable; + for thr_weak in process.tasks.lock_save_irq().values() { + if let Some(thr) = thr_weak.upgrade() { + *thr.state.lock_save_irq() = TaskState::Stopped; } } + + state = State::PickNewTask; + continue 'dispatch; } + Some(KSignalAction::Continue) => { + let process = &task.process; - // Notify the parent that we have continued (SIGCHLD). - if let Some(parent) = process - .parent - .lock_save_irq() - .as_ref() - .and_then(|p| p.upgrade()) - { - parent - .child_notifiers - .child_update(process.tgid, ChildState::Continue); - parent.signals.lock_save_irq().set_pending(SigId::SIGCHLD); + // Wake up all sleeping threads in the process. + for thr_weak in process.tasks.lock_save_irq().values() { + if let Some(thr) = thr_weak.upgrade() { + let mut st = thr.state.lock_save_irq(); + if *st == TaskState::Sleeping { + *st = TaskState::Runnable; + } + } + } + + // Notify the parent that we have continued (SIGCHLD). + if let Some(parent) = process + .parent + .lock_save_irq() + .as_ref() + .and_then(|p| p.upgrade()) + { + parent + .child_notifiers + .child_update(process.tgid, ChildState::Continue); + parent + .pending_signals + .lock_save_irq() + .set_signal(SigId::SIGCHLD); + } + + // Re-process kernel work for this task (there may be more to do). + state = State::ProcessKernelWork; + continue 'dispatch; } + Some(KSignalAction::Userspace(id, action)) => { + let fut = ArchImpl::do_signal(id, action); - // Re-process kernel work for this task (there may be more to do). - state = State::ProcessKernelWork; - continue; - } - KSignalAction::Userspace(id, action) => { - let fut = ArchImpl::do_signal(id, action); + task.ctx.put_signal_work(Box::pin(fut)); - current_task().ctx.put_signal_work(Box::pin(fut)); - - state = State::ProcessKernelWork; - continue; + state = State::ProcessKernelWork; + continue 'dispatch; + } } } }