diff mbox series

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

Message ID 20191014205656.29834-4-adhemerval.zanella@linaro.org
State New
Headers show
Series nptl: Fix Race conditions in pthread cancellation (BZ#12683) | expand

Commit Message

Adhemerval Zanella Oct. 14, 2019, 8:56 p.m. UTC 
This patches adds the x86_64 modification required for the BZ#12683.
The arch-specific {libc-,librt-}cancelation.S files are remove in favor
of the default libc-cancellation implementation, the TCB_CANCELING_BITMASK
is remove since it is now unused, and the arch-specific syscall_cancel
is provided.

Checked on x86_64-linux-gnu.
---
 sysdeps/unix/sysv/linux/x86_64/cancellation.S | 104 ------------------
 .../sysv/linux/x86_64/libc-cancellation.S     |  21 ----
 .../sysv/linux/x86_64/librt-cancellation.S    |  21 ----
 .../unix/sysv/linux/x86_64/syscall_cancel.S   |  61 ++++++++++
 sysdeps/x86_64/nptl/tcb-offsets.sym           |   1 -
 5 files changed, 61 insertions(+), 147 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
 create mode 100644 sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S

Comments

Florian Weimer Oct. 15, 2019, 11:03 a.m. UTC | #1 
* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S
> new file mode 100644
> index 0000000000..c7364bfff8
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S

> +ENTRY (__syscall_cancel_arch)
> +
> +	.globl __syscall_cancel_arch_start
> +	.type  __syscall_cancel_arch_start,@function

I don't think we should specify a type for this symbol.  It's not a
function.

> +__syscall_cancel_arch_start:
> +
> +	/* if (*cancelhandling & CANCELED_BITMASK)
> +	     __syscall_do_cancel()  */
> +	mov    (%rdi),%eax
> +	testb  $4, (%rdi)
> +	jne    __syscall_do_cancel

I'm pretty sure 4 should be one of the _BITMASK constants.

> +	/* Issue a 6 argument syscall, the nr [%rax] being the syscall
> +	   number.  */
> +	mov    %rdi,%r11
> +	mov    %rsi,%rax
> +	mov    %rdx,%rdi
> +	mov    %rcx,%rsi
> +	mov    %r8,%rdx
> +	mov    %r9,%r10
> +	mov    8(%rsp),%r8
> +	mov    16(%rsp),%r9
> +	mov    %r11,8(%rsp)
> +	syscall
> +
> +	.globl __syscall_cancel_arch_end
> +	.type  __syscall_cancel_arch_end,@function

Again, no type here please.  It will confuse the disassembler.

Thanks,
Florian
Adhemerval Zanella Oct. 16, 2019, 9:22 p.m. UTC | #2 
On 15/10/2019 08:03, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S
>> new file mode 100644
>> index 0000000000..c7364bfff8
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S
> 
>> +ENTRY (__syscall_cancel_arch)
>> +
>> +	.globl __syscall_cancel_arch_start
>> +	.type  __syscall_cancel_arch_start,@function
> 
> I don't think we should specify a type for this symbol.  It's not a
> function.

Ack, I can't recall exactly I have added it on previous iterations.
I removed it.

> 
>> +__syscall_cancel_arch_start:
>> +
>> +	/* if (*cancelhandling & CANCELED_BITMASK)
>> +	     __syscall_do_cancel()  */
>> +	mov    (%rdi),%eax
>> +	testb  $4, (%rdi)
>> +	jne    __syscall_do_cancel
> 
> I'm pretty sure 4 should be one of the _BITMASK constants.

Right, I think we should use TCB_CANCELED_BITMASK macro for this (and
add it for architectures that do not define it already).

> 
>> +	/* Issue a 6 argument syscall, the nr [%rax] being the syscall
>> +	   number.  */
>> +	mov    %rdi,%r11
>> +	mov    %rsi,%rax
>> +	mov    %rdx,%rdi
>> +	mov    %rcx,%rsi
>> +	mov    %r8,%rdx
>> +	mov    %r9,%r10
>> +	mov    8(%rsp),%r8
>> +	mov    16(%rsp),%r9
>> +	mov    %r11,8(%rsp)
>> +	syscall
>> +
>> +	.globl __syscall_cancel_arch_end
>> +	.type  __syscall_cancel_arch_end,@function
> 
> Again, no type here please.  It will confuse the disassembler.

Ack.

> 
> Thanks,
> Florian
>
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 f8a142da72..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/cancellation.S
+++ /dev/null
@@ -1,104 +0,0 @@ 
-/* Copyright (C) 2009-2019 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
-   <https://www.gnu.org/licenses/>.  */
-
-#include <sysdep.h>
-#include <tcb-offsets.h>
-#include <kernel-features.h>
-#include <lowlevellock-futex.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
-
-
-#define LOAD_PRIVATE_FUTEX_WAIT(reg) \
-	movl	$(FUTEX_WAIT | FUTEX_PRIVATE_FLAG), reg
-
-/* 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 cf8da66eb7..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/libc-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* Copyright (C) 2009-2019 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
-   <https://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 e7dcc61f6e..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/librt-cancellation.S
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* Copyright (C) 2009-2019 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
-   <https://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/syscall_cancel.S b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S
new file mode 100644
index 0000000000..c7364bfff8
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S
@@ -0,0 +1,61 @@ 
+/* Cancellable syscall wrapper - x86_64 version.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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>
+
+/* long int [rax] __syscall_cancel_arch (volatile int *cancelhandling [%rdi],
+					 __syscall_arg_t nr   [%rsi],
+					 __syscall_arg_t arg1 [%rdx],
+					 __syscall_arg_t arg2 [%rcx],
+					 __syscall_arg_t arg3 [%r8],
+					 __syscall_arg_t arg4 [%r9],
+					 __syscall_arg_t arg5 [SP+8],
+					 __syscall_arg_t arg6 [SP+16])  */
+
+ENTRY (__syscall_cancel_arch)
+
+	.globl __syscall_cancel_arch_start
+	.type  __syscall_cancel_arch_start,@function
+__syscall_cancel_arch_start:
+
+	/* if (*cancelhandling & CANCELED_BITMASK)
+	     __syscall_do_cancel()  */
+	mov    (%rdi),%eax
+	testb  $4, (%rdi)
+	jne    __syscall_do_cancel
+
+	/* Issue a 6 argument syscall, the nr [%rax] being the syscall
+	   number.  */
+	mov    %rdi,%r11
+	mov    %rsi,%rax
+	mov    %rdx,%rdi
+	mov    %rcx,%rsi
+	mov    %r8,%rdx
+	mov    %r9,%r10
+	mov    8(%rsp),%r8
+	mov    16(%rsp),%r9
+	mov    %r11,8(%rsp)
+	syscall
+
+	.globl __syscall_cancel_arch_end
+	.type  __syscall_cancel_arch_end,@function
+__syscall_cancel_arch_end:
+	ret
+
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index ae8034743b..5d033d1ac0 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -18,7 +18,6 @@  SSP_BASE_OFFSET		offsetof (tcbhead_t, ssp_base)
 -- 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