On Fri, Dec 1, 2023 at 7:30 PM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Jann Horn
> > Sent: 01 December 2023 16:41
> >
> > mutex_unlock() has a different API contract compared to spin_unlock().
> > spin_unlock() can be used to release ownership of an object, so that
> > as soon as the spinlock is unlocked, another task is allowed to free
> > the object containing the spinlock.
> > mutex_unlock() does not support this kind of usage: The caller of
> > mutex_unlock() must ensure that the mutex stays alive until
> > mutex_unlock() has returned.
>
> The problem sequence might be:
> Thread A Thread B
> mutex_lock()
> code to stop mutex being requested
> ...
> mutex_lock() - sleeps
> mutex_unlock()...
> Waiters woken...
> isr and/or pre-empted
> - wakes up
> mutex_unlock()
> free()
> ... more kernel code access the mutex
> BOOOM
>
> What happens in a PREEMPT_RT kernel where most of the spin_unlock()
> get replaced by mutex_unlock().
> Seems like they can potentially access a freed mutex?
RT spinlocks don't use mutexes, they use rtmutexes, and I think those
explicitly support this usecase. See the call path:
spin_unlock -> rt_spin_unlock -> rt_mutex_slowunlock
rt_mutex_slowunlock() has a comment, added in commit 27e35715df54
("rtmutex: Plug slow unlock race"):
* We must be careful here if the fast path is enabled. If we
* have no waiters queued we cannot set owner to NULL here
* because of:
*
* foo->lock->owner = NULL;
* rtmutex_lock(foo->lock); <- fast path
* free = atomic_dec_and_test(foo->refcnt);
* rtmutex_unlock(foo->lock); <- fast path
* if (free)
* kfree(foo);
* raw_spin_unlock(foo->lock->wait_lock);
That commit also explicitly refers to wanting to support this pattern
with spin_unlock() in the commit message.
