Received-SPF: pass (google.com: domain of linux-kernel+bounces-19227-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Mon, 8 Jan 2024 10:00:12 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-tip-commits@vger.kernel.org,
	Jann Horn <jannh@google.com>, x86@kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH -v2] locking/mutex: Clarify that mutex_unlock(), and most
 other sleeping locks, can still use the lock object after it's unlocked
Message-ID: <ZZu5nBicXKwgYrsg@gmail.com>
References: <20231130204817.2031407-1-jannh@google.com>
 <170142744948.398.4203675877225809071.tip-bot2@tip-bot2>
 <20231201121808.GL3818@noisy.programming.kicks-ass.net>
 <ZZu2F8KNygWzWVY7@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZZu2F8KNygWzWVY7@gmail.com>


* Ingo Molnar <mingo@kernel.org> wrote:

> > > +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?

Refined the language a bit more in the -v2 patch below.

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, can still use the lock object after it's unlocked

Clarify the mutex 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>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/20231201121808.GL3818@noisy.programming.kicks-ass.net
---
 Documentation/locking/mutex-design.rst | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/locking/mutex-design.rst b/Documentation/locking/mutex-design.rst
index 7572339b2f12..7c30b4aa5e28 100644
--- a/Documentation/locking/mutex-design.rst
+++ b/Documentation/locking/mutex-design.rst
@@ -101,12 +101,24 @@ 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.
+Mutexes - and most other sleeping locks like rwsems - do not provide an
+implicit reference for the memory they occupy, which reference is 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()/completion_done() releases
+  the lock. ]
+
+mutex_unlock() may access the mutex structure even after it has internally
+released the lock already - so it's not safe for another context to
+acquire the mutex and assume that the mutex_unlock() context is not using
+the structure anymore.
+
+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
 ----------