Soundness of CriticalSectionDefaultMutex implementation

[peterkrull]

I am currently experiencing some deadlocks with the WaitQueue when using the cs-backed mutex in a single-core system with interrupts. The stack trace seems to be stuck at the SpinLock inside the CriticalSectionDefaultMutex, which in my mind is a place the cs-mutex should never be on a single-core system.

I looked at the code:

https://github.com/hawkw/mycelium/blob/main/maitake-sync/src/blocking/default_mutex.rs#L294-L326

Specifically the ScopedRawMutex trait function

fn with_lock<R>(&self, f: impl FnOnce() -> R) -> R {
    self.0.with_lock(|| critical_section::with(|_cs| f()))
}

with the SpinLock::with_lock being

fn with_lock<R>(&self, f: impl FnOnce() -> R) -> R {
    self.lock();
    // Using a drop guard ensures that the mutex is unlocked when this
    // function exits, even if `f()` panics.
    let _unlock = Unlock(self);
    f()
}

Is it not incorrect to first lock the spin-lock, and then enter the critical section after? If an interrupt fires between locking the SpinLock and entering the critical section with f() the interrupt context would just get stuck in the spin loop trying to lock.

[hawkw]

Agh, I think you're right; we need the critical_section::with to be outside of self.0.with_lock rather than inside it. Should be an easy fix, at least. If you're interested in submitting a patch, I'll happily merge it and publish a point release, otherwise I can go fix it.

[peterkrull]

Will do, just a moment

[peterkrull]

Just a gentle bump :) Also for #515

[hawkw]

Alright, published maitake-sync v0.2.1 with the fix for this, thanks for your help!