[PATCH][BZ #14652] Fix deadlock on PI mutex in pthread_cond_wait cleanup handler

[Jeff Law]

On 10/03/2012 04:44 AM, Siddhesh Poyarekar wrote:
> Hi,
>
> This bug is related in principle to pr#14417.  If a thread is
> cancelled just after it returns successfully from a futex wait for a PI
> mutex and before async cancellation is disabled, it already holds the
> mutex and hence, the cleanup handler gets deadlocked trying to lock a
> mutex it already owns.  The bug report has a reproducer that
> demonstrates the problem.
>
> The attached patch fixes this by checking if the mutex is owned by the
> current thread and if it is, then resume rewinding without attempting
> to lock the mutex.  The patch has a test case that verifies that this
> is fixed.  I have verified this on x86_64 and i686 (32-bit build on
> x86_64).  I have also verified that the test case fails without this
> patch.
>
> The patch applies on top of the patch for pr #14417 that I just
> submitted.  OK to commit once the pr #14417 patch is approved?
>
> Regards,
> Siddhesh
>
> Regards,
> Siddhesh
>
> nptl/ChangeLog:
>
> 	[BZ #14652]
> 	* Makefile (tests): New test case tst-cond25.
> 	(LDFLAGS-tst-cond25): Link tst-cond25 against librt.
> 	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S
> 	(__condvar_tw_cleanup): Lock mutex only if we don't already
> 	own it.
> 	* sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S
> 	(__condvar_w_cleanup): Likewise.
> 	* sysdeps/unix/sysv/linux/pthread-pi-defines.sym: Add TID_MASK.
> 	* sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S
> 	(__condvar_cleanup2): Lock mutex only if we don't already
> 	own it.
> 	* sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S
> 	(__condvar_cleanup1): Likewise.
> 	* tst-cond25.c: New test case.
This looks good to me.  Please install it -- it is almost totally 
independent of the issues Rich has raised as far as I can tell.

Siddhesh is actually fixing the cleanup handlers in code which 
implements cancellation handling by way of exception tables.

+	andl	$(ROBUST_BIT|PI_BIT), %ebx
+	cmpl	$PI_BIT, %ebx
What's the point behind including ROBUST_BIT in the mask here?

+	movl	(%eax), %ebx
+	andl	$TID_MASK, %ebx
+	cmpl	%ebx, %gs:TID
+	je	9f
So when the TID doesn't match don't lock and just bail out?  Is that 
really the right thing to do?

Those two questions span each of the 4 implementations...