diff mbox

[11/13] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683)

Message ID 1444234995-9542-12-git-send-email-adhemerval.zanella@linaro.com
State New
Headers show

Commit Message

Adhemerval Zanella Netto Oct. 7, 2015, 4:23 p.m. UTC 
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

This patch adds the ARM modifications required for the BZ#12683 fix.
It basically removes the enable_asynccancel/disable_asynccancel function
usage on code, provide a arch-specific symbol that contains global
markers to be used in SIGCANCEL handler.

Checked on armhf.

	* sysdeps/unix/sysv/linux/arm/syscall_cancel.S: New file.
	* sysdeps/unix/sysv/linux/arm/sysdep-cancel.h (PSEUDO): Redefine
	to call __syscall_cancel function for cancellable syscalls.
	* sysdeps/unix/sysv/linux/arm/sysdep.h (SYSCALL_CANCEL_ERROR): Add
	definition.
	(SYSCALL_CANCEL_ERRNO): Likewise.
---
 ChangeLog                                    |   7 ++
 sysdeps/unix/sysv/linux/arm/syscall_cancel.S |  72 +++++++++++
 sysdeps/unix/sysv/linux/arm/sysdep-cancel.h  | 181 ++++++---------------------
 sysdeps/unix/sysv/linux/arm/sysdep.h         |   8 ++
 4 files changed, 124 insertions(+), 144 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/arm/syscall_cancel.S

Comments

Phil Blundell Oct. 8, 2015, 3:29 p.m. UTC | #1 
On Wed, 2015-10-07 at 13:23 -0300, Adhemerval Zanella wrote:
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> This patch adds the ARM modifications required for the BZ#12683 fix.
> It basically removes the enable_asynccancel/disable_asynccancel
> function
> usage on code, provide a arch-specific symbol that contains global
> markers to be used in SIGCANCEL handler.
> 
> Checked on armhf.

What architecture variant specifically?  Have you verified that this at
least compiles on older arches?  (I think we still support everything
back to ARMv4.)

> 
+	PSEUDO_CANCEL_BEFORE;					
> 	\
> +	ldr	r0, =SYS_ify (syscall_name);			
> 	\
> +	PSEUDO_CANCEL_AFTER;						
> 

It's still not very clear to me that the PSEUDO_CANCEL_BEFORE and
PSEUDO_CANCEL_AFTER macros are achieving anything other than making the
code harder to read.  Is there any reason these couldn't just be
written in line?

p.
Adhemerval Zanella Netto Oct. 9, 2015, 1:27 p.m. UTC | #2 
On 08-10-2015 12:29, Phil Blundell wrote:
> On Wed, 2015-10-07 at 13:23 -0300, Adhemerval Zanella wrote:
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>> This patch adds the ARM modifications required for the BZ#12683 fix.
>> It basically removes the enable_asynccancel/disable_asynccancel
>> function
>> usage on code, provide a arch-specific symbol that contains global
>> markers to be used in SIGCANCEL handler.
>>
>> Checked on armhf.
> 
> What architecture variant specifically?  Have you verified that this at
> least compiles on older arches?  (I think we still support everything
> back to ARMv4.)

I checked on arm7l and arm8l.  I will try with a build for armv4.

> 
>>
> +	PSEUDO_CANCEL_BEFORE;					
>> 	\
>> +	ldr	r0, =SYS_ify (syscall_name);			
>> 	\
>> +	PSEUDO_CANCEL_AFTER;						
>>
> 
> It's still not very clear to me that the PSEUDO_CANCEL_BEFORE and
> PSEUDO_CANCEL_AFTER macros are achieving anything other than making the
> code harder to read.  Is there any reason these couldn't just be
> written in line?

Indeed, I will change it.

> 
> p.
>
Adhemerval Zanella Netto Oct. 9, 2015, 1:59 p.m. UTC | #3 
On 09-10-2015 10:27, Adhemerval Zanella wrote:
> 
> 
> On 08-10-2015 12:29, Phil Blundell wrote:
>> On Wed, 2015-10-07 at 13:23 -0300, Adhemerval Zanella wrote:
>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>
>>> This patch adds the ARM modifications required for the BZ#12683 fix.
>>> It basically removes the enable_asynccancel/disable_asynccancel
>>> function
>>> usage on code, provide a arch-specific symbol that contains global
>>> markers to be used in SIGCANCEL handler.
>>>
>>> Checked on armhf.
>>
>> What architecture variant specifically?  Have you verified that this at
>> least compiles on older arches?  (I think we still support everything
>> back to ARMv4.)
> 
> I checked on arm7l and arm8l.  I will try with a build for armv4.
> 
>>
>>>
>> +	PSEUDO_CANCEL_BEFORE;					
>>> 	\
>>> +	ldr	r0, =SYS_ify (syscall_name);			
>>> 	\
>>> +	PSEUDO_CANCEL_AFTER;						
>>>
>>
>> It's still not very clear to me that the PSEUDO_CANCEL_BEFORE and
>> PSEUDO_CANCEL_AFTER macros are achieving anything other than making the
>> code harder to read.  Is there any reason these couldn't just be
>> written in line?
> 
> Indeed, I will change it.

