Re: [PATCH][BZ 13690] Do not violate mutex destruction requirements.

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2015-07-21 at 19:14 +0200, OndÅej BÃlka wrote:
> On Tue, Jul 21, 2015 at 06:43:47PM +0200, Torvald Riegel wrote:
> > On Tue, 2015-07-21 at 14:18 +0200, OndÅej BÃlka wrote:
> > > On Tue, Jul 14, 2015 at 10:21:13PM +0200, Torvald Riegel wrote:
> > > > POSIX and C++11 require that a thread can destroy a mutex if no thread
> > > > owns the mutex, is blocked on the mutex, or will try to acquire it in
> > > > the future.  After destroying the mutex, it can reuse or unmap the
> > > > underlying memory.  Thus, we must not access a mutex' memory after
> > > > releasing it.  Currently, we can load the private flag after releasing
> > > > the mutex, which is fixed by this patch.
> > > > 
> > > > See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more
> > > > background.
> > > >     
> > > > We need to call futex_wake on the lock after releasing it, however.
> > > > This is by design, and can lead to spurious wake-ups on unrelated futex
> > > > words (e.g., when the mutex memory is reused for another mutex).  This
> > > > behavior is documented in the glibc-internal futex API and in recent
> > > > drafts of the Linux kernel's futex documentation (see the draft_futex
> > > > branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
> > > > 
> > > > Carlos, do you want to consider this for 2.22, or should this rather
> > > > target 2.23?  The actual change is simple.
> > > > 
> > > > Not tested.  Could someone test this on a non-x86-linux machine (I don't
> > > > have one handy).  Dave, could you test on sparc, please?
> > > > 
> > > > 
> > > > 2015-07-14  Torvald Riegel  <triegel@redhat.com>
> > > > 
> > > > 	[BZ #13690]
> > > > 	* sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock
> > > > 	after releasing it.
> > > > 	(__lll_robust_unlock): Likewise.
> > > > 	* nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise.
> > > > 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise.
> > > > 	(lll_robust_unlock): Likewise.
> > > >
> > > 
> > > So this is equivalent of my patch from 2013
> > 
> > I don't remember such a patch, and you didn't gave a citation.
> > 
> > I thought we had fixed this already, saw that we didn't when browsing
> > through the concurrency-related bugs, and thus proposed the patch.
> > 
> > > where you said that you need
> > > to ask posix with fixed bitrot?
> > 
> > I don't understand this, sorry.
> > 
> As for bitrot my patch isn't valid as we meanwhile removed duplicate
> lowlevellock.h

It also seems to redefine lll_unlock, and it doesn't cover all the
problematic cases (see my patch, there are more than in lll_unlock).

I also didn't remember that you had posted a patch about that, and you
didn't follow up on it.  

> Then you said here following:
> 
> https://sourceware.org/ml/libc-ports/2013-12/msg00029.html
> 
> 
> This needs clarification in POSIX; see my comment on #13690
> (https://sourceware.org/bugzilla/show_bug.cgi?id=13690#c24).  Depending
> on how the Austin Group decides, this is either not a bug, or we have to
> fix the pending load AND investigate whether the pending futex_wake call
> is harmless.  The latter might be the case for normal mutexes, but I
> wouldn't be surprised to find out that PI or robust mutexes aren't as
> simple and need a more complex fix.
> 
> Therefore, I think we should wait for the POSIX clarification and then
> decide what the next steps should be.
> 
> 

POSIX has clarified (http://austingroupbugs.net/view.php?id=811) and we
have resolved the futex spurious wake-up issue (or, agreed on a status
quo with the kernel folks).  Thus, those things are settled and what
remained was to fix the order of the loads from private together and the
actual unlock atomic operation.