diff mbox series

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

Message ID 1519679016-12241-7-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 patch adds the i386 modifications required for the BZ#12683.
It basically provides the required ucontext_get_pc symbol, add the
cancelable syscall wrapper and fix a thread atomic update macro.

On i386 an arch-specific cancellation implementation is required
because depending of the glibc configuration and underlying kernel
the syscall may be done using a vDSO symbol (__kernel_vsyscall).
By using the vDSO symbol the resulting PC value for an interrupted
syscall points to an adress outside the expected markers in
__syscall_cancel_arch.  It has been discussed in LKML [1] on how
kernel could help userland to accomplish it, but afaik discussion
was stalled.

Also, since glibc supports i486, the old 'int 0x80' should be used
in the syscall wrapper.  One option could make minimum default chip
to pentium II (which implements sysenter) or add a runtime check
on syscall_cancel.S to use 'int 0x80' or sysenter.

Similar to x86_64, 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.

Checked on i686-linux-gnu.

	[BZ #12683]
	* sysdeps/i386/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
	THREAD_ATOMIC_BIT_SET): Remove macro.
	* sysdeps/unix/sysv/linux/i386/Makefile
	[$(subdir) = elf] (sysdep-rtld_routines): Add libc-do-syscall object.
	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (lll_wait_tid): Use
	cancellable futex syscall macro.
	* sysdeps/unix/sysv/linux/i386/syscall_cancel.S: New file.
	* sysdeps/unix/sysv/linux/i386/sigcontextinfo.h (ucontext_get_pc):
	New function.

[1] https://lkml.org/lkml/2016/3/8/1105
---
 ChangeLog                                     |  10 +++
 sysdeps/i386/nptl/tls.h                       |  11 ---
 sysdeps/unix/sysv/linux/i386/Makefile         |   2 +-
 sysdeps/unix/sysv/linux/i386/lowlevellock.h   |   2 +-
 sysdeps/unix/sysv/linux/i386/sigcontextinfo.h |  13 ++++
 sysdeps/unix/sysv/linux/i386/syscall_cancel.S | 107 ++++++++++++++++++++++++++
 6 files changed, 132 insertions(+), 13 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/i386/syscall_cancel.S

-- 
2.7.4

Comments

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

> It basically provides the required ucontext_get_pc symbol, add the

> cancelable syscall wrapper and fix a thread atomic update macro.


This also seems fine.

> On i386 an arch-specific cancellation implementation is required

> because depending of the glibc configuration and underlying kernel

> the syscall may be done using a vDSO symbol (__kernel_vsyscall).

...
> Also, since glibc supports i486, the old 'int 0x80' should be used

> in the syscall wrapper.  One option could make minimum default chip

> to pentium II (which implements sysenter) or add a runtime check

> on syscall_cancel.S to use 'int 0x80' or sysenter.


If I remember correctly, there can be only one 'sysenter' instruction
in the entire user address space, due to awkward limitations of the
interface it presents to the kernel.  That was why __kernel_vsyscall
was added in the first place.

We can probably live with using int 0x80 for these syscalls that may
well be blocking anyway.

> Similar to x86_64, 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.


Same comment as for x86_64 -- shouldn't we get rid of or repair _all_ of
the THREAD_ATOMIC_ macros that don't honor their arguments?

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

>> This patch adds the i386 modifications required for the BZ#12683.

>> It basically provides the required ucontext_get_pc symbol, add the

>> cancelable syscall wrapper and fix a thread atomic update macro.

> 

> This also seems fine.

> 

>> On i386 an arch-specific cancellation implementation is required

>> because depending of the glibc configuration and underlying kernel

>> the syscall may be done using a vDSO symbol (__kernel_vsyscall).

> ...

>> Also, since glibc supports i486, the old 'int 0x80' should be used

>> in the syscall wrapper.  One option could make minimum default chip

>> to pentium II (which implements sysenter) or add a runtime check

>> on syscall_cancel.S to use 'int 0x80' or sysenter.

> 

> If I remember correctly, there can be only one 'sysenter' instruction

> in the entire user address space, due to awkward limitations of the

> interface it presents to the kernel.  That was why __kernel_vsyscall

> was added in the first place.

> 

> We can probably live with using int 0x80 for these syscalls that may

> well be blocking anyway.

> 

>> Similar to x86_64, 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.

> 

> Same comment as for x86_64 -- shouldn't we get rid of or repair _all_ of

> the THREAD_ATOMIC_ macros that don't honor their arguments?

> 

> zw

> 


Alright, I will remove then in an extra patch as well.
diff mbox series

Patch

diff --git a/sysdeps/i386/nptl/tls.h b/sysdeps/i386/nptl/tls.h
index fcda135..ae4de89 100644
--- a/sysdeps/i386/nptl/tls.h
+++ b/sysdeps/i386/nptl/tls.h
@@ -383,17 +383,6 @@  tls_fill_user_desc (union user_desc_init *desc,
 	      abort (); })
 
 
