Skip to content

Commit

Permalink
Fix interrupt::free() unsoundness on multi-hart systems.
Browse files Browse the repository at this point in the history
This is unsound on multi-hart because it only disables interrupts in the
current hart. For multi-hart chips, a chip-specific critical section implementation
is needed instead.

Unsoundness is fixed by not returning the `CriticalSection` token.

This is a breaking change.
  • Loading branch information
Dirbaio committed Oct 12, 2022
1 parent 65db20e commit caec777
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 11 deletions.
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ targets = [
critical-section-single-hart = ["critical-section/restore-state-bool"]

[dependencies]
bare-metal = "1.0.0"
bit_field = "0.10.0"
critical-section = "1.1.0"
embedded-hal = "0.2.6"
20 changes: 12 additions & 8 deletions src/interrupt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

// NOTE: Adapted from cortex-m/src/interrupt.rs
use crate::register::mstatus;
pub use bare_metal::{CriticalSection, Mutex};

/// Disables all interrupts
/// Disables all interrupts in the current hart.
#[inline]
pub unsafe fn disable() {
match () {
Expand All @@ -15,11 +14,11 @@ pub unsafe fn disable() {
}
}

/// Enables all the interrupts
/// Enables all the interrupts in the current hart.
///
/// # Safety
///
/// - Do not call this function inside an `interrupt::free` critical section
/// - Do not call this function inside a critical section.
#[inline]
pub unsafe fn enable() {
match () {
Expand All @@ -30,13 +29,18 @@ pub unsafe fn enable() {
}
}

/// Execute closure `f` in an interrupt-free context.
/// Execute closure `f` with interrupts disabled in the current hart.
///
/// This as also known as a "critical section".
/// This method does not synchronise multiple harts, so it is not suitable for
/// using as a critical section. See the `critical-section` crate for a cross-platform
/// way to enter a critical section which provides a `CriticalSection` token.
///
/// This crate provides an implementation for `critical-section` suitable for single-hart systems,
/// based on disabling all interrupts. It can be enabled with the `critical-section-single-hart` feature.
#[inline]
pub fn free<F, R>(f: F) -> R
where
F: FnOnce(&CriticalSection) -> R,
F: FnOnce() -> R,
{
let mstatus = mstatus::read();

Expand All @@ -45,7 +49,7 @@ where
disable();
}

let r = f(unsafe { &CriticalSection::new() });
let r = f();

// If the interrupts were active before our `disable` call, then re-enable
// them. Otherwise, keep them disabled
Expand Down
7 changes: 7 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,10 @@ mod macros;

#[cfg(all(riscv, feature = "critical-section-single-hart"))]
mod critical_section;

/// Used to reexport items for use in macros. Do not use directly.
/// Not covered by semver guarantees.
#[doc(hidden)]
pub mod _export {
pub use critical_section;
}
7 changes: 5 additions & 2 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
/// at most once in the whole lifetime of the program.
///
/// # Note
/// this macro is unsound on multi-core systems
///
/// This macro requires a `critical-section` implementation to be set. For most single-hart systems,
/// you can enable the `critical-section-single-hart` feature for this crate. For other systems, you
/// have to provide one from elsewhere, typically your chip's HAL crate.
///
/// # Example
///
Expand All @@ -29,7 +32,7 @@
#[macro_export]
macro_rules! singleton {
(: $ty:ty = $expr:expr) => {
$crate::interrupt::free(|_| {
$crate::_export::critical_section::with(|_| {
static mut VAR: Option<$ty> = None;

#[allow(unsafe_code)]
Expand Down

0 comments on commit caec777

Please sign in to comment.