This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BZ #14652] Fix deadlock on PI mutex in pthread_cond_waitcleanup handler


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...



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]