zephyr: sync: Disallow Mutex reentrancy to prevent unsound mutable aliasing (#181)

## Problem

`zephyr::sync::Mutex` wraps Zephyr's underlying `k_mutex`. In Zephyr,
`k_mutex` is recursive by design, allowing the owning thread to lock it
multiple times without blocking. Consequently, `Mutex::lock()` and
`Mutex::try_lock()` allowed reentrant acquisition from the same thread.

Because `MutexGuard` implements `DerefMut`, allowing reentrancy
permitted safe Rust code to obtain multiple concurrent mutable
references (`&mut T`) to the inner data:

```rust
let mutex = Mutex::new(0);
let mut g1 = mutex.lock().unwrap();
let mut g2 = mutex.lock().unwrap(); // Succeeded because k_mutex is recursive!
let r1: &mut i32 = &mut *g1;
let r2: &mut i32 = &mut *g2; // Undefined Behavior: concurrent mutable references to the same data
```

This violates Rust's fundamental aliasing rules ("aliasing XOR
mutability") and introduces undefined behavior in safe code.

## Solution

Track whether the mutex is currently locked by a guard using an
`AtomicBool`:
1. In `lock()`, acquire the underlying `sys::Mutex`. If `locked` was
already true (indicating recursive acquisition by the current thread),
immediately unlock the `sys::Mutex` to restore its lock count and panic
with `"cannot recursively lock Mutex"`.
2. In `try_lock()`, acquire the underlying `sys::Mutex`. If `locked` was
already true, immediately unlock and return
`Err(TryLockError::WouldBlock)`.
3. In `MutexGuard::drop()`, clear `locked` before unlocking
`sys::Mutex`.
4. In `Condvar::wait()`, clear `locked` before calling
`sys::Condvar::wait()` and re-assert `locked` upon re-acquiring the
mutex.

## Prior Issues / PRs

Checked the repository issues and pull requests; there are currently no
open or linked pull requests or issues addressing mutex reentrancy
soundness.

Author: d3zd3z

zephyr/src/sync/mutex.rs

@@ -12,6 +12,7 @@ use core::{
     ops::{Deref, DerefMut},
 };
 
+use crate::sync::atomic::{AtomicBool, Ordering};
 use crate::sys::sync as sys;
 use crate::time::{Forever, NoWait};
 
@@ -46,6 +47,7 @@ pub enum TryLockError {
 pub struct Mutex<T: ?Sized> {
     inner: sys::Mutex,
     // poison: ...
+    locked: AtomicBool,
     data: UnsafeCell<T>,
 }
 
@@ -93,6 +95,7 @@ impl<T> Mutex<T> {
     pub const fn new_from(t: T, raw_mutex: sys::Mutex) -> Mutex<T> {
         Mutex {
             inner: raw_mutex,
+            locked: AtomicBool::new(false),
             data: UnsafeCell::new(t),
         }
     }
@@ -111,12 +114,17 @@ impl<T: ?Sized> Mutex<T> {
     /// allow scoped unlock of the lock. When the guard goes out of scope, the mutex will be
     /// unlocked.
     ///
-    /// In `std`, an attempt to lock a mutex by a thread that already holds the mutex is
-    /// unspecified.  Zephyr explicitly supports this behavior, by simply incrementing a lock
-    /// count.
+    /// Attempting to lock a mutex that is already held by the current thread is not permitted and
+    /// will panic. Because [`MutexGuard`] provides mutable access to the protected data via
+    /// [`DerefMut`], allowing reentrancy would produce multiple mutable references to the same
+    /// data, which is undefined behavior in Rust.
     pub fn lock(&self) -> LockResult<MutexGuard<'_, T>> {
         // With `Forever`, should never return an error.
         self.inner.lock(Forever).unwrap();
+        if self.locked.swap(true, Ordering::Acquire) {
+            self.inner.unlock().unwrap();
+            panic!("cannot recursively lock Mutex");
+        }
         unsafe { Ok(MutexGuard::new(self)) }
     }
 
@@ -125,10 +133,18 @@ impl<T: ?Sized> Mutex<T> {
     /// If the lock could not be acquired at this time, then [`Err`] is returned. Otherwise, an RAII
     /// guard is returned. The lock will be unlocked when the guard is dropped.
     ///
-    /// This function does not block.
+    /// This function does not block. Attempting to lock a mutex that is already held by the current
+    /// thread will return `Err(TryLockError::WouldBlock)`.
     pub fn try_lock(&self) -> TryLockResult<MutexGuard<'_, T>> {
         match self.inner.lock(NoWait) {
-            Ok(()) => unsafe { Ok(MutexGuard::new(self)) },
+            Ok(()) => {
+                if self.locked.swap(true, Ordering::Acquire) {
+                    self.inner.unlock().unwrap();
+                    Err(TryLockError::WouldBlock)
+                } else {
+                    unsafe { Ok(MutexGuard::new(self)) }
+                }
+            }
             // TODO: It might be better to distinguish these errors, and only return the WouldBlock
             // if that is the corresponding error. But, the lock shouldn't fail in Zephyr.
             Err(_) => Err(TryLockError::WouldBlock),
@@ -163,6 +179,7 @@ impl<T: ?Sized> DerefMut for MutexGuard<'_, T> {
 impl<T: ?Sized> Drop for MutexGuard<'_, T> {
     #[inline]
     fn drop(&mut self) {
+        self.lock.locked.store(false, Ordering::Release);
         if let Err(e) = self.lock.inner.unlock() {
             panic!("Problem unlocking MutexGuard in drop: {:?}", e);
         }
@@ -210,7 +227,9 @@ impl Condvar {
     /// have a boolean predicate associated with them, and the predicate must always be checked
     /// each time this function returns to protect against spurious wakeups.
     pub fn wait<'a, T>(&self, guard: MutexGuard<'a, T>) -> LockResult<MutexGuard<'a, T>> {
+        guard.lock.locked.store(false, Ordering::Release);
         self.inner.wait(&guard.lock.inner);
+        guard.lock.locked.store(true, Ordering::Release);
         Ok(guard)
     }