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