locking/mutex: Clarify that mutex_unlock(), and most other sleeping locks, cannot be used to reference-count objects
Commit Message
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Dec 01, 2023 at 10:44:09AM -0000, tip-bot2 for Jann Horn wrote:
>
> > --- 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
>
> I still object to this confusing usage of atomic. Also all this also
> applies to all sleeping locks, rwsem etc. I don't see why we need to
> special case mutex here.
>
> Also completion_done() has an explicit lock+unlock on wait.lock to
> deal with this there.
Fair enough - but Jan's original observation stands: mutexes are the
sleeping locks most similar to spinlocks, so the locking & object lifetime
pattern that works under spinlocks cannot be carried over to mutexes in all
cases, and it's fair to warn about this pitfall.
We single out mutex_lock(), because they are the most similar in behavior
to spinlocks, and because this concern isn't hypothethical, it has been
observed in the wild with mutex users.
How about the language in the attached patch?
Thanks,
Ingo
================>
From: Ingo Molnar <mingo@kernel.org>
Date: Mon, 8 Jan 2024 09:31:16 +0100
Subject: [PATCH] locking/mutex: Clarify that mutex_unlock(), and most other sleeping locks, cannot be used to reference-count objects
Clarify the mutex_unlock() lock lifetime rules a bit more.
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jann Horn <jannh@google.com>
Link: https://lore.kernel.org/r/20231201121808.GL3818@noisy.programming.kicks-ass.net
---
Documentation/locking/mutex-design.rst | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
Comments
On Mon, Jan 8, 2024 at 9:45 AM Ingo Molnar <mingo@kernel.org> wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Fri, Dec 01, 2023 at 10:44:09AM -0000, tip-bot2 for Jann Horn wrote:
> >
> > > --- 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
> >
> > I still object to this confusing usage of atomic. Also all this also
> > applies to all sleeping locks, rwsem etc. I don't see why we need to
> > special case mutex here.
> >
> > Also completion_done() has an explicit lock+unlock on wait.lock to
> > deal with this there.
>
> Fair enough - but Jan's original observation stands: mutexes are the
> sleeping locks most similar to spinlocks, so the locking & object lifetime
> pattern that works under spinlocks cannot be carried over to mutexes in all
> cases, and it's fair to warn about this pitfall.
>
> We single out mutex_lock(), because they are the most similar in behavior
> to spinlocks, and because this concern isn't hypothethical, it has been
> observed in the wild with mutex users.
>
> How about the language in the attached patch?
In case you missed it, I sent this rewritten documentation patch in
response to the feedback I got, intended to replace the patch that is
now sitting in the tip tree (but I don't know how that works
procedurally for something that's already in the tip tree, whether
you'd want to just swap out the patch with a forced update, or revert
out the old version, or something else):
<https://lore.kernel.org/all/20231204132259.112152-1-jannh@google.com/>
Since there were comments on how this is really a more general rule
than a mutex-specific one, that version doesn't touch
Documentation/locking/mutex-design.rst and instead documents the rule
in Documentation/locking/locktypes.rst; and then it adds comments
above some of the most common unlock-type functions that would be
affected.
@@ -101,12 +101,21 @@ 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.
+A mutex - and most other sleeping locks like rwsems - do not provide an
+implicit refcount for the memory they occupy, which could then be released
+with mutex_unlock().
+
+[ This is in contrast with spin_unlock() [or completion_done()], which APIs can
+ be used to guarantee that the memory is not touched by the lock implementation
+ after spin_unlock() releases the lock. ]
+
+Once a mutex release operation has begun within mutex_unlock(), another context
+may be able to acquire the mutex before the release operation has fully completed,
+and it's not safe to free the object then.
+
+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
----------