On Fri, Dec 01, 2023 at 11:33:19AM +0100, Ingo Molnar wrote: > =======================> > From: Jann Horn <jannh@xxxxxxxxxx> > Date: Thu, 30 Nov 2023 21:48:17 +0100 > Subject: [PATCH] locking/mutex: Document that mutex_unlock() is non-atomic > > I have seen several cases of attempts to use mutex_unlock() to release an > object such that the object can then be freed by another task. > > This is not safe because mutex_unlock(), in the > MUTEX_FLAG_WAITERS && !MUTEX_FLAG_HANDOFF case, accesses the mutex > structure after having marked it as unlocked; so mutex_unlock() requires > its caller to ensure that the mutex stays alive until mutex_unlock() > returns. > > If MUTEX_FLAG_WAITERS is set and there are real waiters, those waiters > have to keep the mutex alive, but we could have a spurious > MUTEX_FLAG_WAITERS left if an interruptible/killable waiter bailed > between the points where __mutex_unlock_slowpath() did the cmpxchg > reading the flags and where it acquired the wait_lock. > > ( With spinlocks, that kind of code pattern is allowed and, from what I > remember, used in several places in the kernel. ) > > Document this, such a semantic difference between mutexes and spinlocks > is fairly unintuitive. > > [ mingo: Made the changelog a bit more assertive, refined the comments. ] > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx> > Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx> > Link: https://lore.kernel.org/r/20231130204817.2031407-1-jannh@xxxxxxxxxx > --- > Documentation/locking/mutex-design.rst | 6 ++++++ > kernel/locking/mutex.c | 5 +++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/locking/mutex-design.rst b/Documentation/locking/mutex-design.rst > index 78540cd7f54b..7572339b2f12 100644 > --- a/Documentation/locking/mutex-design.rst > +++ b/Documentation/locking/mutex-design.rst > @@ -101,6 +101,12 @@ features that make lock debugging easier and faster: > - Detects multi-task circular deadlocks and prints out all affected > locks and tasks (and only those tasks). > > +Releasing a mutex is not an atomic operation: Once a mutex release operation > +has begun, another context may be able to acquire the mutex before the release > +operation has fully completed. The mutex user must ensure that the mutex is not > +destroyed while a release operation is still in progress - in other words, > +callers of mutex_unlock() must ensure that the mutex stays alive until > +mutex_unlock() has returned. > > Interfaces > ---------- > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c > index 2deeeca3e71b..cbae8c0b89ab 100644 > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -532,6 +532,11 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne > * This function must not be used in interrupt context. Unlocking > * of a not locked mutex is not allowed. > * > + * The caller must ensure that the mutex stays alive until this function has > + * returned - mutex_unlock() can NOT directly be used to release an object such > + * that another concurrent task can free it. > + * Mutexes are different from spinlocks & refcounts in this aspect. > + * > * This function is similar to (but not equivalent to) up(). > */ > void __sched mutex_unlock(struct mutex *lock) Hi Ingo and Jann, thanks for the patch. The patch LGTM, thanks! Reviewed-by: Bagas Sanjaya <bagasdotme@xxxxxxxxx> -- An old man doll... just what I always wanted! - Clara
Attachment:
signature.asc
Description: PGP signature
