Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <151f2b05-bcb7-4e69-866a-c76286383175@redhat.com>
Date:   Thu, 30 Nov 2023 18:56:52 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
Content-Language: en-US
To:     Jann Horn <jannh@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org
References: <20231130204817.2031407-1-jannh@google.com>
 <2f17a9a6-5781-43ef-a09b-f39310843fe6@redhat.com>
 <CAG48ez1oXW=4MfQ0A6tthud-cvDZUTA+VB=jzu-HxvWzbj+X0g@mail.gmail.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <CAG48ez1oXW=4MfQ0A6tthud-cvDZUTA+VB=jzu-HxvWzbj+X0g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 11/30/23 17:24, Jann Horn wrote:
> On Thu, Nov 30, 2023 at 10:53 PM Waiman Long <longman@redhat.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