Documentation: What is the contract to call NVIC::unmask soundly?

[rubberduck203]

NVIC::enable() was deprecated with the following warning.

WARNING This method is a soundness hole in the API; it should actually be an unsafe function. Use NVIC::unmask which has the right unsafety.

https://docs.rs/stm32f3xx-hal/0.3.0/stm32f3xx_hal/stm32/struct.NVIC.html#method.enable

NVIC::unmask() has the following documentation.

This function is unsafe because it can break mask-based critical sections

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.

[rubberduck203]

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

[rubberduck203]

List of UB in Rust

[rubberduck203]

The commit that introduced NVIC::unmask.
40a60d3

[rubberduck203]

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.

[rubberduck203]

@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?

[rubberduck203]

https://github.com/rtfm-rs/cortex-m-rtfm/blob/8a1f009c34b8cad3f7478aa67432fc60d47be4c0/book/en/src/internals/critical-sections.md#lock

https://github.com/rtfm-rs/cortex-m-rtfm/blob/master/src/export.rs#L120

[jonas-schievink]

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).