Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231130204817.2031407-1-jannh@google.com> <06c05c8b-9a3b-4c04-b898-0f82e98da70f@redhat.com>
 <CAG48ez1a=VuEWwPTjcXFAwCyt9bRH-WzAfw0uP-qVu83kdxkZw@mail.gmail.com> <28b147c3d7354d1a8ff0b903da9b54f4@AcuMS.aculab.com>
In-Reply-To: <28b147c3d7354d1a8ff0b903da9b54f4@AcuMS.aculab.com>
From:   Jann Horn <jannh@google.com>
Date:   Fri, 1 Dec 2023 19:18:02 +0100
Message-ID: <CAG48ez1bOwbqEbD_ycC2fyWK_tW4rqr=gogJbQOx5CECyPGZcg@mail.gmail.com>
Subject: Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
To:     David Laight <David.Laight@aculab.com>
Cc:     Waiman Long <longman@redhat.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-kernel@vger.kernel.org>,
        "linux-doc@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 7:12=E2=80=AFPM David Laight <David.Laight@aculab.co=
m> wrote:
> From: Jann Horn
> > Sent: 01 December 2023 15:02
> >
> > On Fri, Dec 1, 2023 at 1:33=E2=80=AFAM 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 rele=
ase an
> > > > object such that the object can then be freed by another task.
> > > > My understanding is that this is not safe because mutex_unlock(), i=
n the
> > > > MUTEX_FLAG_WAITERS && !MUTEX_FLAG_HANDOFF case, accesses the mutex
> > > > structure after having marked it as unlocked; so mutex_unlock() req=
uires
> > > > 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 wait=
ers
> > > > 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 wha=
t I
> > > > remember, used in several places in the kernel.)
> > > >
> > > > If my understanding of this is correct, we should probably document=
 this -
> > > > I think such a semantic difference between mutexes and spinlocks is=
 fairly
> > > > unintuitive.
> > >
> > > Spinlocks are fair. So doing a lock/unlock sequence will make sure th=
at
> > > all the previously waiting waiters are done with the lock. Para-virtu=
al
> > > spinlocks, however, can be a bit unfair so doing a lock/unlock sequen=
ce
> > > may not be enough to guarantee there is no waiter. The same is true f=
or
> > > mutex. Adding a spin_is_locked() or mutex_is_locked() check can make
> > > sure that all the waiters are gone.
> >
> > I think this pattern anyway only works when you're only trying to wait
> > for the current holder of the lock, not tasks that are queued up on
> > the lock as waiters - so a task initially holds a stable reference to
> > some object, then acquires the object's lock, then drops the original
> > reference, and then later drops the lock.
> > You can see an example of such mutex usage (which is explicitly legal
> > with userspace POSIX mutexes, but is forbidden with kernel mutexes) at
> > the bottom of the POSIX manpage for pthread_mutex_destroy() at
> > <https://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mute=
x_destroy.html>,
> > in the section "Destroying Mutexes".
>
> I don't understand at all what any of this is about.
> You cannot de-initialise, free (etc) a mutex (or any other piece of
> memory for that matter) if another thread can have a reference to it.
> If some other code might be holding the mutex it also might be just
> about to acquire it - you always need another lock of some kind to
> ensure that doesn't happen.
>
> IIRC pretty much the only time you need to acquire the mutex in the
> free path is if locks are chained, eg:
>         lock(table)
>         entry =3D find_entry();
>         lock(entry)
>         unlock(table)
>         ...
>         unlock(entry)
>
> Then the free code has to:
>         lock(table)
>         remove_from_table(entry)
>         lock(entry)
>         unlock(entry)
>         unlock(table)
>         free(entry)

Yep, this is exactly the kind of code pattern for which I'm trying to
document that it is forbidden with mutexes (while it is allowed with
spinlocks).