From 7d42e8d9666f310f0215799bff28636ebc7bc073 Mon Sep 17 00:00:00 2001 From: Anhad Singh Date: Mon, 19 Jan 2026 15:21:13 +1100 Subject: [PATCH] fix(switch): deadlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During the snapshot shown, interrupts would be _disabled_ on both CPUs and both would have have the _same_ address space (i.e. `PercpuBlock::current().current_addrsp`). ```rs // CPU 0 CPU 1 (inside `context::switch()`) // ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ // let guard = current_addrsp.acquire_write() // let flusher = Flusher::with_cpu_set(&addrsp.used_by, ¤t_addrsp.tlb_ack); // ... // drop(flusher); // Flusher::flush() // // send IPI to CPU 1 as it is in the `active_set` // shootdown_tlb_ipi(Some(1)); // percpu::switch_arch_hook(); // // spin until TLB shootdown IPI has been acknowledged by CPU 1 // acquire_read() to remove the CPU from the `used_by` set in `prev_addrsp` // while ackword < affected_cpu_count prev_addrsp.acquire_read() // PercpuBlock::current().maybe_handle_tlb_shootdown(); // spin_loop(); // } // drop(guard); ``` Now CPU 0 will spin _until_ CPU 1 has acknowledged the IPI. However, CPU 1 has interrupts disabled and is in the middle of a context switch. To complete the context switch it needs to remove itself from the `used_by` set in `prev_addrsp` (which is the current address space atm). To do that it needs to `acquire_read()` lock but it cannot since it is held by CPU 0 and won't release it until CPU 1 has acknowledged the IPI; creating a deadlock. Also note: ```rs // src/context/memory.rs // NOTE: Lock must be held, which must be guaranteed by the caller. pub fn flush(&mut self) { ``` Here is some output from GDB to back this up: ``` pwndbg> info threads Id Target Id Frame 1 Thread 1.1 (CPU#0 [running]) core::sync::atomic::atomic_compare_exchange_weak (dst=0xffffff7f801972f0, old=0, new=1, success=core::sync::atomic::Ordering::Acquire, failure=core::sync::atomic::Ordering::Relaxed) at /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:4100 2 Thread 1.2 (CPU#1 [running]) kernel::context::memory::Flusher::flush (self=0xffff80004212f4b8) at src/context/memory.rs:2827 3 Thread 1.3 (CPU#2 [running]) spin::rwlock::RwLock::write (self=0xffffff7f801a99c0) at /root/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/spin-0.9.8/src/rwlock.rs:239 * 4 Thread 1.4 (CPU#3 [running]) 0xffffffff80059cf9 in kernel::context::memory::AddrSpaceWrapper::acquire_read (self=0xffffff7f80188110) at /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs:25 pwndbg> p kernel::context::arch::CONTEXT_SWITCH_LOCK $1 = core::sync::atomic::AtomicBool { v: core::cell::UnsafeCell { value: 1 } } pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[0].p.value).wants_tlb_shootdown.v.value $2 = 0 pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[1].p.value).wants_tlb_shootdown.v.value $3 = 0 pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[2].p.value).wants_tlb_shootdown.v.value $4 = 1 pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[3].p.value).wants_tlb_shootdown.v.value $5 = 0 pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[3].p.value).current_addrsp $6 = core::cell::RefCell>> { borrow: core::cell::Cell { value: core::cell::UnsafeCell { value: 0 } }, value: core::cell::UnsafeCell>> { value: core::option::Option>::Some(alloc::sync::Arc { ptr: core::ptr::non_null::NonNull> { pointer: 0xffffff7f80188100 }, phantom: core::marker::PhantomData>, alloc: alloc::alloc::Global }) } } pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[2].p.value).current_addrsp $7 = core::cell::RefCell>> { borrow: core::cell::Cell { value: core::cell::UnsafeCell { value: 0 } }, value: core::cell::UnsafeCell>> { value: core::option::Option>::Some(alloc::sync::Arc { ptr: core::ptr::non_null::NonNull> { pointer: 0xffffff7f80188100 }, phantom: core::marker::PhantomData>, alloc: alloc::alloc::Global }) } } pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[1].p.value).current_addrsp $8 = core::cell::RefCell>> { borrow: core::cell::Cell { value: core::cell::UnsafeCell { value: 0 } }, value: core::cell::UnsafeCell>> { value: core::option::Option>::Some(alloc::sync::Arc { ptr: core::ptr::non_null::NonNull> { pointer: 0xffffff7f80188100 }, phantom: core::marker::PhantomData>, alloc: alloc::alloc::Global }) } } pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[0].p.value).current_addrsp $9 = core::cell::RefCell>> { borrow: core::cell::Cell { value: core::cell::UnsafeCell { value: 0 } }, value: core::cell::UnsafeCell>> { value: core::option::Option>::Some(alloc::sync::Arc { ptr: core::ptr::non_null::NonNull> { pointer: 0xffffff7f80133ac8 }, phantom: core::marker::PhantomData>, alloc: alloc::alloc::Global }) } } ``` ``` pwndbg> thread 4 [Switching to thread 4 (Thread 1.4)] 25 in /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs pwndbg> info threads Id Target Id Frame 1 Thread 1.1 (CPU#0 [running]) core::sync::atomic::atomic_compare_exchange_weak (dst=0xffffff7f801972f0, old=0, new=1, success=core::sync::atomic::Ordering::Acquire, failure=core::sync::atomic::Ordering::Relaxed) at /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:4100 2 Thread 1.2 (CPU#1 [running]) kernel::context::memory::Flusher::flush (self=0xffff80004212f4b8) at src/context/memory.rs:2827 3 Thread 1.3 (CPU#2 [running]) spin::rwlock::RwLock::write (self=0xffffff7f801a99c0) at /root/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/spin-0.9.8/src/rwlock.rs:239 * 4 Thread 1.4 (CPU#3 [running]) 0xffffffff80059cf9 in kernel::context::memory::AddrSpaceWrapper::acquire_read (self=0xffffff7f80188110) at /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs:25 pwndbg> thread 4 [Switching to thread 4 (Thread 1.4)] 25 in /mnt/redox/prefix/x86_64-unknown-redox/sysroot/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/x86/sse2.rs pwndbg> bt ``` This is *after* everything freezes. For relibc tests, if I run `while true; do /root/relibc-tests/pthread/once; done` it eventually does freeze up and does so at the same location: ``` ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────[ THREADS (4 TOTAL) ]────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ► 1 "" stopped: 0xffffffff800750a2 4 "" stopped: 0xffffffff80074d5a 3 "" stopped: 0xffffffff80059cf9 2 "" stopped: 0xffffffff80020839 ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ``` Related: https://gitlab.redox-os.org/redox-os/kernel/-/issues/175 Signed-off-by: Anhad Singh --- src/context/context.rs | 13 ++++++------ src/context/memory.rs | 48 +++++++++++++++++++++++++++--------------- src/cpu_set.rs | 16 ++++++++------ src/percpu.rs | 19 ++++++++++++----- 4 files changed, 62 insertions(+), 34 deletions(-) diff --git a/src/context/context.rs b/src/context/context.rs index f1de4ab1f6..f256b27943 100644 --- a/src/context/context.rs +++ b/src/context/context.rs @@ -355,10 +355,12 @@ impl Context { this_percpu.current_addrsp.borrow().as_ref().unwrap(), prev_addrsp )); - prev_addrsp - .acquire_read() - .used_by - .atomic_clear(this_percpu.cpu_id); + + // See [`crate::percpu::switch_arch_hook`]. + prev_addrsp.used_by.atomic_clear(this_percpu.cpu_id); + + core::sync::atomic::fence(Ordering::SeqCst); + this_percpu.maybe_handle_tlb_shootdown(); } let _old_addrsp = core::mem::replace( @@ -368,9 +370,8 @@ impl Context { match addr_space { Some(ref new) => { + new.used_by.atomic_set(this_percpu.cpu_id); let new_addrsp = new.acquire_read(); - new_addrsp.used_by.atomic_set(this_percpu.cpu_id); - unsafe { new_addrsp.table.utable.make_current(); } diff --git a/src/context/memory.rs b/src/context/memory.rs index 45eb69fc4e..5d62b71713 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -82,14 +82,16 @@ impl UnmapResult { #[derive(Debug)] pub struct AddrSpaceWrapper { - inner: RwLock, + pub inner: RwLock, pub tlb_ack: AtomicU32, + pub used_by: LogicalCpuSet, } impl AddrSpaceWrapper { pub fn new() -> Result> { Ok(Arc::new(Self { inner: RwLock::new(AddrSpace::new()?), tlb_ack: AtomicU32::new(0), + used_by: LogicalCpuSet::empty(), })) } pub fn acquire_read(&self) -> RwLockReadGuard<'_, AddrSpace> { @@ -155,8 +157,9 @@ impl AddrSpaceWrapper { let new = Arc::get_mut(&mut new_arc).expect("expected new address space Arc not to be aliased"); + let _this_mapper = &mut guard.table.utable; let this_mapper = &mut guard.table.utable; - let mut this_flusher = Flusher::with_cpu_set(&mut guard.used_by, &self.tlb_ack); + let mut this_flusher = Flusher::with_cpu_set(&self.used_by, &self.tlb_ack); for (grant_base, grant_info) in guard.grants.iter() { let new_grant = match grant_info.provider { @@ -241,7 +244,7 @@ impl AddrSpaceWrapper { let guard = &mut *guard; let mapper = &mut guard.table.utable; - let mut flusher = Flusher::with_cpu_set(&mut guard.used_by, &self.tlb_ack); + let mut flusher = Flusher::with_cpu_set(&self.used_by, &self.tlb_ack); // TODO: Remove allocation (might require BTreeMap::set_key or interior mutability). let regions = guard @@ -305,7 +308,7 @@ impl AddrSpaceWrapper { let mut guard = self.acquire_write(); let guard = &mut *guard; - let mut flusher = Flusher::with_cpu_set(&mut guard.used_by, &self.tlb_ack); + let mut flusher = Flusher::with_cpu_set(&self.used_by, &self.tlb_ack); AddrSpace::munmap_inner( &mut guard.grants, &mut guard.table.utable, @@ -330,12 +333,12 @@ impl AddrSpaceWrapper { let mut src_flusher; let mut src_opt = match src_opt { Some((aw, a)) => { - src_flusher = Flusher::with_cpu_set(&mut a.used_by, &aw.tlb_ack); + src_flusher = Flusher::with_cpu_set(&aw.used_by, &aw.tlb_ack); Some((&mut a.grants, &mut a.table.utable, &mut src_flusher)) } None => None, }; - let mut dst_flusher = Flusher::with_cpu_set(&mut dst.used_by, &dst_lock.tlb_ack); + let mut dst_flusher = Flusher::with_cpu_set(&dst_lock.used_by, &dst_lock.tlb_ack); let dst_base = match requested_dst_base { Some(base) if new_flags.contains(MapFlags::MAP_FIXED_NOREPLACE) => { @@ -711,7 +714,7 @@ impl AddrSpace { selected_span.base, page_flags(flags), &mut self.table.utable, - &mut Flusher::with_cpu_set(&mut self.used_by, &dst_lock.tlb_ack), + &mut Flusher::with_cpu_set(&dst_lock.used_by, &dst_lock.tlb_ack), )?; self.grants.insert(grant); @@ -1329,7 +1332,7 @@ impl Grant { new_flags: PageFlags, file_ref: GrantFileRef, src: Option>, - lock: &AddrSpaceWrapper, + _lock: &AddrSpaceWrapper, mapper: &mut PageMapper, flusher: &mut Flusher, token: &mut CleanLockToken, @@ -1338,7 +1341,8 @@ impl Grant { let mut guard = src.addr_space_guard; let mut src_addrspace = &mut *guard; let mut src_flusher_state = - Flusher::with_cpu_set(&mut src_addrspace.used_by, &lock.tlb_ack).detach(); + Flusher::with_cpu_set(&src.addr_space_lock.used_by, &src.addr_space_lock.tlb_ack) + .detach(); for dst_page in span.pages() { let src_page = src.src_base.next_by(dst_page.offset_from(span.base)); @@ -2401,7 +2405,7 @@ fn correct_inner<'l>( token: &mut CleanLockToken, ) -> Result<(Frame, PageFlush, RwLockWriteGuard<'l, AddrSpace>), PfError> { let mut addr_space = &mut *addr_space_guard; - let mut flusher = Flusher::with_cpu_set(&mut addr_space.used_by, &addr_space_lock.tlb_ack); + let mut flusher = Flusher::with_cpu_set(&addr_space_lock.used_by, &addr_space_lock.tlb_ack); let Some((grant_base, grant_info)) = addr_space.grants.contains(faulting_page) else { debug!("Lacks grant"); @@ -2651,7 +2655,7 @@ fn correct_inner<'l>( addr_space_guard = addr_space_lock.acquire_write(); addr_space = &mut *addr_space_guard; - flusher = Flusher::with_cpu_set(&mut addr_space.used_by, &addr_space_lock.tlb_ack); + flusher = Flusher::with_cpu_set(&addr_space_lock.used_by, &addr_space_lock.tlb_ack); info!("Got frame {:?} from external fmap", frame); @@ -2749,6 +2753,7 @@ fn handle_free_action(base: Frame, phys_contiguous_count: Option) } } } +#[derive(Debug)] struct FlusherState<'addrsp> { // TODO: what capacity? pagequeue: ArrayVec, @@ -2757,6 +2762,7 @@ struct FlusherState<'addrsp> { ackword: &'addrsp AtomicU32, } +#[derive(Debug)] enum PageQueueEntry { Free { base: Frame, @@ -2768,12 +2774,14 @@ enum PageQueueEntry { }, } -pub struct Flusher<'guard, 'addrsp> { - active_cpus: &'guard mut LogicalCpuSet, +#[derive(Debug)] +pub struct Flusher<'a, 'addrsp> { + active_cpus: &'a LogicalCpuSet, state: FlusherState<'addrsp>, } -impl<'guard, 'addrsp> Flusher<'guard, 'addrsp> { - fn with_cpu_set(set: &'guard mut LogicalCpuSet, ackword: &'addrsp AtomicU32) -> Self { + +impl<'a, 'addrsp> Flusher<'a, 'addrsp> { + fn with_cpu_set(set: &'a LogicalCpuSet, ackword: &'addrsp AtomicU32) -> Self { Self { active_cpus: set, state: FlusherState { @@ -2811,13 +2819,18 @@ impl<'guard, 'addrsp> Flusher<'guard, 'addrsp> { let current_cpu_id = crate::cpu_id(); - for cpu_id in self.active_cpus.iter_mut() { + for cpu_id in self.active_cpus.iter() { if cpu_id == current_cpu_id { continue; } crate::percpu::shootdown_tlb_ipi(Some(cpu_id)); - affected_cpu_count += 1; + + core::sync::atomic::fence(Ordering::SeqCst); + + if self.active_cpus.contains(cpu_id) { + affected_cpu_count += 1; + } } if self.active_cpus.contains(current_cpu_id) { @@ -2872,6 +2885,7 @@ impl Drop for Flusher<'_, '_> { } } bitflags::bitflags! { + #[derive(Debug)] pub struct TlbShootdownActions: usize { // Delay the deallocation of one or more contiguous frames. const FREE = 1; diff --git a/src/cpu_set.rs b/src/cpu_set.rs index 65871e0bc5..9e7a5a9b32 100644 --- a/src/cpu_set.rs +++ b/src/cpu_set.rs @@ -63,17 +63,21 @@ impl LogicalCpuSet { pub const fn empty() -> Self { Self([const { AtomicUsize::new(0) }; SET_WORDS]) } + pub const fn all() -> Self { Self([const { AtomicUsize::new(!0) }; SET_WORDS]) } - pub fn contains(&mut self, id: LogicalCpuId) -> bool { + + pub fn contains(&self, id: LogicalCpuId) -> bool { let (word, bit) = parts(id); - *self.0[word].get_mut() & (1 << bit) != 0 + self.0[word].load(Ordering::Acquire) & (1 << bit) != 0 } + pub fn atomic_set(&self, id: LogicalCpuId) { let (word, bit) = parts(id); let _ = self.0[word].fetch_or(1 << bit, Ordering::Release); } + pub fn atomic_clear(&self, id: LogicalCpuId) { let (word, bit) = parts(id); let _ = self.0[word].fetch_and(!(1 << bit), Ordering::Release); @@ -82,15 +86,15 @@ impl LogicalCpuSet { pub fn override_from(&mut self, raw: &RawMask) { self.0 = raw.map(AtomicUsize::new); } + pub fn to_raw(&self) -> RawMask { self.0.each_ref().map(|w| w.load(Ordering::Acquire)) } - pub fn iter_mut(&mut self) -> impl Iterator + '_ { - // TODO: Will this be optimized away? - self.0.iter_mut().enumerate().flat_map(move |(i, w)| { + pub fn iter(&self) -> impl Iterator + '_ { + self.0.iter().enumerate().flat_map(move |(i, w)| { (0..usize::BITS).filter_map(move |b| { - if *w.get_mut() & (1 << b) != 0 { + if w.load(Ordering::Acquire) & (1 << b) != 0 { Some(LogicalCpuId::new(i as u32 * usize::BITS + b)) } else { None diff --git a/src/percpu.rs b/src/percpu.rs index 7453772b6f..fa6adf64ba 100644 --- a/src/percpu.rs +++ b/src/percpu.rs @@ -142,10 +142,19 @@ pub unsafe fn switch_arch_hook() { return; } if let Some(prev_addrsp) = &*cur_addrsp { - prev_addrsp - .acquire_read() - .used_by - .atomic_clear(percpu.cpu_id); + prev_addrsp.used_by.atomic_clear(percpu.cpu_id); + + // See [`Flusher::flush`]. + // + // Without the fence, `wants_tlb_shootdown` check *may* happen + // before the CPU is removed from the `used_by` set. Hence, if a + // shootdown request arises *after* the check and *before* removing + // the CPU from the set, it would be missed and the CPU who + // requested the shootdown would spin forever since the request was + // never ACKed. + core::sync::atomic::fence(Ordering::SeqCst); + + percpu.maybe_handle_tlb_shootdown(); } drop(cur_addrsp); @@ -156,9 +165,9 @@ pub unsafe fn switch_arch_hook() { match &*percpu.current_addrsp.borrow() { Some(next_addrsp) => { + next_addrsp.used_by.atomic_set(percpu.cpu_id); let next = next_addrsp.acquire_read(); - next.used_by.atomic_set(percpu.cpu_id); next.table.utable.make_current(); } _ => {