Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231130204817.2031407-1-jannh@google.com> <20231201091007.GG3818@noisy.programming.kicks-ass.net>
In-Reply-To: <20231201091007.GG3818@noisy.programming.kicks-ass.net>
From:   Jann Horn <jannh@google.com>
Date:   Fri, 1 Dec 2023 16:58:04 +0100
Message-ID: <CAG48ez3oqU+P7NJ3Lj1qVKuqgDDdNqsaXdQVR5KaRx5J+BJGJg@mail.gmail.com>
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Waiman Long <longman@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Dec 1, 2023 at 10:10=E2=80=AFAM Peter Zijlstra <peterz@infradead.or=
g> wrote:
> On Thu, Nov 30, 2023 at 09:48:17PM +0100, 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 th=
e
> > MUTEX_FLAG_WAITERS && !MUTEX_FLAG_HANDOFF case, accesses the mutex
> > structure after having marked it as unlocked; so mutex_unlock() require=
s
> > 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.
> >
> > (With spinlocks, that kind of code pattern is allowed and, from what I
> > remember, used in several places in the kernel.)
> >
> > If my understanding of this is correct, we should probably document thi=
s -
> > I think such a semantic difference between mutexes and spinlocks is fai=
rly
> > unintuitive.
>
> IIRC this is true of all sleeping locks, and I think completion was the
> explcicit exception here, but it's been a while.

In addition to completions, I think this also applies to up()? But I
don't know if that's intentionally supported or just an implementation
detail.

Is there some central place where this should be documented instead of
Documentation/locking/mutex-design.rst as a more general kernel
locking design thing? Maybe Documentation/locking/locktypes.rst?

I think it should also be documented on top of the relevant locking
function(s) though, since I don't think everyone who uses locking
functions necessarily reads the separate documentation files first.
Mutexes kind of stand out as the most common locking type, but I guess
to be consistent, we'd have to put the same comment on functions like
up_read() and up_write()? And maybe drop the "Mutexes are different
from spinlocks in this aspect" part?

(Sidenote: Someone pointed out to me that an additional source of
confusion could be that userspace POSIX mutexes support this usage
pattern.)

> > index 78540cd7f54b..087716bfa7b2 100644
> > --- a/Documentation/locking/mutex-design.rst
> > +++ b/Documentation/locking/mutex-design.rst
> > @@ -101,6 +101,12 @@ features that make lock debugging easier and faste=
r:
> >      - Detects multi-task circular deadlocks and prints out all affecte=
d
> >        locks and tasks (and only those tasks).
> >
> > +Releasing a mutex is not an atomic operation: Once a mutex release ope=
ration
>
> Well, it very much is an atomic store-release. That is, I object to your
> confusing use of atomic here :-)

I'd say it involves an atomic store-release, but the whole operation
is not atomic. :P

But yeah, I see how this is confusing wording, and I'm not
particularly attached to my specific choice of words.