Re: [PATCH] locking: Document that mutex_unlock() is non-atomic

Waiman Long
2023-12-01T19:15:58Z

On 12/1/23 13:44, David Laight wrote:
>
> (Top post due to perverted outluck rules on html)
>
> Pending waiters aren't the problem.
>
Pending waiters can still be a problem if code decides to free the lock
containing object after a lock/unlock sequence as it may cause
use-after-free.
>
> You have to ensure there aren't any, but the mutex() can be held.
>
Using reference count to track the number of active users is one way to
prevent that if you only release the reference count after
mutex_unlock() returns but not in the lock critical section.

Cheers,
Longman

> David
>
> *From:*Waiman Long <longman@redhat.com>
> *Sent:* 01 December 2023 18:40
> *To:* Jann Horn <jannh@google.com>; David Laight <David.Laight@ACULAB.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
> *Subject:* Re: [PATCH] locking: Document that mutex_unlock() is non-atomic
>
> On 12/1/23 13:18, Jann Horn wrote:
>
> On Fri, Dec 1, 2023 at 7:12 PM David Laight<David.Laight@aculab.com> <mailto:David.Laight@aculab.com> wrote:
>
> From: Jann Horn
>
> 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_mutex_destroy.html> <https://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_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 = 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).
>
> Actually, even spinlocks may not guarantee the lock/unlock sequence
> will flush out all the pending waiters in the case of paravirt spinlocks.
>
> Cheers,
> Longman
>
>
>
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
> P *Please consider the environment and don't print this e-mail unless
> you really need to*
>