Jika/redox-kernel
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'futex-fix' into 'master'

Browse Source 
Use physical addresses internally in futex, and fix a context switching data race

See merge request redox-os/kernel!166
This commit is contained in:
Jeremy Soller
2021-02-13 17:52:09 +00:00
parent 5e10feeaeb 6f3fc3a4f4
commit 76d8c1074c
3 changed files with 119 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/context/list.rs
View File

@@ -47,6 +47,9 @@ impl ContextList {
pub fn iter(&self) -> ::alloc::collections::btree_map::Iter<ContextId, Arc<RwLock<Context>>> {
self.map.iter()
}
pub fn range(&self, range: impl core::ops::RangeBounds<ContextId>) -> ::alloc::collections::btree_map::Range<'_, ContextId, Arc<RwLock<Context>>> {
self.map.range(range)
}
/// Create a new context.
pub fn new_context(&mut self) -> Result<&Arc<RwLock<Context>>> {

138
src/context/switch.rs
View File

@@ -1,5 +1,8 @@
use core::ops::Bound;
use core::sync::atomic::Ordering;
use alloc::sync::Arc;
use crate::context::signal::signal_handler;
use crate::context::{arch, contexts, Context, Status, CONTEXT_ID};
use crate::gdt;
@@ -72,8 +75,6 @@ unsafe fn runnable(context: &Context, cpu_id: usize) -> bool {
///
/// Do not call this while holding locks!
pub unsafe fn switch() -> bool {
use core::ops::DerefMut;
//set PIT Interrupt counter to 0, giving each process same amount of PIT ticks
let ticks = PIT_TICKS.swap(0, Ordering::SeqCst);
@@ -84,93 +85,108 @@ pub unsafe fn switch() -> bool {
let cpu_id = crate::cpu_id();
let from_ptr;
let mut to_ptr = 0 as *mut Context;
let from_context_lock;
let mut from_context_guard;
let mut to_context_lock: Option<(Arc<spin::RwLock<Context>>, *mut Context)> = None;
let mut to_sig = None;
{
let contexts = contexts();
{
let context_lock = contexts
from_context_lock = Arc::clone(contexts
.current()
.expect("context::switch: not inside of context");
let mut context = context_lock.write();
context.ticks += ticks as u64 + 1; // Always round ticks up
from_ptr = context.deref_mut() as *mut Context;
}
macro_rules! to {
($context:expr) => {{
let context: &mut Context = $context;
if runnable(context, cpu_id) {
to_ptr = context as *mut Context;
if context.ksig.is_none() {
to_sig = context.pending.pop_front();
}
true
} else {
false
}
}};
};
for (_pid, context_lock) in contexts.iter() {
let mut context = context_lock.write();
update(&mut context, cpu_id);
.expect("context::switch: not inside of context"));
from_context_guard = from_context_lock.write();
from_context_guard.ticks += ticks as u64 + 1; // Always round ticks up
}
for (pid, context_lock) in contexts.iter() {
if *pid > (*from_ptr).id {
let mut context = context_lock.write();
if to!(&mut context) {
break;
}
}
let mut context;
let context_ref = if *pid == from_context_guard.id {
&mut *from_context_guard
} else {
context = context_lock.write();
&mut *context
};
update(context_ref, cpu_id);
}
if to_ptr as usize == 0 {
for (pid, context_lock) in contexts.iter() {
if *pid < (*from_ptr).id {
let mut context = context_lock.write();
if to!(&mut context) {
break;
}
for (_pid, context_lock) in contexts
// Include all contexts with IDs greater than the current...
.range(
(Bound::Excluded(from_context_guard.id), Bound::Unbounded)
)
.chain(contexts
// ... and all contexts with IDs less than the current...
.range((Bound::Unbounded, Bound::Excluded(from_context_guard.id)))
)
// ... but not the current context, which is already locked
{
let context_lock = Arc::clone(context_lock);
let mut to_context_guard = context_lock.write();
if runnable(&*to_context_guard, cpu_id) {
if to_context_guard.ksig.is_none() {
to_sig = to_context_guard.pending.pop_front();
}
let ptr: *mut Context = &mut *to_context_guard;
core::mem::forget(to_context_guard);
to_context_lock = Some((context_lock, ptr));
break;
} else {
continue;
}
}
};
// Switch process states, TSS stack pointer, and store new context ID
if to_ptr as usize != 0 {
(*from_ptr).running = false;
(*to_ptr).running = true;
if let Some(ref stack) = (*to_ptr).kstack {
if let Some((to_context_lock, to_ptr)) = to_context_lock {
let to_context: &mut Context = &mut *to_ptr;
from_context_guard.running = false;
to_context.running = true;
if let Some(ref stack) = to_context.kstack {
gdt::set_tss_stack(stack.as_ptr() as usize + stack.len());
}
gdt::set_tcb((*to_ptr).id.into());
CONTEXT_ID.store((*to_ptr).id, Ordering::SeqCst);
}
gdt::set_tcb(to_context.id.into());
CONTEXT_ID.store(to_context.id, Ordering::SeqCst);
if to_ptr as usize == 0 {
// No target was found, unset global lock and return
arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst);
false
} else {
if let Some(sig) = to_sig {
// Signal was found, run signal handler
//TODO: Allow nested signals
assert!((*to_ptr).ksig.is_none());
assert!(to_context.ksig.is_none());
let arch = (*to_ptr).arch.clone();
let kfx = (*to_ptr).kfx.clone();
let kstack = (*to_ptr).kstack.clone();
(*to_ptr).ksig = Some((arch, kfx, kstack, sig));
(*to_ptr).arch.signal_stack(signal_handler, sig);
let arch = to_context.arch.clone();
let kfx = to_context.kfx.clone();
let kstack = to_context.kstack.clone();
to_context.ksig = Some((arch, kfx, kstack, sig));
to_context.arch.signal_stack(signal_handler, sig);
}
let from_ptr: *mut Context = &mut *from_context_guard;
let to_ptr: *mut Context = &mut *to_context;
// FIXME: Ensure that this critical section is somehow still protected by the lock, and not
// just for other processes' context switching, but for other operations which do not
// require CONTEXT_SWITCH_LOCK.
//
// What I suggest, is that we wrap the Context struct (typically stored as
// `Arc<RwLock<Context>>`), into a wrapper with interior locking for the inner context type
// (which could be something like `RwLock<ContextInner>`). The wrapper would also contain
// a field of type `UnsafeCell<ContextArch>`, which would be accessible if and only if the
// `running` field is set to false, making that field function as a lock.
drop(from_context_guard);
drop(from_context_lock);
to_context_lock.force_write_unlock();
drop(to_context_lock);
(*from_ptr).arch.switch_to(&mut (*to_ptr).arch);
true
} else {
// No target was found, unset global lock and return
arch::CONTEXT_SWITCH_LOCK.store(false, Ordering::SeqCst);
false
}
}

49
src/syscall/futex.rs
View File

@@ -9,12 +9,19 @@ use spin::{Once, RwLock, RwLockReadGuard, RwLockWriteGuard};
use crate::context::{self, Context};
use crate::time;
use crate::memory::PhysicalAddress;
use crate::paging::{ActivePageTable, VirtualAddress};
use crate::syscall::data::TimeSpec;
use crate::syscall::error::{Error, Result, ESRCH, EAGAIN, EINVAL};
use crate::syscall::error::{Error, Result, ESRCH, EAGAIN, EFAULT, EINVAL};
use crate::syscall::flag::{FUTEX_WAIT, FUTEX_WAKE, FUTEX_REQUEUE};
use crate::syscall::validate::{validate_slice, validate_slice_mut};
type FutexList = VecDeque<(usize, Arc<RwLock<Context>>)>;
type FutexList = VecDeque<FutexEntry>;
pub struct FutexEntry {
target_physaddr: PhysicalAddress,
context_lock: Arc<RwLock<Context>>,
}
/// Fast userspace mutex list
static FUTEXES: Once<RwLock<FutexList>> = Once::new();
@@ -34,7 +41,17 @@ pub fn futexes_mut() -> RwLockWriteGuard<'static, FutexList> {
FUTEXES.call_once(init_futexes).write()
}
// FIXME: Don't take a mutable reference to the addr, since rustc can make assumptions that the
// pointee cannot be changed by another thread, which could make atomic ops useless.
pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32) -> Result<usize> {
let target_physaddr = unsafe {
let mut active_table = ActivePageTable::new();
let virtual_address = VirtualAddress::new(addr as *mut i32 as usize);
// FIXME: Already validated in syscall/mod.rs
active_table.translate(virtual_address).ok_or(Error::new(EFAULT))?
};
match op {
FUTEX_WAIT => {
let timeout_opt = if val2 != 0 {
@@ -69,7 +86,10 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32)
context.block("futex");
}
futexes.push_back((addr as *mut i32 as usize, context_lock));
futexes.push_back(FutexEntry {
target_physaddr,
context_lock,
});
}
unsafe { context::switch(); }
@@ -97,9 +117,10 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32)
let mut i = 0;
while i < futexes.len() && (woken as i32) < val {
if futexes[i].0 == addr as *mut i32 as usize {
if futexes[i].target_physaddr == target_physaddr {
if let Some(futex) = futexes.swap_remove_back(i) {
futex.1.write().unblock();
let mut context_guard = futex.context_lock.write();
context_guard.unblock();
woken += 1;
}
} else {
@@ -111,7 +132,15 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32)
Ok(woken)
},
FUTEX_REQUEUE => {
let addr2_safe = validate_slice_mut(addr2, 1).map(|addr2_safe| &mut addr2_safe[0])?;
let addr2_physaddr = unsafe {
let mut active_table = ActivePageTable::new();
let addr2_safe = validate_slice_mut(addr2, 1).map(|addr2_safe| &mut addr2_safe[0])?;
let addr2_virt = VirtualAddress::new(addr2_safe as *mut i32 as usize);
// FIXME: Already validated.
active_table.translate(addr2_virt).ok_or(Error::new(EFAULT))?
};
let mut woken = 0;
let mut requeued = 0;
@@ -121,9 +150,9 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32)
let mut i = 0;
while i < futexes.len() && (woken as i32) < val {
if futexes[i].0 == addr as *mut i32 as usize {
if futexes[i].target_physaddr == target_physaddr {
if let Some(futex) = futexes.swap_remove_back(i) {
futex.1.write().unblock();
futex.context_lock.write().unblock();
woken += 1;
}
} else {
@@ -131,8 +160,8 @@ pub fn futex(addr: &mut i32, op: usize, val: i32, val2: usize, addr2: *mut i32)
}
}
while i < futexes.len() && requeued < val2 {
if futexes[i].0 == addr as *mut i32 as usize {
futexes[i].0 = addr2_safe as *mut i32 as usize;
if futexes[i].target_physaddr == target_physaddr {
futexes[i].target_physaddr = addr2_physaddr;
requeued += 1;
}
i += 1;