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

fix(switch): deadlock

Browse Source 
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, &current_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<usize> (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<kernel::context::context::Context, spin::relax::Spin>::write<kernel::context::context::Context, spin::relax::Spin> (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<u8> {
    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<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
  borrow: core::cell::Cell<isize> {
    value: core::cell::UnsafeCell<isize> {
      value: 0
    }
  },
  value: core::cell::UnsafeCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
    value: core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>::Some(alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global> {
        ptr: core::ptr::non_null::NonNull<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>> {
          pointer: 0xffffff7f80188100
        },
        phantom: core::marker::PhantomData<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>>,
        alloc: alloc::alloc::Global
      })
  }
}
pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[2].p.value).current_addrsp
$7 = core::cell::RefCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
  borrow: core::cell::Cell<isize> {
    value: core::cell::UnsafeCell<isize> {
      value: 0
    }
  },
  value: core::cell::UnsafeCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
    value: core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>::Some(alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global> {
        ptr: core::ptr::non_null::NonNull<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>> {
          pointer: 0xffffff7f80188100
        },
        phantom: core::marker::PhantomData<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>>,
        alloc: alloc::alloc::Global
      })
  }
}
pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[1].p.value).current_addrsp
$8 = core::cell::RefCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
  borrow: core::cell::Cell<isize> {
    value: core::cell::UnsafeCell<isize> {
      value: 0
    }
  },
  value: core::cell::UnsafeCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
    value: core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>::Some(alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global> {
        ptr: core::ptr::non_null::NonNull<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>> {
          pointer: 0xffffff7f80188100
        },
        phantom: core::marker::PhantomData<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>>,
        alloc: alloc::alloc::Global
      })
  }
}
pwndbg> p (*kernel::percpu::ALL_PERCPU_BLOCKS[0].p.value).current_addrsp
$9 = core::cell::RefCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
  borrow: core::cell::Cell<isize> {
    value: core::cell::UnsafeCell<isize> {
      value: 0
    }
  },
  value: core::cell::UnsafeCell<core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>> {
    value: core::option::Option<alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global>>::Some(alloc::sync::Arc<kernel::context::memory::AddrSpaceWrapper, alloc::alloc::Global> {
        ptr: core::ptr::non_null::NonNull<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>> {
          pointer: 0xffffff7f80133ac8
        },
        phantom: core::marker::PhantomData<alloc::sync::ArcInner<kernel::context::memory::AddrSpaceWrapper>>,
        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<usize> (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<kernel::context::context::Context, spin::relax::Spin>::write<kernel::context::context::Context, spin::relax::Spin> (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 <kernel::context::switch::switch+1282>
    4   "" stopped: 0xffffffff80074d5a <kernel::context::switch::switch+442>
    3   "" stopped: 0xffffffff80059cf9 <kernel::context::context::Context::set_addr_space+217>
    2   "" stopped: 0xffffffff80020839 <kernel::context::memory::Flusher::flush+361>
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
```

Related: https://gitlab.redox-os.org/redox-os/kernel/-/issues/175

Signed-off-by: Anhad Singh <andypython@protonmail.com>
This commit is contained in:
Anhad Singh
2026-01-19 15:21:13 +11:00
parent a919d2626b
commit 7d42e8d966
4 changed files with 62 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/context/context.rs
+7 -6
View File
@@ -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();
}
src/context/memory.rs
+31 -17
View File
@@ -82,14 +82,16 @@ impl UnmapResult {
#[derive(Debug)]
pub struct AddrSpaceWrapper {
inner: RwLock<AddrSpace>,
pub inner: RwLock<AddrSpace>,
pub tlb_ack: AtomicU32,
pub used_by: LogicalCpuSet,
}
impl AddrSpaceWrapper {
pub fn new() -> Result<Arc<Self>> {
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<RmmA>,
file_ref: GrantFileRef,
src: Option<BorrowedFmapSource<'_>>,
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<RmmA>, 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<NonZeroUsize>)
}
}
}
#[derive(Debug)]
struct FlusherState<'addrsp> {
// TODO: what capacity?
pagequeue: ArrayVec<PageQueueEntry, 32>,
@@ -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;
src/cpu_set.rs
+10 -6
View File
@@ -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<Item = LogicalCpuId> + '_ {
// TODO: Will this be optimized away?
self.0.iter_mut().enumerate().flat_map(move |(i, w)| {
pub fn iter(&self) -> impl Iterator<Item = LogicalCpuId> + '_ {
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
src/percpu.rs
+14 -5
View File
@@ -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();
}
_ => {