-/* Atomic set bit.  */
-#define THREAD_ATOMIC_BIT_SET(descr, member, bit) \
-  (void) ({ if (sizeof ((descr)->member) == 4)				      \
-	      asm volatile (LOCK_PREFIX "orl %1, %%gs:%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)
diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile
index 4080b8c..bae2e4b 100644
--- a/sysdeps/unix/sysv/linux/i386/Makefile
+++ b/sysdeps/unix/sysv/linux/i386/Makefile
@@ -6,7 +6,7 @@  sysdep_routines += ioperm iopl vm86
 endif
 
 ifeq ($(subdir),elf)
-sysdep-dl-routines += libc-do-syscall
+sysdep-rtld_routines += libc-do-syscall
 sysdep-others += lddlibc4
 install-bin += lddlibc4
 endif
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index 38fbc25..072bc11 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -230,7 +230,7 @@  extern int __lll_timedlock_elision (int *futex, short *adapt_count,
   do {					\
     __typeof (tid) __tid;		\
     while ((__tid = (tid)) != 0)	\
-      lll_futex_wait (&(tid), __tid, LLL_SHARED);\
+      lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);\
   } while (0)
 
 extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)
diff --git a/sysdeps/unix/sysv/linux/i386/sigcontextinfo.h b/sysdeps/unix/sysv/linux/i386/sigcontextinfo.h
index a4ed29a..e7a9f17 100644
--- a/sysdeps/unix/sysv/linux/i386/sigcontextinfo.h
+++ b/sysdeps/unix/sysv/linux/i386/sigcontextinfo.h
@@ -16,6 +16,11 @@ 
    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 struct sigcontext
 #define SIGCONTEXT_EXTRA_ARGS
 #define GET_PC(ctx)	((void *) ctx.eip)
@@ -48,3 +53,11 @@  do {									      \
 		      "i" (sizeof (struct sigcontext) / 4)		      \
 		    : "cc", "edi");					      \
 } while (0)
+
+static inline uintptr_t
+ucontext_get_pc (const ucontext_t *uc)
+{
+  return uc->uc_mcontext.gregs[REG_EIP];
+}
+
+#endif /* _SIGCONTEXTINFO_H  */
diff --git a/sysdeps/unix/sysv/linux/i386/syscall_cancel.S b/sysdeps/unix/sysv/linux/i386/syscall_cancel.S
new file mode 100644
index 0000000..5596b3e
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/i386/syscall_cancel.S
@@ -0,0 +1,107 @@ 
+/* Cancellable syscall wrapper.  Linux/i686 version.
+   Copyright (C) 2017 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 [eax] __syscall_cancel_arch (int *cancelhandling [SP],
+					 long int nr   [SP+4],
+					 long int arg1 [SP+8],
+					 long int arg2 [SP+12],
+					 long int arg3 [SP+16],
+					 long int arg4 [SP+20],
+					 long int arg5 [SP+24],
+					 long int arg6 [SP+28])  */
+
+ENTRY (__syscall_cancel_arch)
+	pushl %ebp
+	cfi_def_cfa_offset (8)
+	cfi_offset (ebp, -8)
+	pushl %edi
+	cfi_def_cfa_offset (12)
+	cfi_offset (edi, -12)
+	pushl %esi
+	cfi_def_cfa_offset (16)
+	cfi_offset (esi, -16)
+	pushl %ebx
+	cfi_def_cfa_offset (20)
+	cfi_offset (ebx, -20)
+
+	.global __syscall_cancel_arch_start
+	.type   __syscall_cancel_arch_start, @function
+__syscall_cancel_arch_start:
+
+	/* if (*cancelhandling & CANCELED_BITMASK)
+	     __syscall_do_cancel()  */
+	testb	$4, (%eax)
+	jne     1f
+
+	/* Issue a 6 argument syscall, the nr [%eax] being the syscall
+	   number.  */
+	movl    24(%esp), %eax
+	movl    28(%esp), %ebx
+	movl    32(%esp), %ecx
+	movl    36(%esp), %edx
+	movl    40(%esp), %esi
+	movl    44(%esp), %edi
+	movl    48(%esp), %ebp
+
+	/* We can not use the vDSO helper for syscall (__kernel_vsyscall)
+	   because the returned PC from kernel will indicate whether the
+	   interrupted syscall have any side-effects that need to be reported
+	   back to program.  And the signal handler (sigcancel_handler at
+	   nptl-init.c) checks the PC agains the __syscall_cancel_arch_*
+	   marks.  */
+	int	$128
+
+	.global __syscall_cancel_arch_end
+	.type   __syscall_cancel_arch_end, @function
+__syscall_cancel_arch_end:
+
+	popl %ebx
+	cfi_restore (ebx)
+	cfi_def_cfa_offset (16)
+	popl %esi
+	cfi_restore (esi)
+	cfi_def_cfa_offset (12)
+	popl %edi
+	cfi_restore (edi)
+	cfi_def_cfa_offset (8)
+	popl %ebp
+	cfi_restore (ebp)
+	cfi_def_cfa_offset (4)
+        ret
+
+1:
+	/* Although the __syscall_do_cancel do not return, we need to stack
+	   being set correctly for unwind.  */
+	popl %ebx
+	cfi_restore (ebx)
+	cfi_def_cfa_offset (16)
+	popl %esi
+	cfi_restore (esi)
+	cfi_def_cfa_offset (12)
+	popl %edi
+	cfi_restore (edi)
+	cfi_def_cfa_offset (8)
+	popl %ebp
+	cfi_restore (ebp)
+	cfi_def_cfa_offset (4)
+	jmp __syscall_do_cancel
+
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)