Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Sat, 2 Dec 2023 08:37:10 +0700
From:   Bagas Sanjaya <bagasdotme@gmail.com>
To:     Ingo Molnar <mingo@kernel.org>, Jann Horn <jannh@google.com>
Cc:     Waiman Long <longman@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Documentation <linux-doc@vger.kernel.org>
Subject: Re: [PATCH -v2] locking/mutex: Document that mutex_unlock() is
 non-atomic
Message-ID: <ZWqKRkhYm7hxUDW_@archie.me>
References: <20231130204817.2031407-1-jannh@google.com>
 <2f17a9a6-5781-43ef-a09b-f39310843fe6@redhat.com>
 <CAG48ez1oXW=4MfQ0A6tthud-cvDZUTA+VB=jzu-HxvWzbj+X0g@mail.gmail.com>
 <ZWm2b+QGpOvzHjc1@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="oz6cg3XNiAwQ353b"
Content-Disposition: inline
In-Reply-To: <ZWm2b+QGpOvzHjc1@gmail.com>
Precedence: bulk


--oz6cg3XNiAwQ353b
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 01, 2023 at 11:33:19AM +0100, Ingo Molnar wrote:
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D>
> From: Jann Horn <jannh@google.com>
> Date: Thu, 30 Nov 2023 21:48:17 +0100
> Subject: [PATCH] locking/mutex: Document that mutex_unlock() is non-atomic
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> ( With spinlocks, that kind of code pattern is allowed and, from what I
>   remember, used in several places in the kernel. )
>=20
> Document this, such a semantic difference between mutexes and spinlocks
> is fairly unintuitive.
>=20
> [ mingo: Made the changelog a bit more assertive, refined the comments. ]
>=20
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Link: https://lore.kernel.org/r/20231130204817.2031407-1-jannh@google.com
> ---
>  Documentation/locking/mutex-design.rst | 6 ++++++
>  kernel/locking/mutex.c                 | 5 +++++
>  2 files changed, 11 insertions(+)
>=20
> diff --git a/Documentation/locking/mutex-design.rst b/Documentation/locki=
ng/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).
> =20
> +Releasing a mutex is not an atomic operation: Once a mutex release opera=
tion
> +has begun, another context may be able to acquire the mutex before the r=
elease
> +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 word=
s,
> +callers of mutex_unlock() must ensure that the mutex stays alive until
> +mutex_unlock() has returned.
> =20
>  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 obje=
ct 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@gmail.com>

--=20
An old man doll... just what I always wanted! - Clara

--oz6cg3XNiAwQ353b
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSSYQ6Cy7oyFNCHrUH2uYlJVVFOowUCZWqKPwAKCRD2uYlJVVFO
o/M0AQCWThPVVQ64FGDrGvqpOy6HkiUdyNm4B2y18fcJPzVeGAD/Yt1nlTW5XE+7
tvPu9yAzTY0HdYy/CVr/UJc7UzICbQU=
=yca7
-----END PGP SIGNATURE-----

--oz6cg3XNiAwQ353b--