I just recalled why I had to continue use PSEUDO_CANCEL_AFTER/BEFORE:
the c-preprocesso complains with the use of # (in stack and .pad handling)
in the c-macro definition.  I am not well versed in ARM assembly, so if
you have an alternative way to define constants without '#' let me know.

> 
>>
>> p.
>>
Phil Blundell Oct. 9, 2015, 2:21 p.m. UTC | #4 
On Fri, 2015-10-09 at 10:59 -0300, Adhemerval Zanella wrote:
> 
> I just recalled why I had to continue use PSEUDO_CANCEL_AFTER/BEFORE:
> the c-preprocesso complains with the use of # (in stack and .pad
> handling)
> in the c-macro definition.  I am not well versed in ARM assembly, so
> if
> you have an alternative way to define constants without '#' let me 
> know.

GAS accepts $ as an alternative to #.  

p.
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 9b0f0ee..f922fa0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@ 
 2015-10-07  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
 
+	* sysdeps/unix/sysv/linux/arm/syscall_cancel.S: New file.
+	* sysdeps/unix/sysv/linux/arm/sysdep-cancel.h (PSEUDO): Redefine
+	to call __syscall_cancel function for cancellable syscalls.
+	* sysdeps/unix/sysv/linux/arm/sysdep.h (SYSCALL_CANCEL_ERROR): Add
+	definition.
+	(SYSCALL_CANCEL_ERRNO): Likewise.
+
 	* sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S: New file.
 	* sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h (PSEUDO): Redefine
 	to call __syscall_cancel function for cancellable syscalls.
diff --git a/sysdeps/unix/sysv/linux/arm/syscall_cancel.S b/sysdeps/unix/sysv/linux/arm/syscall_cancel.S
new file mode 100644
index 0000000..da4b454
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/arm/syscall_cancel.S
@@ -0,0 +1,72 @@ 
+/* Cancellable syscall wrapper - arm version.
+   Copyright (C) 2015 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 [r0] __syscall_cancel_arch (int *cancelhandling [r0],
+					long int nr   [r1],
+					long int arg1 [r2],
+					long int arg2 [r3],
+					long int arg3 [SP],
+					long int arg4 [SP+4],
+					long int arg5 [SP+8],
+					long int arg6 [SP+12])  */
+
+#ifdef __thumb2__
+	.thumb
+#endif
+	.syntax unified
+
+ENTRY (__syscall_cancel_arch)
+	.fnstart
+        mov	ip,sp
+        stmfd	sp!,{r4,r5,r6,lr}
+	.save	{r4,r5,r6,lr}
+
+	cfi_adjust_cfa_offset (20)
+	cfi_rel_offset (lr, 16)
+
+	.globl __syscall_cancel_arch_start
+__syscall_cancel_arch_start:
+
+	/* if (*cancelhandling & CANCELED_BITMASK)
+	     __syscall_do_cancel()  */
+        ldr	r0,[r0]
+	tst	r0, #4
+        bne	1f
+
+	/* Issue a 6 argument syscall, the nr [r1] being the syscall
+	   number.  */
+        mov	r7,r1
+        mov	r0,r2
+        mov	r1,r3
+        ldmfd	ip,{r2,r3,r4,r5,r6}
+        svc	0x0
+
+	.globl __syscall_cancel_arch_end
+__syscall_cancel_arch_end:
+	ldmfd	sp!,{r4,r5,r6,lr}
+	cfi_adjust_cfa_offset (-16);
+        bx	lr
+
+1:
+	mov	lr, pc
+	b	__syscall_do_cancel
+	.fnend
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)
diff --git a/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h b/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h
index bdefa80..afbf07d 100644
--- a/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h
+++ b/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h
@@ -19,10 +19,17 @@ 
 #include <tls.h>
 #ifndef __ASSEMBLER__
 # include <nptl/pthreadP.h>
+# include <sys/ucontext.h>
 #endif
 
 #if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
 
