[1/2] qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK

PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork.
The way to safely do fork is to bring all threads to a quiescent
state by acquiring locks (either in callers---as we do for the
iothread mutex---or using pthread_atfork's prepare callbacks)
and then release them in the child.

The problem is that releasing error-checking locks in the child
fails under glibc with EPERM, because the mutex stores a different
owner tid than the duplicated thread in the child process.  We
could make it work for locks acquired via pthread_atfork, by
recreating the mutex in the child instead of unlocking it
(we know that there are no other threads that could have taken
the mutex; but when the lock is acquired in fork's caller
that would not be possible.

The simplest solution is just to forgo error checking.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-thread-posix.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

On 03/06/2015 02:22 AM, Paolo Bonzini wrote:
> PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork.
> The way to safely do fork is to bring all threads to a quiescent
> state by acquiring locks (either in callers---as we do for the
> iothread mutex---or using pthread_atfork's prepare callbacks)
> and then release them in the child.

That, and POSIX itself says that pthread_atfork is a dangerous API, and
should not be used if at all possible, because it is broken by design.

> 
> The problem is that releasing error-checking locks in the child
> fails under glibc with EPERM, because the mutex stores a different
> owner tid than the duplicated thread in the child process.

Is that a bug in glibc?

>  We
> could make it work for locks acquired via pthread_atfork, by
> recreating the mutex in the child instead of unlocking it
> (we know that there are no other threads that could have taken
> the mutex; but when the lock is acquired in fork's caller
> that would not be possible.
> 
> The simplest solution is just to forgo error checking.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-thread-posix.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

I'm not sure that weakening things is always the wisest idea, but you've
provided some good arguments for why we want it here, so:

Reviewed-by: Eric Blake <eblake@redhat.com>


> 
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 50a29d8..ba67cec 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -51,12 +51,8 @@ static void error_exit(int err, const char *msg)
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
>      int err;
> -    pthread_mutexattr_t mutexattr;
>  
> -    pthread_mutexattr_init(&mutexattr);
> -    pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK);
> -    err = pthread_mutex_init(&mutex->lock, &mutexattr);
> -    pthread_mutexattr_destroy(&mutexattr);
> +    err = pthread_mutex_init(&mutex->lock, NULL);
>      if (err)
>          error_exit(err, __func__);
>  }
>

On 06/03/2015 16:54, Eric Blake wrote:
> > The problem is that releasing error-checking locks in the child
> > fails under glibc with EPERM, because the mutex stores a different
> > owner tid than the duplicated thread in the child process.
> 
> Is that a bug in glibc?

Possibly, but I wouldn't be surprised if other libcs had the same bug.
And if you ran it through the Austin Group, I wouldn't be surprised if
it were declared undefined.

Paolo