Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fix interrupt::free() unsoundness on multicore systems.
This is unsound on multicore systems because it only disables interrupts in the
current core. For multicore 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 Aug 21, 2022
commit e38d775761b5b2ffb296359bc4a5c9e9f04e0796
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-core = ["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 core.
#[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 core.
///
/// # 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 core.
///
/// This as also known as a "critical section".
/// This method does not synchronise multiple cores, 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-core systems,
/// based on disabling all interrupts. It can be enabled with the `critical-section-single-core` 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 @@ -25,3 +25,10 @@ mod macros;

#[cfg(all(riscv, feature = "critical-section-single-core"))]
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 single core systems, you can
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe feature-gate this macro with critical-section-single-core?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro can still be used if critical-section-single-core is not enabled, if the user supplies a c-s implementation from somewhere else (such as a chip-specific impl for a multicore chip, from its HAL crate).

/// enable the `critical-section-single-core` feature for this crate. For multi core 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