linux-kernel - Re: [PATCH] locking: 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]

Message-ID: <151f2b05-bcb7-4e69-866a-c76286383175@redhat.com>
Date:   Thu, 30 Nov 2023 18:56:52 -0500
From:   Waiman Long <longman@...hat.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic


On 11/30/23 17:24, Jann Horn wrote:
> On Thu, Nov 30, 2023 at 10:53 PM Waiman Long <longman@...hat.com> wrote:
>> On 11/30/23 15:48, Jann Horn wrote:
>>> 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.
>>> My understanding is that 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, I think; 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.
>> Could you clarify under what condition a concurrent task can decide to
>> free the object holding the mutex? Is it !mutex_is_locked() or after a
>> mutex_lock()/mutex_unlock sequence?
> I mean a mutex_lock()+mutex_unlock() sequence.
Because of optimistic spinning, a mutex_lock()/mutex_unlock() can 
succeed even if there are still waiters waiting for the lock.
>
>> mutex_is_locked() will return true if the mutex has waiter even if it
>> is currently free.
> I don't understand your point, and maybe I also don't understand what
> you mean by "free". Isn't mutex_is_locked() defined such that it only
> looks at whether a mutex has an owner, and doesn't look at the waiter
> list?

What I mean is that the mutex is in an unlocked state ready to be 
acquired by another locker. mutex_is_locked() considers the state of the 
mutex as locked if any of the owner flags is set.

Beside the mutex_lock()/mutex_unlock() sequence, I will suggest adding a 
mutex_is_locked() check just to be sure.

Cheers,
Longman