Skip to content
Merged
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 multi-hart systems.
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
commit caec77731c598d2f7dca854cc1ed2b326c3dcb8a
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"
Copy link
Member

Choose a reason for hiding this comment

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

Make it optional maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's required unconditionally by the singleton! macro

(and 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)

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