Received-SPF: pass (google.com: domain of linux-kernel+bounces-19203-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Mon, 8 Jan 2024 09:45:11 +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] locking/mutex: Clarify that mutex_unlock(), and most other
 sleeping locks, cannot be used to reference-count objects
Message-ID: <ZZu2F8KNygWzWVY7@gmail.com>
References: <20231130204817.2031407-1-jannh@google.com>
 <170142744948.398.4203675877225809071.tip-bot2@tip-bot2>
 <20231201121808.GL3818@noisy.programming.kicks-ass.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231201121808.GL3818@noisy.programming.kicks-ass.net>


* 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(-)

diff --git a/Documentation/locking/mutex-design.rst b/Documentation/locking/mutex-design.rst
index 7572339b2f12..f5270323cf0b 100644
--- a/Documentation/locking/mutex-design.rst
+++ b/Documentation/locking/mutex-design.rst
@@ -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
 ----------