diff mbox series

[v2,04/21] nptl: x86_64: Fix Race conditions in pthread cancellation (BZ#12683)

Message ID 1519679016-12241-5-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series nptl: Fix Race conditions in pthread cancellation (BZ#12683) | expand

Commit Message

Adhemerval Zanella Feb. 26, 2018, 9:03 p.m. UTC
This patches adds the x86_64 modification required for the BZ#12683.
It basically provide the required ucontext_get_pc symbol and remove
the arch-specific libc-cancellation implementations.

It also remove bogus arch-specific THREAD_ATOMIC_BIT_SET where it
always reference to current thread instead of the one referenced by
input 'descr' argument. It works as long the input is the self thread
pointer, however it creates wrong code is it used along with a
description to a different one (as on nptl/pthread_cancel.c).

The code generated create an additional load to reference to TLS segment,
for instance the code:

  THREAD_ATOMIC_BIT_SET (THREAD_SELF, cancelhandling, CANCELED_BIT);

Compiles to:

  lock;orl $4, %fs:776

Where with patch changes it now compiles to:

  mov %fs:16,%rax
  lock;orl $4, 776(%rax)

If some usage indeed proves to be a hotspot we can add an extra macro
with a more descriptive name (THREAD_ATOMIC_BIT_SET_SELF for instance)
where x86_64 might optimize it.  In fact all x86_64 THREAD_ATOMIC_* macros
do not respect the input descr and possible will fail when used with
a 'descr' difference than THREAD_SELF.

Checked on x86_64-linux-gnu.

	[BZ #12683]
	* sysdeps/unix/sysv/linux/x86_64/cancellation.S: Remove file.
	* sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S: Remove file.
	* sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S: Remove file.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (lll_wait_tid):
	Use cancellable futex wait call.
	* sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h (ucontext_get_pc):
	New function.
	* sysdeps/x86_64/nptl/tcb-offsets.sym (TCB_CANCELING_BITMASK):
	Remove.
	* sysdeps/x86_64/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
	THREAD_ATOMIC_BIT_SET): Remove macros.
---
 ChangeLog                                          |  13 +++
 sysdeps/unix/sysv/linux/x86_64/cancellation.S      | 115 ---------------------
 sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S |  21 ----
 .../unix/sysv/linux/x86_64/librt-cancellation.S    |  21 ----
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h      |   8 +-
 sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h    |  11 ++
 sysdeps/x86_64/nptl/tcb-offsets.sym                |   1 -
 sysdeps/x86_64/nptl/tls.h                          |  11 --
 8 files changed, 28 insertions(+), 173 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/cancellation.S
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S

-- 
2.7.4

Comments

Zack Weinberg May 7, 2018, 2:49 a.m. UTC | #1
On 26 Feb 2018, Adhemerval Zanella wrote:
> This patches adds the x86_64 modification required for the BZ#12683.

> It basically provide the required ucontext_get_pc symbol and remove

> the arch-specific libc-cancellation implementations.


These changes all look fine to me.

>   lock;orl $4, %fs:776

>

> Where with patch changes it now compiles to:

>

>   mov %fs:16,%rax

>   lock;orl $4, 776(%rax)


That might actually be faster on some microarchitectures due to not
having multiple unusual prefixes on one instruction.  I wouldn't worry
about it.

> In fact all x86_64 THREAD_ATOMIC_* macros

> do not respect the input descr and possible will fail when used with

> a 'descr' difference than THREAD_SELF.


Maybe we should correct or remove all of them, then?  They sound like
they're bugs waiting to happen.

zw
Adhemerval Zanella May 8, 2018, 5:55 p.m. UTC | #2
On 06/05/2018 23:49, Zack Weinberg wrote:
> On 26 Feb 2018, Adhemerval Zanella wrote:

>> This patches adds the x86_64 modification required for the BZ#12683.

>> It basically provide the required ucontext_get_pc symbol and remove

>> the arch-specific libc-cancellation implementations.

> 

> These changes all look fine to me.

> 

>>   lock;orl $4, %fs:776

>>

>> Where with patch changes it now compiles to:

>>

>>   mov %fs:16,%rax

>>   lock;orl $4, 776(%rax)

> 

> That might actually be faster on some microarchitectures due to not

> having multiple unusual prefixes on one instruction.  I wouldn't worry

> about it.

> 

>> In fact all x86_64 THREAD_ATOMIC_* macros

>> do not respect the input descr and possible will fail when used with

>> a 'descr' difference than THREAD_SELF.

> 

> Maybe we should correct or remove all of them, then?  They sound like

> they're bugs waiting to happen.

> 

> zw

> 


Right, I will fix in on an extra patch then.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/x86_64/cancellation.S b/sysdeps/unix/sysv/linux/x86_64/cancellation.S
deleted file mode 100644
index f3fb19d..0000000
--- a/sysdeps/unix/sysv/linux/x86_64/cancellation.S
+++ /dev/null
@@ -1,115 +0,0 @@ 
-/* Copyright (C) 2009-2018 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sysdep.h>
-#include <tcb-offsets.h>
-#include <kernel-features.h>
-#include "lowlevellock.h"
-
-#define PTHREAD_UNWIND JUMPTARGET(__pthread_unwind)
-#if IS_IN (libpthread)
-# if defined SHARED && !defined NO_HIDDEN
-#  undef PTHREAD_UNWIND
-#  define PTHREAD_UNWIND __GI___pthread_unwind
-# endif
-#else
-# ifndef SHARED
-	.weak __pthread_unwind
-# endif
-#endif
-
-
-#ifdef __ASSUME_PRIVATE_FUTEX
-# define LOAD_PRIVATE_FUTEX_WAIT(reg) \
-	movl	$(FUTEX_WAIT | FUTEX_PRIVATE_FLAG), reg
-#else
-# if FUTEX_WAIT == 0
-#  define LOAD_PRIVATE_FUTEX_WAIT(reg) \
-	movl	%fs:PRIVATE_FUTEX, reg
-# else
-#  define LOAD_PRIVATE_FUTEX_WAIT(reg) \
-	movl	%fs:PRIVATE_FUTEX, reg ; \
-	orl	$FUTEX_WAIT, reg
-# endif
-#endif
-
-/* It is crucial that the functions in this file don't modify registers
-   other than %rax and %r11.  The syscall wrapper code depends on this
-   because it doesn't explicitly save the other registers which hold
-   relevant values.  */
-	.text
-
-	.hidden __pthread_enable_asynccancel
-ENTRY(__pthread_enable_asynccancel)
-	movl	%fs:CANCELHANDLING, %eax
-2:	movl	%eax, %r11d
-	orl	$TCB_CANCELTYPE_BITMASK, %r11d
-	cmpl	%eax, %r11d
-	je	1f
-
-	lock
-	cmpxchgl %r11d, %fs:CANCELHANDLING
-	jnz	2b
-
-	andl	$(TCB_CANCELSTATE_BITMASK|TCB_CANCELTYPE_BITMASK|TCB_CANCELED_BITMASK|TCB_EXITING_BITMASK|TCB_CANCEL_RESTMASK|TCB_TERMINATED_BITMASK), %r11d
-	cmpl	$(TCB_CANCELTYPE_BITMASK|TCB_CANCELED_BITMASK), %r11d
-	je	3f
-
-1:	ret
-
-3:	subq	$8, %rsp
-	cfi_adjust_cfa_offset(8)
-	LP_OP(mov) $TCB_PTHREAD_CANCELED, %fs:RESULT
-	lock
-	orl	$TCB_EXITING_BITMASK, %fs:CANCELHANDLING
-	mov	%fs:CLEANUP_JMP_BUF, %RDI_LP
-	call	PTHREAD_UNWIND
-	hlt
-END(__pthread_enable_asynccancel)
-
-
-	.hidden __pthread_disable_asynccancel
-ENTRY(__pthread_disable_asynccancel)
-	testl	$TCB_CANCELTYPE_BITMASK, %edi
-	jnz	1f
-
-	movl	%fs:CANCELHANDLING, %eax
-2:	movl	%eax, %r11d
-	andl	$~TCB_CANCELTYPE_BITMASK, %r11d
-	lock
-	cmpxchgl %r11d, %fs:CANCELHANDLING
-	jnz	2b
-
-	movl	%r11d, %eax
-3:	andl	$(TCB_CANCELING_BITMASK|TCB_CANCELED_BITMASK), %eax
-	cmpl	$TCB_CANCELING_BITMASK, %eax
-	je	4f
-1:	ret
-
-	/* Performance doesn't matter in this loop.  We will
-	   delay until the thread is canceled.  And we will unlikely
-	   enter the loop twice.  */
-4:	mov	%fs:0, %RDI_LP
-	movl	$__NR_futex, %eax
-	xorq	%r10, %r10
-	addq	$CANCELHANDLING, %rdi
-	LOAD_PRIVATE_FUTEX_WAIT (%esi)
-	syscall
-	movl	%fs:CANCELHANDLING, %eax
-	jmp	3b
-END(__pthread_disable_asynccancel)
diff --git a/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S b/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
deleted file mode 100644
index ed7af0d..0000000
--- a/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* Copyright (C) 2009-2018 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#define __pthread_enable_asynccancel __libc_enable_asynccancel
-#define __pthread_disable_asynccancel __libc_disable_asynccancel
-#include "cancellation.S"
diff --git a/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S b/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
deleted file mode 100644
index d0f0ee4..0000000
--- a/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* Copyright (C) 2009-2018 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2009.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#define __pthread_enable_asynccancel __librt_enable_asynccancel
-#define __pthread_disable_asynccancel __librt_disable_asynccancel
-#include "cancellation.S"
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index eedb6fc..957d4c1 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -231,10 +231,10 @@  extern int __lll_timedlock_elision (int *futex, short *adapt_count,
    afterwards.  The kernel up to version 3.16.3 does not use the private futex
    operations for futex wake-up when the clone terminates.  */
 #define lll_wait_tid(tid) \
-  do {					\
-    __typeof (tid) __tid;		\
-    while ((__tid = (tid)) != 0)	\
-      lll_futex_wait (&(tid), __tid, LLL_SHARED);\
+  do {									      \
+    __typeof (tid) __tid;						      \
+    while ((__tid = (tid)) != 0)					      \
+      lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		      \
   } while (0)
 
 extern int __lll_timedwait_tid (int *, const struct timespec *)
diff --git a/sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h b/sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h
index 41ace60..3cc7336 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h
+++ b/sysdeps/unix/sysv/linux/x86_64/sigcontextinfo.h
@@ -15,6 +15,9 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#ifndef _SIGCONTEXTINFO_H
+#define _SIGCONTEXTINFO_H
+
 #include <stdint.h>
 
 #define SIGCONTEXT siginfo_t *_si, ucontext_t *
@@ -28,3 +31,11 @@ 
 
 #define CALL_SIGHANDLER(handler, signo, ctx) \
   (handler)((signo), SIGCONTEXT_EXTRA_ARGS (ctx))
+
+static inline
+uintptr_t ucontext_get_pc (const ucontext_t *uc)
+{
+  return uc->uc_mcontext.gregs[REG_RIP];
+}
+
+#endif
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index 8a25c48..b225e5b 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -19,7 +19,6 @@  PRIVATE_FUTEX		offsetof (tcbhead_t, private_futex)
 -- Not strictly offsets, but these values are also used in the TCB.
 TCB_CANCELSTATE_BITMASK	 CANCELSTATE_BITMASK
 TCB_CANCELTYPE_BITMASK	 CANCELTYPE_BITMASK
-TCB_CANCELING_BITMASK	 CANCELING_BITMASK
 TCB_CANCELED_BITMASK	 CANCELED_BITMASK
 TCB_EXITING_BITMASK	 EXITING_BITMASK
 TCB_CANCEL_RESTMASK	 CANCEL_RESTMASK
diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index bdd0237..ed0c65b 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -315,17 +315,6 @@  typedef struct
 	      abort (); })
 
 
-/* Atomic set bit.  */
-# define THREAD_ATOMIC_BIT_SET(descr, member, bit) \
-  (void) ({ if (sizeof ((descr)->member) == 4)				      \
-	      asm volatile (LOCK_PREFIX "orl %1, %%fs:%P0"		      \
-			    :: "i" (offsetof (struct pthread, member)),	      \
-			       "ir" (1 << (bit)));			      \
-	    else							      \
-	      /* Not necessary for other sizes in the moment.  */	      \
-	      abort (); })
-
-
 /* Set the stack guard field in TCB head.  */
 # define THREAD_SET_STACK_GUARD(value) \
     THREAD_SETMEM (THREAD_SELF, header.stack_guard, value)