linux-kernel - Re: [PATCH -v2] locking/mutex: Document that mutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 2 Dec 2023 08:37:10 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Ingo Molnar <mingo@...nel.org>, Jann Horn <jannh@...gle.com>
Cc:     Waiman Long <longman@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Documentation <linux-doc@...r.kernel.org>
Subject: Re: [PATCH -v2] locking/mutex: Document that mutex_unlock() is
 non-atomic

On Fri, Dec 01, 2023 at 11:33:19AM +0100, Ingo Molnar wrote:
> =======================>
> From: Jann Horn <jannh@...gle.com>
> 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@...gle.com>
> Signed-off-by: Ingo Molnar <mingo@...nel.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(+)
> 
> 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@...il.com>

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)