[lxc-devel] [PATCH] Use pthread_atfork() to unlock mutexes after fork()

[Andrey Mazo]

On Wed, 01 Jan 2014 23:52:33 +0400, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> Quoting Andrey Mazo (mazo at telum.ru):
[snip]
>> diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
>> index 28691b0..36c4bd3 100644
>> --- a/src/lxc/lxclock.c
>> +++ b/src/lxc/lxclock.c
>> @@ -87,7 +87,7 @@ void unlock_mutex(pthread_mutex_t *l)
>>  	int ret;
>>
>>  	if ((ret = pthread_mutex_unlock(l)) != 0) {
>> -		fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, strerror(ret));
>> +		fprintf(stderr, "pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
>>  		dump_stacktrace();
>>  		exit(1);
>>  	}
>> @@ -315,6 +315,21 @@ void process_unlock(void)
>>  	unlock_mutex(&thread_mutex);
>>  }
>>
>> +/* One thread can do fork() while another one is holding a mutex.
>> + * There is only one thread in child just after the fork(), so noone will ever release that mutex.
>> + * We setup a "child" fork handler to unlock the mutex just after the fork().
>> + * For several mutex types, unlocking an unlocked mutex can lead to undefined behavior.
>
> Thanks for pointing this out.  I have to say I don't like that we have
> to lock this...  though it should be safe with our current usage (and
> I see Caglar has verified it :)
I don't like the implicit restriction on fork() inside process_lock() either, but,
as far as I can see, it's the most reliable/portable approach (across mutex types).

> An alternative would be to have a unlock_if_locked() for each lock,
> and do pthread_atfork(NULL, NULL, process_unlock_if_locked()).
> There would be no risk of a race since the child is single-threaded.
This is a good alternative.
Another way is to trylock() the mutex before unlocking it.
But I've decided not to go this way because of the following note for PTHREAD_MUTEX_DEFAULT in pthread_mutex_lock(3P):
"Attempting to unlock the mutex if it was not locked by the calling thread results in undefined behavior."

> But this is ok for now.
>
> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
>
> (Note I'm acking, but not applying until tomorrow)
Thank you!

>> + * One way to deal with it is to setup "prepare" fork handler
>> + * to lock the mutex before fork() and both "parent" and "child" fork handlers
>> + * to unlock the mutex.
>> + * This forbids doing fork() while explicitly holding the lock.
>> + */
>> +__attribute__((constructor))
>> +static void process_lock_setup_atfork(void)
>> +{
>> +	pthread_atfork(process_lock, process_unlock, process_unlock);
>> +}
>> +
>>  /* Protects static const values inside the lxc_global_config_value funtion */
>>  void static_lock(void)
>>  {
[snip]

-- 
Andrey Mazo.