Open rubberduck203 opened 4 years ago
Copying some discussion from the matrix channel here.
jamesmunns Unmask is unsafe if you are relying on mutexes/critical sections/rtfm Because those mechanisms rely on masked interrupts to prevent shared access to data, you must ensure that any interrupt you unmask is not using shared data in one of these ways So basically it is only unsafe because of how we implement mutexes/CSs by just masking all interrupts
rubberduck203 So, it's unsound to call unmask from inside of a critical section. That makes sense.
jamesmunns That sounds about right to me
rubberduck203 Does it follow that it is sound to call anywhere outside of a CS?
jamesmunns Could be wider than that in rtfm It depends on what libraries you are using, I suppose Anything that relies on masking of interrupts for soundness, unmask could be bad RTFM doesn't take full critical sections, only partial ones using primask, iirc But same issues remain It could still be sound inside of a critical section too, if you aren't sharing data with the interrupt you unmask, for example if it only increments an atomic counter or something (and you have full CAS) But "usually sound outside of a critical section" sounds like a reasonable rule of thumb, if not totally comprehensive
The commit that introduced NVIC::unmask
.
https://github.com/rust-embedded/cortex-m/commit/40a60d3d9a6431604751e98f3b4aeb073329510e
I’m at a loss. I don’t see how NVIC::unmask
could possibly cause undefined behavior. Even if called from inside of interrupt::free
, interrupts are disabled, so unmasking a particular vector would have no effect until the critical section has been exited (interrupts globally re-enabled).
Pseudo code
interrupt::disable();
NVIC::unmask(EXTI0);
// it’s okay to access shared data here
interrupt::enable();
The only potential I see here for UB is if a theoretical function existed that temporarily masked some interrupts, returning a critical section token.
pub fn exti0_free<F, R>(f: F) -> R
where
F: FnOnce(&CriticalSection) -> R,
{
NVIC::mask(EXTI0);
let r = f(unsafe { &CriticalSection::new() });
NVIC::unmask(EXTI0);
r
}
exti0_free(|cs| {
NVIC::unmask(EXTI0);
// access shared data after re-enabling interrupt that is supposed to be disabled
// depending on the access, this could result in a data race?
});
This could potentially result in a data race, but the call to NVIC::unmask
is not itself the unsafe
thing here. It’s accessing shared data in an unsynchronized context that could cause the undefined behavior.
The anonymous function itself should be unsafe
so far as I can tell. Unfortunately, I don’t believe there is something akin to UnsafeFnOnce
.
@jamesmunns I know japaric has been busy with other things, so I’d like to avoid pinging him as the original author of the code. Can you provide some more insight on this? Am I understanding this correctly?
Neither NVIC::unmask
nor interrupt::enable
can directly cause UB by themselves. However, if NVIC::unmask
were safe, it would be impossible to build something like RTFM and have it expose a safe interface to the user, and if interrupt::enable
were safe, the bare-metal mutex could also not expose a safe interface to user code (no mutex relying on disabling interrupts could).
It was decided that this was an unacceptable tradeoff, as almost every user would have to make sure to use these unsafe interfaces correctly, so "enable interrupts" was made unsafe
. At this point this decision is basically just a social contract that says "this cannot cause UB by itself, but please don't expose it as a safe operation anywhere, since we have other safe operations that would be unsound if this one were safe, too", but we should write it down in the wg repo (as it applies to all architectures).
NVIC::enable()
was deprecated with the following warning.https://docs.rs/stm32f3xx-hal/0.3.0/stm32f3xx_hal/stm32/struct.NVIC.html#method.enable
NVIC::unmask()
has the following documentation.https://docs.rs/stm32f3xx-hal/0.3.0/stm32f3xx_hal/stm32/struct.NVIC.html#method.unmask
This isn't sufficient information for someone to know how they can soundly call the
unmask
function to enable interrupts. The documentation should include a# Safety
section per the API guidelines describing the invariants the caller is expected to uphold.