Re: BZ 13939 malloc deadlock

[Jeff Law <law at redhat dot com>]

On 07/02/2012 02:00 PM, Maxim Kuvyrkov wrote:
> > +      mstate prev = ar_ptr->next ? ar_ptr : 0; +
> > (void)mutex_unlock(&ar_ptr->mutex); +      ar_ptr =
> > arena_get2(prev, bytes + 2*pagesz + MINSIZE, true);
>
> I still think that introduction of 'prev' is a bit too verbose (it is
> used once and it's initializer would serve the purpose just as well),
> but this is way in the area of nit-picks that I can make my peace
> with it :-).
That was the style used in the libc_memalign version which I found to be 
the cleanest and most robust WRT racing on ar_ptr->next and thus was the 
variant I settled on for all the retry paths.



> > The only thing that prevents us from pulling the mutex_unlock call
> > up is concerns about racing on ar_ptr->next, which is used in the
> > sbrk() failed, retrying via mmap path.
> >
> > I did evaluate the code for races on that object and didn't find
> > any, but this is a new codebase for me and I could have missed
> > something.  If we can conclude that a race on that object isn't an
> > issue, then we can trivially simplify this code.
>
> I don't recommend going this way as it is error-prone for bugs from
> future changes.  A reasonable programmer would expect ar_ptr->mutex
> to lock the entirety of ar_ptr, including ar_ptr->next.
Precisely.  Thus if we're trying to be robust WRT future changes and 
avoiding problems with ar->next changing after the lock is released but 
before we call arena_get2 (which is what it looks like libc_memalign is 
trying to do), then we need to get the value from ar->next before we 
release the lock.

Note that libc_malloc and libc_calloc's current implementations can't 
race on ar->next because the lock is still held.  Of course it's the 
holding of that lock which leads to the deadlock that we're trying to 
fix right now.

libc_pvalloc and libc_valloc's current implementation would have 
problems if there are races with writes to ar->next as they unlock the 
arena then blindly use the current value of ar->next.


Jeff