Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date:   Fri, 1 Dec 2023 11:20:50 +0100
From:   Ingo Molnar <mingo@kernel.org>
To:     Waiman Long <longman@redhat.com>
Cc:     Jann Horn <jannh@google.com>,
        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
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
Message-ID: <ZWmzgqcvMIvYvWw1@gmail.com>
References: <20231130204817.2031407-1-jannh@google.com>
 <2f17a9a6-5781-43ef-a09b-f39310843fe6@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2f17a9a6-5781-43ef-a09b-f39310843fe6@redhat.com>
Precedence: bulk


* 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?
> 
> mutex_is_locked() will return true if the mutex has waiter even if it? is
> currently free.

I believe the correct condition is what the changelog already says:

  "until mutex_unlock() returns".

What happens within mutex_unlock() is kernel implementation specific and 
once a caller has called mutex_unlock(), the mutex must remain alive until 
it returns. No other call can substitute for this: neither 
mutex_is_locked(), nor some sort of mutex_lock()+mutex_unlock() sequence.

Thanks,

	Ingo