Skip to content

Commit

Permalink
Fix thumbv7 soundness issue in the lock implementation
Browse files Browse the repository at this point in the history
The old lock implementation did not set basepri to max(current ceiling,
resource ceiling), it simply set basepri to the resource ceiling.
  • Loading branch information
korken89 committed Feb 27, 2024
1 parent 4a23c8d commit d2e8479
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 2 deletions.
1 change: 1 addition & 0 deletions rtic/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top!

### Fixed

- **Soundness fix:** `thumbv7` was subject to priority inversion.
- **Soundness fix:** Monotonics did not wait long enough in `Duration` based delays.
This is not directly a change for `rtic`, but required bumping the minimal version of `rtic-monotonics`.

Expand Down
6 changes: 6 additions & 0 deletions rtic/ci/expected/prio-inversion.run
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
foo - start
pre baz spawn 0 0
post baz spawn 0 0
baz - start
baz - end
foo - end
87 changes: 87 additions & 0 deletions rtic/examples/prio-inversion.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
//! examples/prio-inversion.rs
//!
//! Here we test to make sure we don't have priority inversion.
#![no_main]
#![no_std]
#![deny(warnings)]
#![deny(unsafe_code)]
#![deny(missing_docs)]
#![feature(type_alias_impl_trait)]

use panic_semihosting as _;
use rtic::app;

// t1 p1 use b, a
// t2 p2 use a
// t3 p3
// t4 p4 use b
//
// so t1 start , take b take a, pend t3
// t3 should not start
// try to see if it starts, IT SHOULD NOT

#[app(device = lm3s6965, dispatchers = [SSI0, QEI0, GPIOA, GPIOB])]
mod app {
use cortex_m_semihosting::{debug, hprintln};

#[shared]
struct Shared {
a: u32,
b: u32,
}

#[local]
struct Local {}

#[init]
fn init(_: init::Context) -> (Shared, Local) {
foo::spawn().unwrap();

(Shared { a: 0, b: 0 }, Local {})
}

#[task(priority = 1, shared = [a, b])]
async fn foo(cx: foo::Context) {
let foo::SharedResources { mut a, mut b, .. } = cx.shared;

hprintln!("foo - start");

// basepri = 0
b.lock(|b| {
// basepri = max(basepri = 0, ceil(b) = 4) = 4
a.lock(|a| {
// basepri = max(basepri = 4, ceil(a) = 2) = 4

hprintln!("pre baz spawn {} {}", a, b);

// This spawn should be blocked as prio(baz) = 3
baz::spawn().unwrap();

hprintln!("post baz spawn {} {}", a, b);
});
// basepri = 4
});
// basepri = 0

hprintln!("foo - end");
debug::exit(debug::EXIT_SUCCESS); // Exit QEMU simulator
}

#[task(priority = 2, shared = [a])]
async fn bar(_: bar::Context) {
hprintln!(" bar");
}

#[task(priority = 3)]
async fn baz(_: baz::Context) {
hprintln!(" baz - start");
hprintln!(" baz - end");
}

#[task(priority = 4, shared = [b])]
async fn pow(_: pow::Context) {
hprintln!(" pow - start");
hprintln!(" pow - end");
}
}
4 changes: 2 additions & 2 deletions rtic/src/export/cortex_basepri.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::cortex_logical2hw;
use cortex_m::register::basepri;
use cortex_m::register::{basepri, basepri_max};
pub use cortex_m::{
asm::wfi,
interrupt,
Expand Down Expand Up @@ -73,7 +73,7 @@ pub unsafe fn lock<T, R>(
critical_section::with(|_| f(&mut *ptr))
} else {
let current = basepri::read();
basepri::write(cortex_logical2hw(ceiling, nvic_prio_bits));
basepri_max::write(cortex_logical2hw(ceiling, nvic_prio_bits));
let r = f(&mut *ptr);
basepri::write(current);
r
Expand Down

0 comments on commit d2e8479

Please sign in to comment.