rust-embedded / riscv

Low level access to RISC-V processors
841 stars 162 forks source link

The `critical_section` implementation is wrong #116

Closed Tnze closed 2 years ago

Tnze commented 2 years ago

These two lines is not atomic and will cause problems

https://github.com/rust-embedded/riscv/blob/d9c60763b8fb53097ad1d95ff99c1eabe216dc34/src/critical_section.rs#L11-L13

and could be replaced by a single csr instruction:

let mut mstatus: usize;
asm!("csrrci {}, 0x300, 0b100", out(reg) mstatus);
core::mem::transmute::<_, Mstatus>(mstatus).mie()

which prevents from being interrupted between reading status and disabling interrupt.

Tnze commented 2 years ago

To prevent from using core::mem::transmute, we should make the Mstatus.bits public and then:

let mut mstatus: Mstatus;
asm!("csrrci {}, 0x300, 0b100", out(reg) mstatus.bits);
mstatus.mie()
almindor commented 2 years ago

Good find. Thanks, pushed PR, please review. I've decided to keep the bits pub(crate) only to not break the API

almindor commented 2 years ago

Hmm actually we can't make the Mstatus::bits pub(crate) since it'd allow instantiation which leads to UB.