vasilito/RedBear-OS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix the SIGKILL fd leak bug.

Browse Source
This commit is contained in:
4lDO2
2024-03-07 20:01:44 +01:00
parent 8e19da338b
commit 4a3dc2dadd
9 changed files with 58 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/context/context.rs
+20 -11
View File
@@ -1,4 +1,5 @@
use alloc::{borrow::Cow, sync::Arc, vec::Vec};
use syscall::{SIGKILL, SIGSTOP};
use core::{cmp::Ordering, mem, num::NonZeroUsize};
use spin::RwLock;
@@ -23,7 +24,7 @@ use crate::syscall::{
/// Unique identifier for a context (i.e. `pid`).
use ::core::sync::atomic::AtomicUsize;
use super::memory::{GrantFileRef, AddrSpaceWrapper};
use super::{memory::{GrantFileRef, AddrSpaceWrapper}, empty_cr3};
int_like!(ContextId, AtomicContextId, usize, AtomicUsize);
/// The status of a context - used for scheduling
@@ -396,13 +397,14 @@ impl Context {
}
pub fn set_addr_space(
&mut self,
addr_space: Arc<AddrSpaceWrapper>,
addr_space: Option<Arc<AddrSpaceWrapper>>,
) -> Option<Arc<AddrSpaceWrapper>> {
if let Some(ref old) = self.addr_space && Arc::ptr_eq(old, &addr_space) {
return Some(addr_space);
if let (Some(ref old), Some(ref new)) = (&self.addr_space, &addr_space) && Arc::ptr_eq(old, new) {
return addr_space;
};
if self.id == super::context_id() {
// TODO: Share more code with context::arch::switch_to.
let this_percpu = PercpuBlock::current();
if let Some(ref prev_addrsp) = self.addr_space {
@@ -410,19 +412,25 @@ impl Context {
prev_addrsp.acquire_read().used_by.atomic_clear(this_percpu.cpu_id);
}
*this_percpu.current_addrsp.borrow_mut() = Some(Arc::clone(&addr_space));
let _old_addrsp = core::mem::replace(&mut *this_percpu.current_addrsp.borrow_mut(), addr_space.clone());
let new_addrsp = addr_space.acquire_read();
new_addrsp.used_by.atomic_set(this_percpu.cpu_id);
if let Some(ref new) = addr_space {
let new_addrsp = new.acquire_read();
new_addrsp.used_by.atomic_set(this_percpu.cpu_id);
unsafe {
new_addrsp.table.utable.make_current();
unsafe {
new_addrsp.table.utable.make_current();
}
} else {
unsafe {
crate::paging::RmmA::set_table(rmm::TableKind::User, empty_cr3());
}
}
} else {
assert!(!self.running);
}
self.addr_space.replace(addr_space)
core::mem::replace(&mut self.addr_space, addr_space)
}
pub fn empty_actions() -> Arc<RwLock<Vec<(SigAction, usize)>>> {
Arc::new(RwLock::new(vec![(
@@ -470,7 +478,8 @@ impl Context {
impl SignalState {
pub fn deliverable(&self) -> u64 {
self.pending & !self.procmask
const CANT_BLOCK: u64 = (1 << (SIGKILL - 1)) | (1 << (SIGSTOP - 1));
self.pending & (CANT_BLOCK | !self.procmask)
}
}
src/context/list.rs
+1 -1
View File
@@ -113,7 +113,7 @@ impl ContextList {
let context_lock = self.new_context()?;
{
let mut context = context_lock.write();
let _ = context.set_addr_space(AddrSpaceWrapper::new()?);
let _ = context.set_addr_space(Some(AddrSpaceWrapper::new()?));
let mut stack_top = unsafe { stack.as_mut_ptr().add(KSTACK_SIZE) };
src/context/signal.rs
+14 -2
View File
@@ -42,7 +42,7 @@ pub fn kmain_signal_handler() {
// TODO: Move everything but SIGKILL to userspace. SIGCONT and SIGSTOP do not necessarily need to
// be done from this current context.
pub fn signal_handler(eintr: bool) {
pub fn signal_handler() {
let (action, sig) = {
// FIXME: Can any low-level state become corrupt if a panic occurs here?
let context_lock = context::current().expect("context::signal_handler not inside of context");
@@ -53,7 +53,18 @@ pub fn signal_handler(eintr: bool) {
if context.sig.deliverable() == 0 {
return;
}
let selected = context.sig.deliverable().trailing_zeros() as usize + 1;
let bits = context.sig.deliverable();
// Always prioritize SIGKILL and SIGSTOP, so processes can't simply keep themselves alive
// by killing themselves.
let selected = if bits & (1 << (SIGKILL - 1)) != 0 {
SIGKILL
} else if bits & (1 << (SIGSTOP - 1)) != 0 {
SIGSTOP
} else {
bits.trailing_zeros() as usize + 1
};
context.sig.pending &= !(1 << (selected - 1));
let actions = context.actions.read();
@@ -111,6 +122,7 @@ pub fn signal_handler(eintr: bool) {
}
}
}
// FIXME: SIGSTOP shouldn't be overridable
SIGSTOP | SIGTSTP | SIGTTIN | SIGTTOU => {
// println!("Stop {}", sig);
src/context/switch.rs
+2 -2
View File
@@ -80,7 +80,7 @@ pub fn tick() {
if new_ticks >= 3 {
match switch() {
SwitchResult::Switched { signal: true } => {
crate::context::signal::signal_handler(false);
crate::context::signal::signal_handler();
},
_ => (),
}
@@ -94,8 +94,8 @@ pub unsafe extern "C" fn switch_finish_hook() {
// TODO: unreachable_unchecked()?
crate::arch::stop::emergency_reset();
}
crate::percpu::switch_arch_hook();
arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst);
crate::percpu::switch_arch_hook();
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
src/main.rs
+1 -1
View File
@@ -269,7 +269,7 @@ pub fn ksignal(signal: usize) {
context.sig.pending |= 1 << (signal - 1);
}
}
crate::context::signal::signal_handler(false);
crate::context::signal::signal_handler();
}
// TODO: Use this macro on aarch64 too.
src/scheme/proc.rs
+1 -1
View File
@@ -637,7 +637,7 @@ impl<const FULL: bool> KernelScheme for ProcScheme<FULL> {
regs.set_instr_pointer(new_ip);
regs.set_stack_pointer(new_sp);
Ok(context.set_addr_space(new))
Ok(context.set_addr_space(Some(new)))
})?;
let _ = ptrace::send_event(crate::syscall::ptrace_event!(
PTRACE_EVENT_ADDRSPACE_SWITCH,
src/scheme/user.rs
+17 -8
View File
@@ -15,7 +15,7 @@ use spin::{Mutex, RwLock};
use spinning_top::RwSpinlock;
use syscall::{
FobtainFdFlags, MunmapFlags, SendFdFlags, MAP_FIXED_NOREPLACE, SKMSG_FOBTAINFD,
SKMSG_FRETURNFD, SKMSG_PROVIDE_MMAP, KSMSG_CANCEL,
SKMSG_FRETURNFD, SKMSG_PROVIDE_MMAP, KSMSG_CANCEL, SIGKILL,
};
use crate::{
@@ -190,6 +190,14 @@ impl UserInner {
loop {
context::switch();
let eintr_if_sigkill = || if context::current()?.read().sig.deliverable() & (1 << (SIGKILL - 1)) != 0 {
// EINTR directly if SIGKILL was found without waiting for scheme. Data loss
// doesn't matter.
Err(Error::new(EINTR))
} else {
Ok(())
};
let mut states = self.states.lock();
match states.entry(id) {
// invalid state
@@ -198,12 +206,17 @@ impl UserInner {
// signal wakeup while awaiting cancelation
old_state @ State::Waiting { canceling: true, .. } => {
*o.get_mut() = old_state;
continue;
drop(states);
eintr_if_sigkill()?;
context::current()?.write().block("UserInner::call");
}
// spurious wakeup
State::Waiting { canceling: false, fd, context } => {
*o.get_mut() = State::Waiting { canceling: true, fd, context };
drop(states);
eintr_if_sigkill()?;
// TODO: Is this too dangerous when the states lock is held?
self.todo.send(Packet {
id: 0,
@@ -213,7 +226,7 @@ impl UserInner {
..packet
});
event::trigger(self.root_id, self.handle_id, EVENT_READ);
continue;
context::current()?.write().block("UserInner::call");
}
// invalid state
@@ -585,10 +598,6 @@ impl UserInner {
Ok(())
}
fn handle_packet(&self, packet: &Packet) -> Result<()> {
if packet.a == KSMSG_CANCEL {
log::warn!("Context {} did KSMSG_CANCEL", context::current().unwrap().read().name);
}
if packet.id == 0 {
// TODO: Simplify logic by using SKMSG with packet.id being ignored?
match packet.a {
@@ -597,7 +606,7 @@ impl UserInner {
packet.b,
EventFlags::from_bits_truncate(packet.c),
),
_ => log::warn!("Unknown scheme -> kernel message {}", packet.a),
_ => log::warn!("Unknown scheme -> kernel message {} from {}", packet.a, context::current().unwrap().read().name),
}
} else if Error::demux(packet.a) == Err(Error::new(ESKMSG)) {
// The reason why the new ESKMSG mechanism was introduced, is that passing packet IDs
src/syscall/mod.rs
+1 -1
View File
@@ -328,7 +328,7 @@ pub fn syscall(
// 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(true);
crate::context::signal::signal_handler();
}
}
}
src/syscall/process.rs
+1 -2
View File
@@ -43,8 +43,7 @@ pub fn exit(status: usize) -> ! {
let mut context = context_lock.write();
close_files = Arc::try_unwrap(mem::take(&mut context.files))
.map_or_else(|_| Vec::new(), RwLock::into_inner);
addrspace_opt =
mem::take(&mut context.addr_space).and_then(|a| Arc::try_unwrap(a).ok());
addrspace_opt = context.set_addr_space(None).and_then(|a| Arc::try_unwrap(a).ok());
drop(context.syscall_head.take());
drop(context.syscall_tail.take());
context.id