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

[Waiman Long <longman@redhat.com>]

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*
>