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

[Waiman Long]

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@xxxxxxxxxx>
> *Sent:* 01 December 2023 18:40
> *To:* Jann Horn <jannh@xxxxxxxxxx>; David Laight <David.Laight@xxxxxxxxxx>
> *Cc:* Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar 
> <mingo@xxxxxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Jonathan Corbet 
> <corbet@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> *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@xxxxxxxxxx>  <mailto:David.Laight@xxxxxxxxxx>  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*