Re: [PATCH v2] malloc: Fix list_lock/arena lock deadlock [BZ #19182]

["Carlos O'Donell" <carlos at redhat dot com>]

Florian,

One quibble.

On 12/18/2015 08:37 AM, Florian Weimer wrote:
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 73bda84..85f1194 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -68,15 +68,28 @@ extern int sanity_check_heap_info_alignment[(sizeof (heap_info)
>  
>  static __thread mstate thread_arena attribute_tls_model_ie;
>  
> -/* Arena free list.  list_lock protects the free_list variable below,
> -   and the next_free and attached_threads members of the mstate
> -   objects.  No other (malloc) locks must be taken while list_lock is
> -   active, otherwise deadlocks may occur.  */
> +/* Arena free list.  free_list_lock synchronizes access to the
> +   free_list variable below, and the next_free and attached_threads
> +   members of struct malloc_state objects.  No other locks must be
> +   acquired after free_list_lock has been acquired.  */
>  
> -static mutex_t list_lock = _LIBC_LOCK_INITIALIZER;
> +static mutex_t free_list_lock = _LIBC_LOCK_INITIALIZER;
>  static size_t narenas = 1;
>  static mstate free_list;
>  
> +/* list_lock prevents concurrent writes to the next member of struct
> +   malloc_state objects.
> +
> +   Read access to the next member is supposed to synchronize with the
> +   atomic_write_barrier and the write to the next member in
> +   _int_new_arena.  This suffers from data races; see the FIXME
> +   comments in _int_new_arena and reused_arena.
> +
> +   list_lock also prevents concurrent forks.  When list_lock is
> +   acquired, no arena lock must be acquired, but it is permitted to
> +   acquire arena locks after list_lock.  */

This last sentence seems ambiguous to me. I assume you mean to say that
at the point at which list_lock is acquired there are no other arena
locks held, but that after list_lock is acquired, other arena locks may
be acquired afterwards?

In which case the sentence should end with ".. arena locks after list_lock
is acquired." which clarifies the sentence a bit more.

Cheers,
Carlos.