Unsoundness when using shared resources in exception handlers on ARMv6-M

[FeldrinH]

According to the RTIC documentation (source):

[...] ARMv6-M/ARMv8-M-base cannot have tasks with shared resources bound to exception handlers, as these cannot be masked in hardware.

This sentence seems to indicate that on ARMv6-M something like this won't work (and indeed on RTIC v1 this produced a compile-time error):

#[task(binds = SysTick, shared = [peripheral])]
fn watchdog(mut ctx: watchdog::Context) {
    ctx.shared.peripheral.lock(|p| p.whatever());
}

However, with RTIC v2 this code compiles just fine on thumbv6m-none-eabi with the thumbv6-backend feature enabled.

What's going on here? Is there a bug in RTIC? Is there outdated info in the documentation? Am I just completely misunderstanding something?

[perlindgren]

Thanks for spotting this!

This looks like a bug. One could potentially per-core peripheral enable/disable the corresponding interrupt source according to the current system ceiling but this is not implemented, thus the example above might cause a race condition on the shared resource.

[FeldrinH]

This seems rather troublesome. If I understand correctly the current behavior could lead to UB in safe code. Is there hope for a quick fix?

[FeldrinH]

A simple but somewhat heavy-handed fix might be to just use a global critical section when one of the tasks using a shared resource is an exception handler.

[FeldrinH]

Maybe this would be a better and more general fix: Instead of masking interrupts and disabling exception handlers, it would be simpler and more straightforward to temporarily raise the priority of the current interrupt/exception to ceiling using NVIC IPR registers.

EDIT: Seems like this can't cover all edge cases. As far as I can tell there is no way to raise the priority of the idle task without BASEPRI.

[FeldrinH]

A simple but somewhat heavy-handed fix might be to just use a global critical section when one of the tasks using a shared resource is an exception handler.

I made a PR implementing this fix. Trying to selectively disable specific exceptions seemed quite convoluted, so I left that as a future development for someone with more time and insight.

[perlindgren]

If I recall correctly, changing the priority of an executing interrupt handler may lead to unexpected/undefined behavior on v6m, and as pointed out in the PR the idle task has no running priority, so that approach is not really applicable.

Disabling individual exceptions should work but each exception need some special treatment. For non-maskable exceptions (NMI) the integrity of shared resources might be compromised (as an NMI might occur at any time), so if we want to ensure soundness (guaranteed race-free execution) I don't think there is a way.

One observation though. As long as an exception bound task(s) has the same priority as all other (interrupt bound) tasks accessing the same set (or subset) of resources one should be able to use lock free implementation for both the exception handler(s) and corresponding interrupt driven task(s), but that is really an edge case.

I don't see an elegant solution for this problem, I foresee two possible solutions:

1. brute force exception masking
2. using the FAULT_MASK to disable all maskable exceptions
   Using FAULT_MASK will affect only maskable exceptions, so its not really a global critical section. The lock code generation for normal tasks should disable other normal tasks as we do now, AND set the FAULT_MASK if shared with any exception handler tasks. Codegen for exceptions need to disable other normal tasks as we do now, AND set the FAULT mask in case there is any other (maskable) exception that shares the same resource AND has higher priority.

Alternative 1 is a bit cumbersome, lots of edge cases, but has the proper locking semantics under SRP (besides for NMIs which are sort of error handlers, and not normal tasks).
Alternative 2 is much simpler to implement, it breaks with SRP, but only among exceptions so a reasonable, lest bad solution called for by the assymetric cumbersome exception handling of v6m.

I hope that should cover all cases, but the devil is in the details.

/Per

[perlindgren]

Hmm, seems that also FAULT_MASK is omitted on the M0, so only the manual exception masking would work.

A super ugly solution might be to use the exception handler as trampoline triggering (pending) an interrupt running the actual task. Then we don't need to worry about the pesky M0 architecture and we don't need to do anything regarding codegen. We should just emit a compilation error in case of exception handlers with shared resources (like in RTIC-v1).

One could automate the trampoline code gen, but I'm not sure its worth it.

Maybe as an separate compilation pass under the upcoming RTIC-eVo....
/Per

[FeldrinH]

It seems that all the options have their own issues. I think for now my PR is actually one of the better options.

We should just emit a compilation error in case of exception handlers with shared resources (like in RTIC-v1).

This feels kind of iffy to me, because adding this compilation error now would be a breaking change for everyone that is currently using shared resources in exception handlers.

[FeldrinH]

If I recall correctly, changing the priority of an executing interrupt handler may lead to unexpected/undefined behavior on v6m

This is news to me. Is there some discussion/reference documentation where I could read more about this? Does it cause actual undefined behavior or just unintuitive behavior?