From b0506944738a763d32b46383d27e07d60e1688af Mon Sep 17 00:00:00 2001 From: Jan Niehusmann Date: Wed, 13 Nov 2024 22:00:58 +0000 Subject: [PATCH] Fix unsoundness in definition of stack for spawning core1 --- .../src/bin/multicore_fifo_blink.rs | 6 +- .../src/bin/multicore_polyblink.rs | 5 +- rp2040-hal/src/multicore.rs | 105 +++++++++++++++++- 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/rp2040-hal-examples/src/bin/multicore_fifo_blink.rs b/rp2040-hal-examples/src/bin/multicore_fifo_blink.rs index d5daa25d0..e05a22f0c 100644 --- a/rp2040-hal-examples/src/bin/multicore_fifo_blink.rs +++ b/rp2040-hal-examples/src/bin/multicore_fifo_blink.rs @@ -53,7 +53,7 @@ const CORE1_TASK_COMPLETE: u32 = 0xEE; /// So instead, core1.spawn takes a [usize] which gets used for the stack. /// NOTE: We use the `Stack` struct here to ensure that it has 32-byte alignment, which allows /// the stack guard to take up the least amount of usable RAM. -static mut CORE1_STACK: Stack<4096> = Stack::new(); +static CORE1_STACK: Stack<4096> = Stack::new(); fn core1_task(sys_freq: u32) -> ! { let mut pac = unsafe { pac::Peripherals::steal() }; @@ -116,9 +116,7 @@ fn main() -> ! { let cores = mc.cores(); let core1 = &mut cores[1]; #[allow(static_mut_refs)] - let _test = core1.spawn(unsafe { &mut CORE1_STACK.mem }, move || { - core1_task(sys_freq) - }); + let _test = core1.spawn(CORE1_STACK.take().unwrap(), move || core1_task(sys_freq)); /// How much we adjust the LED period every cycle const LED_PERIOD_INCREMENT: i32 = 2; diff --git a/rp2040-hal-examples/src/bin/multicore_polyblink.rs b/rp2040-hal-examples/src/bin/multicore_polyblink.rs index a399f13f7..2eaa8c1af 100644 --- a/rp2040-hal-examples/src/bin/multicore_polyblink.rs +++ b/rp2040-hal-examples/src/bin/multicore_polyblink.rs @@ -59,7 +59,7 @@ const CORE1_DELAY: u32 = 1_000_000 / CORE1_FREQ; /// NOTE: We use the `Stack` struct here to ensure that it has 32-byte /// alignment, which allows the stack guard to take up the least amount of /// usable RAM. -static mut CORE1_STACK: Stack<4096> = Stack::new(); +static CORE1_STACK: Stack<4096> = Stack::new(); /// Entry point to our bare-metal application. /// @@ -105,9 +105,8 @@ fn main() -> ! { let mut mc = Multicore::new(&mut pac.PSM, &mut pac.PPB, &mut sio.fifo); let cores = mc.cores(); let core1 = &mut cores[1]; - #[allow(static_mut_refs)] core1 - .spawn(unsafe { &mut CORE1_STACK.mem }, move || { + .spawn(CORE1_STACK.take().unwrap(), move || { // Get the second core's copy of the `CorePeripherals`, which are per-core. // Unfortunately, `cortex-m` doesn't support this properly right now, // so we have to use `steal`. diff --git a/rp2040-hal/src/multicore.rs b/rp2040-hal/src/multicore.rs index a70675cbf..cde321445 100644 --- a/rp2040-hal/src/multicore.rs +++ b/rp2040-hal/src/multicore.rs @@ -34,7 +34,10 @@ //! //! For a detailed example, see [examples/multicore_fifo_blink.rs](https://github.com/rp-rs/rp-hal/tree/main/rp2040-hal/examples/multicore_fifo_blink.rs) +use core::cell::Cell; +use core::cell::UnsafeCell; use core::mem::ManuallyDrop; +use core::ops::Range; use core::sync::atomic::compiler_fence; use core::sync::atomic::Ordering; @@ -93,7 +96,8 @@ pub struct Multicore<'p> { #[repr(C, align(32))] pub struct Stack { /// Memory to be used for the stack - pub mem: [usize; SIZE], + mem: UnsafeCell<[usize; SIZE]>, + taken: Cell, } impl Default for Stack { @@ -102,10 +106,100 @@ impl Default for Stack { } } +// Safety: Only one thread can `take` access to contents of the +// struct, guarded by a critical section. +unsafe impl Sync for Stack {} + impl Stack { /// Construct a stack of length SIZE, initialized to 0 + /// + /// The minimum allowed SIZE is 64 bytes, but most programs + /// will need a significantly larger stack. pub const fn new() -> Stack { - Stack { mem: [0; SIZE] } + const { assert!(SIZE >= 64, "Stack too small") }; + Stack { + mem: UnsafeCell::new([0; SIZE]), + taken: Cell::new(false), + } + } + + /// Take the StackAllocation out of this Stack. + /// + /// This returns None if the stack is already taken. + pub fn take(&self) -> Option { + let taken = critical_section::with(|_| self.taken.replace(true)); + if taken { + None + } else { + // Safety: We know the size of this allocation + unsafe { + let start = self.mem.get() as *mut usize; + let end = start.add(SIZE); + Some(StackAllocation::from_raw_parts(start, end)) + } + } + } + + /// Reset the taken flag of the stack area + /// + /// # Safety + /// + /// The caller must ensure that the stack is no longer in use, eg. because + /// the core that used it was reset. This method doesn't do any synchronization + /// so it must not be called from multiple threads concurrently. + pub unsafe fn reset(&self) { + self.taken.replace(false); + } +} + +/// This object represents a memory area which can be used for a stack. +/// +/// It is essentially a range of pointers with these additional guarantees: +/// The begin / end pointers must define a stack +/// with proper alignment (at least 8 bytes, preferably 32 bytes) +/// and sufficient size (64 bytes would be sound but much too little for +/// most real-world workloads). The underlying memory must +/// have a static lifetime and must be owned by the object exclusively. +/// No mutable references to that memory must exist. +/// Therefore, a function that gets passed such an object is free to write +/// to arbitrary memory locations in the range. +pub struct StackAllocation { + /// Start and end pointer of the StackAllocation as a Range + mem: Range<*mut usize>, +} + +impl StackAllocation { + fn get(&self) -> Range<*mut usize> { + self.mem.clone() + } + + /// Unsafely construct a stack allocation + /// + /// This is mainly useful to construct a stack allocation in some memory region + /// defined in a linker script, for example to place the stack in the SRAM4/5 regions. + /// + /// # Safety + /// + /// The caller must ensure that the guarantees that a StackAllocation provides + /// are upheld. + pub unsafe fn from_raw_parts(start: *mut usize, end: *mut usize) -> Self { + StackAllocation { mem: start..end } + } +} + +impl From<&Stack> for Option { + fn from(stack: &Stack) -> Self { + let taken = critical_section::with(|_| stack.taken.replace(true)); + if taken { + None + } else { + // Safety: We know the size of this allocation + unsafe { + let start = stack.mem.get() as *mut usize; + let end = start.add(SIZE); + Some(StackAllocation::from_raw_parts(start, end)) + } + } } } @@ -161,7 +255,7 @@ impl Core<'_> { /// likely if the core being reset happens to be inside a critical section. /// It may even break safety assumptions of some unsafe code. So, be careful when calling this method /// more than once. - pub fn spawn(&mut self, stack: &'static mut [usize], entry: F) -> Result<(), Error> + pub fn spawn(&mut self, stack: StackAllocation, entry: F) -> Result<(), Error> where F: FnOnce() + Send + 'static, { @@ -212,7 +306,8 @@ impl Core<'_> { // array size to guaranty that the base of the stack (the end of the array) meets that requirement. // The start of the array does not need to be aligned. - let mut stack_ptr = stack.as_mut_ptr_range().end; + let stack = stack.get(); + let mut stack_ptr = stack.end; // on rp2040, usize are 4 bytes, so align_offset(8) on a *mut usize returns either 0 or 1. let misalignment_offset = stack_ptr.align_offset(8); @@ -225,7 +320,7 @@ impl Core<'_> { // Push `stack_limit`. stack_ptr = stack_ptr.sub(1); - stack_ptr.cast::<*mut usize>().write(stack.as_mut_ptr()); + stack_ptr.cast::<*mut usize>().write(stack.start); // Push `entry`. stack_ptr = stack_ptr.sub(1);