`interrupt::free` does not disable all exceptions, which breaks critical sections

[jonas-schievink]

It calls interrupt::disable, which uses cpsid i, which disables interrupts and exceptions that have configurable priority by setting PRIMASK. Notably, this does not affect (on thumbv6) the NMI and HardFault exceptions since they have fixed priorities.

This means that interrupt::free is unsoundly creating a CriticalSection token when it's not allowed to do so.

Note that it is impossible to fully fix this just in the cortex-m crate, since NMI is always enabled and can not be masked (which is its entire point, I suppose). Also, FAULTMASK could be used to also disable HardFault, but that only exists on thumbv7+ from what I can tell.

Our options here are:

• Change cortex-m-rt to require unsafe to register HardFault, NMI, etc. handlers
• Use FAULTMASK / cpsid f when targeting thumbv7+, which could allow safe registration of fault handlers except NMI on thumbv7+ (note that FAULTMASK has somewhat subtle behavior that differs from PRIMASK, so this needs some extra care to make sure it's sound)
• Make interrupt::free an unsafe fn and document that it's only safe to call when not in one of the non-maskable fault handlers

We should probably change the docs of bare_metal::CriticalSection to reflect semantics when NMIs are present.

This issue is somewhat thorny since 3 crates rely on properties of each other for soundness (cortex-m, cortex-m-rt, and bare-metal).

(thanks to @japaric for noticing this)

[jonas-schievink]

One of the consequences of this is that registering HardFault to use hprintln! is never sound since that calls interrupt::free internally. Since this seems to be a common use case of HardFault, this is very unfortunate.

[Disasm]

I wonder if it's something that could be implemented as a clippy lint.

[adamgreig]

Hah, what a pain! I think this warrants some careful thought (and reading of the architecture reference manuals) but I have a few early thoughts:

Change cortex-m-rt to require unsafe to register HardFault, NMI, etc. handlers

This doesn't seem terrible, but I'm not sure what meaningful impact it will have in practice; users will presumably still want to access things like semihosting or ITM or whatever from HardFault so we should aim to provide a suitable mechanism.

Also I think the list of potentially unsound fault handlers is exclusively HardFault and NMI (including secure HardFault on ARMv8-M), as all other handlers have configurable priority so cannot pre-empt execution while PRIMASK is set.

Use FAULTMASK / cpsid f when targeting thumbv7+, which could allow safe registration of fault handlers except NMI on thumbv7+

When FAULTMASK is set, the priority level is -1, which has the extra behaviour that any fault or supervisor call causes the processor to enter the Lockup state, from which it generally* cannot recover. I don't think it's ideal to have all our critical sections be in this mode. For example, no user handler would be called if any fault (hard, mem, bus, etc) occurred during such a critical section.

* see ARMv7-M architecture reference manual B1.5.15 for the edge cases where recovery is possible if ill-advised...

Make interrupt::free an unsafe fn and document that it's only safe to call when not in one of the non-maskable fault handlers

While this prevents safe code accessing CriticalSection-based Mutexes from inside HardFault/NMI, it's no use to users who actually want to use a shared resource from inside HardFault, who are presumably left with only the option to put an unsafe block in. I'm not saying we shouldn't do this, just we shouldn't just do this.