+# if IS_IN (libc)
+#  define JMP_SYSCALL_CANCEL  HIDDEN_JUMPTARGET(__syscall_cancel)
+# else
+#  define JMP_SYSCALL_CANCEL  __syscall_cancel(PLT)
+# endif
+
 /* NOTE: We do mark syscalls with unwind annotations, for the benefit of
    cancellation; but they're really only accurate at the point of the
    syscall.  The ARM unwind directives are not rich enough without adding
@@ -31,16 +38,10 @@ 
 # undef PSEUDO
 # define PSEUDO(name, syscall_name, args)				\
 	.text;								\
-  ENTRY (__##syscall_name##_nocancel);					\
-	CFI_SECTIONS;							\
-	DO_CALL (syscall_name, args);					\
-	cmn	r0, $4096;						\
-	PSEUDO_RET;							\
-  END (__##syscall_name##_nocancel);					\
   ENTRY (name);								\
 	SINGLE_THREAD_P;						\
-	DOARGS_##args;							\
 	bne .Lpseudo_cancel;						\
+	DOARGS_##args;							\
 	cfi_remember_state;						\
 	ldr	r7, =SYS_ify (syscall_name);				\
 	swi	0x0;							\
@@ -50,150 +51,36 @@ 
 	cfi_restore_state;						\
   .Lpseudo_cancel:							\
 	.fnstart;	/* matched by the .fnend in UNDOARGS below.  */	\
-	DOCARGS_##args;	/* save syscall args etc. around CENABLE.  */	\
-	CENABLE;							\
-	mov ip, r0;		/* put mask in safe place.  */		\
-	UNDOCARGS_##args;	/* restore syscall args.  */		\
-	ldr	r7, =SYS_ify (syscall_name);				\
-	swi	0x0;		/* do the call.  */			\
-	mov	r7, r0;		/* save syscall return value.  */	\
-	mov	r0, ip;		/* get mask back.  */			\
-	CDISABLE;							\
-	mov	r0, r7;		/* retrieve return value.  */		\
-	RESTORE_LR_##args;						\
-	UNDOARGS_##args;						\
+	push	{r4, r5, lr};						\
+	.save   {r4, r5, lr};						\
+	PSEUDO_CANCEL_BEFORE;						\
+	ldr	r0, =SYS_ify (syscall_name);				\
+	PSEUDO_CANCEL_AFTER;						\
+	pop	{r4, r5, lr};						\
+	.fnend;								\
 	cmn	r0, $4096
 
-/* DOARGS pushes eight bytes on the stack for five arguments, twelve bytes for
-   six arguments, and four bytes for fewer.  In order to preserve doubleword
-   alignment, sometimes we must save an extra register.  */
-
-# define RESTART_UNWIND				\
-	.fnend;					\
-	.fnstart;				\
-	.save	{r7};				\
-	.save	{lr}
-
-# define DOCARGS_0				\
-	.save {r7};				\
-	push	{lr};				\
-	cfi_adjust_cfa_offset (4);		\
-	cfi_rel_offset (lr, 0);			\
-	.save	{lr}
-# define UNDOCARGS_0
-# define RESTORE_LR_0				\
-	pop	{lr};				\
-	cfi_adjust_cfa_offset (-4);		\
-	cfi_restore (lr)
-
-# define DOCARGS_1				\
-	.save	{r7};				\
-	push	{r0, r1, lr};			\
-	cfi_adjust_cfa_offset (12);		\
-	cfi_rel_offset (lr, 8);			\
-	.save	{lr};				\
-	.pad	#8
-# define UNDOCARGS_1				\
-	ldr r0, [sp], #8;			\
-	cfi_adjust_cfa_offset (-8);		\
-	RESTART_UNWIND
-# define RESTORE_LR_1				\
-	RESTORE_LR_0
-
-# define DOCARGS_2				\
-	.save	{r7};				\
-	push	{r0, r1, lr};			\
-	cfi_adjust_cfa_offset (12);		\
-	cfi_rel_offset (lr, 8);			\
-	.save	{lr};				\
-	.pad	#8
-# define UNDOCARGS_2				\
-	pop	{r0, r1};			\
-	cfi_adjust_cfa_offset (-8);		\
-	RESTART_UNWIND
-# define RESTORE_LR_2				\
-	RESTORE_LR_0
-
-# define DOCARGS_3				\
-	.save	{r7};				\
-	push	{r0, r1, r2, r3, lr};		\
-	cfi_adjust_cfa_offset (20);		\
-	cfi_rel_offset (lr, 16);		\
-	.save	{lr};				\
-	.pad	#16
-# define UNDOCARGS_3				\
-	pop	{r0, r1, r2, r3};		\
-	cfi_adjust_cfa_offset (-16);		\
-	RESTART_UNWIND
-# define RESTORE_LR_3				\
-	RESTORE_LR_0
-
-# define DOCARGS_4				\
-	.save	{r7};				\
-	push	{r0, r1, r2, r3, lr};		\
-	cfi_adjust_cfa_offset (20);		\
-	cfi_rel_offset (lr, 16);		\
-	.save	{lr};				\
-	.pad	#16
-# define UNDOCARGS_4				\
-	pop	{r0, r1, r2, r3};		\
-	cfi_adjust_cfa_offset (-16);		\
-	RESTART_UNWIND
-# define RESTORE_LR_4				\
-	RESTORE_LR_0
-
-/* r4 is only stmfd'ed for correct stack alignment.  */
-# define DOCARGS_5				\
-	.save	{r4, r7};			\
-	push	{r0, r1, r2, r3, r4, lr};	\
-	cfi_adjust_cfa_offset (24);		\
-	cfi_rel_offset (lr, 20);		\
-	.save	{lr};				\
-	.pad	#20
-# define UNDOCARGS_5				\
-	pop	{r0, r1, r2, r3};		\
-	cfi_adjust_cfa_offset (-16);		\
-	.fnend;					\
-	.fnstart;				\
-	.save	{r4, r7};			\
-	.save	{lr};				\
-	.pad	#4
-# define RESTORE_LR_5				\
-	pop	{r4, lr};			\
-	cfi_adjust_cfa_offset (-8);		\
-	/* r4 will be marked as restored later.  */ \
-	cfi_restore (lr)
-
-# define DOCARGS_6				\
-	.save	{r4, r5, r7};			\
-	push	{r0, r1, r2, r3, lr};		\
-	cfi_adjust_cfa_offset (20);		\
-	cfi_rel_offset (lr, 16);		\
-	.save	{lr};				\
-	.pad	#16
-# define UNDOCARGS_6				\
-	pop	{r0, r1, r2, r3};		\
-	cfi_adjust_cfa_offset (-16);		\
-	.fnend;					\
-	.fnstart;				\
-	.save	{r4, r5, r7};			\
-	.save	{lr};
-# define RESTORE_LR_6				\
-	RESTORE_LR_0
+# define PSEUDO_CANCEL_BEFORE						\
+	.pad	#20;							\
+	sub	sp, sp, #20;						\
+	ldr	r5, [sp, #32];						\
+	ldr	r4, [sp, #36];						\
+	str	r3, [sp];						\
+	mov	r3, r2;							\
+	str	r5, [sp, #4];						\
+	mov	r2, r1;							\
+	str	r4, [sp, #8];						\
+	mov	r1, r0
+
+# define PSEUDO_CANCEL_AFTER						\
+	bl	JMP_SYSCALL_CANCEL;					\
+	add	sp, sp, #20
+
 
 # if IS_IN (libpthread)
-#  define CENABLE	bl PLTJMP(__pthread_enable_asynccancel)
-#  define CDISABLE	bl PLTJMP(__pthread_disable_asynccancel)
 #  define __local_multiple_threads __pthread_multiple_threads
 # elif IS_IN (libc)
-#  define CENABLE	bl PLTJMP(__libc_enable_asynccancel)
-#  define CDISABLE	bl PLTJMP(__libc_disable_asynccancel)
 #  define __local_multiple_threads __libc_multiple_threads
-# elif IS_IN (librt)
-#  define CENABLE	bl PLTJMP(__librt_enable_asynccancel)
-#  define CDISABLE	bl PLTJMP(__librt_disable_asynccancel)
-# else
-#  error Unsupported library
 # endif
 
 # if IS_IN (libpthread) || IS_IN (libc)
@@ -238,4 +125,10 @@  extern int __local_multiple_threads attribute_hidden;
 # define RTLD_SINGLE_THREAD_P \
   __builtin_expect (THREAD_GETMEM (THREAD_SELF, \
 				   header.multiple_threads) == 0, 1)
+
+static inline
+long int __pthread_get_ip (const struct ucontext *uc)
+{
+  return uc->uc_mcontext.arm_pc;
+}
 #endif
diff --git a/sysdeps/unix/sysv/linux/arm/sysdep.h b/sysdeps/unix/sysv/linux/arm/sysdep.h
index 200f77a..6b18e34 100644
--- a/sysdeps/unix/sysv/linux/arm/sysdep.h
+++ b/sysdeps/unix/sysv/linux/arm/sysdep.h
@@ -387,6 +387,14 @@  __local_syscall_error:						\
 #undef INTERNAL_SYSCALL_ERRNO
 #define INTERNAL_SYSCALL_ERRNO(val, err)	(-(val))
 
+#undef SYSCALL_CANCEL_ERROR
+#define SYSCALL_CANCEL_ERROR(__val) \
+  ((unsigned int) (__val) >= 0xfffff001u)
+
+#undef SYSCALL_CANCEL_ERRNO
+#define SYSCALL_CANCEL_ERRNO(__val) \
+  (-(__val))
+
 /* List of system calls which are supported as vsyscalls.  */
 #define HAVE_CLOCK_GETTIME_VSYSCALL	1
 #define HAVE_GETTIMEOFDAY_VSYSCALL	1