diff mbox

[07/08] nptl: aarch64: Fix Race conditions in pthread cancellation (BZ#12683)

Message ID 558DABCC.3020109@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella June 26, 2015, 7:45 p.m. UTC 
This patch adds the aarch64 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.

--

	* 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.
	(__pthread_get_ip): Add implementation.
	* sysdeps/unix/sysv/linux/aarch64/sysdep.h (SYSCALL_CANCEL_ERROR): Add
	definition.
	(SYSCALL_CANCEL_ERRNO): Likewise.

--

Comments

Adhemerval Zanella June 29, 2015, 10:16 p.m. UTC | #1 
Hi Szabolcs,

Thanks for the review, comments below:

On 29-06-2015 07:32, Szabolcs Nagy wrote:
> On 26/06/15 20:45, Adhemerval Zanella wrote:
>> +ENTRY (__syscall_cancel_arch)
>> +
>> +        stp     x29, x30, [sp, -16]!
>> +        cfi_def_cfa_offset (16)
>> +        cfi_offset (29, -16)
>> +        cfi_offset (30, -8)
>> +        add     x29, sp, 0
>> +        cfi_def_cfa_register (29)
>> +
> 
> you save things on the stack here ...
> 
> 
> ... and tail call into a function that does not restore the stack.
> 
> i think you can omit saving the frame pointer.
> (neither syscall, nor tail call needs it).

My first though was the FP save-restore was required to correct create
unwind information for cancellation handlers, but on a second though
they are not necessary indeed. I have removed them and cancellation
handlers works as intended, right now the syscall wrappers looks like:

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()  */
        ldr     w0, [x0]
        tbnz    w0, 2, 1f

        /* Issue a 6 argument syscall, the nr [x1] being the syscall
           number.  */
        mov     x8, x1
        mov     x0, x2
        mov     x1, x3
        mov     x2, x4
        mov     x3, x5
        mov     x4, x6
        mov     x5, x7
        svc     0x0

        .globl __syscall_cancel_arch_end
        .type  __syscall_cancel_arch_end,@function
__syscall_cancel_arch_end:
        ret

1:
        b       __syscall_do_cancel

END (__syscall_cancel_arch)

> 
>> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
>> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
>> @@ -20,42 +20,50 @@
>>  #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
>> +# endif
>> +
>>  # undef PSEUDO
>>  # define PSEUDO(name, syscall_name, args)                              \
>> -       .section ".text";                                               \
>> -ENTRY (__##syscall_name##_nocancel);                                   \
>> -.Lpseudo_nocancel:                                                     \
>> -       DO_CALL (syscall_name, args);                                   \
>> -.Lpseudo_finish:                                                       \
>> -       cmn     x0, 4095;                                               \
>> -       b.cs    .Lsyscall_error;                                        \
>> -       .subsection 2;                                                  \
>> -       .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
>> +        .section ".text";                                               \
>> +ENTRY (__##syscall_name##_nocancel);                                    \
>> +L(pseudo_nocancel):                                                    \
>> +        DO_CALL (syscall_name, args);                                   \
>> +L(pseudo_finish):                                                      \
>> +        cmn     x0, 4095;                                               \
>> +        b.cs    L(syscall_error);                                      \
>> +        .subsection 2;                                                 \
>> +        .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
> 
> i think there is a problem here that may need independent
> fix (not a regression, but worth a mention):
> 
> a glibc test (debug/tst-backtrace5) checks if 'read' symbol
> is found when read is interrupted and the signal handler
> calls backtrace_symbols.
> 
> however the interrupt will be in __read_nocancel and that
> name is not exported so backtrace does not find it in the
> dynamic symbol table so the check fails. (on some other
> archs the test pass because the nocancel code is within
> the read code, not a separate function).
> 
> it's a silly issue so i haven't got around proposing a fix
> for this yet.
> 

Indeed it would be much better to add more cleanup in this macros.
I have simplified to:

# if IS_IN (libc)
#  define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel)
# else
#  define JMP_SYSCALL_CANCEL __syscall_cancel
# endif

# undef PSEUDO
# define PSEUDO(name, syscall_name, args)                               \
ENTRY (name);                                                           \
        SINGLE_THREAD_P(16);                                            \
        cbnz    w16, L(pseudo_cancel);                                  \
        DO_CALL (syscall_name, args);                                   \
        b       L(pseudo_finish);                                       \
L(pseudo_cancel):                                                       \
        stp     x29, x30, [sp, -16]!;                                   \
        cfi_def_cfa_offset (16);                                        \
        cfi_offset (29, -16);                                           \
        cfi_offset (30, -8);                                            \
        add     x29, sp, 0;                                             \
        cfi_def_cfa_register (29);                                      \
        mov     x6, x5;                                                 \
        mov     x5, x4;                                                 \
        mov     x4, x3;                                                 \
        mov     x3, x2;                                                 \
        mov     x2, x1;                                                 \
        mov     x1, x0;                                                 \
        mov     x0, SYS_ify (syscall_name);                             \
        bl      JMP_SYSCALL_CANCEL;                                     \
        ldp     x29, x30, [sp], 16;                                     \
        cfi_restore (30);                                               \
        cfi_restore (29);                                               \
        cfi_def_cfa (31, 0);                                            \
L(pseudo_finish):                                                       \
        cmn     x0, 4095;                                               \
        b.cs    L(syscall_error);

# undef PSEUDO_END
# define PSEUDO_END(name)                                               \
        SYSCALL_ERROR_HANDLER;                                          \
        cfi_endproc;                                                    \
        .size   name, .-name;

And debug/tst-backtrace{5-6} work as intended as well. What do you think?
Adhemerval Zanella June 30, 2015, 2:12 p.m. UTC | #2 
On 30-06-2015 05:46, Szabolcs Nagy wrote:
> On 29/06/15 23:16, Adhemerval Zanella wrote:
>> they are not necessary indeed. I have removed them and cancellation
>> handlers works as intended, right now the syscall wrappers looks like:
>>
>> 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()  */
>>         ldr     w0, [x0]
>>         tbnz    w0, 2, 1f
>>
>>         /* Issue a 6 argument syscall, the nr [x1] being the syscall
>>            number.  */
>>         mov     x8, x1
>>         mov     x0, x2
>>         mov     x1, x3
>>         mov     x2, x4
>>         mov     x3, x5
>>         mov     x4, x6
>>         mov     x5, x7
>>         svc     0x0
>>
>>         .globl __syscall_cancel_arch_end
>>         .type  __syscall_cancel_arch_end,@function
>> __syscall_cancel_arch_end:
>>         ret
>>
>> 1:
>>         b       __syscall_do_cancel
>>
>> END (__syscall_cancel_arch)
>>
> 
> looks good.
> 
>> Indeed it would be much better to add more cleanup in this macros.
>> I have simplified to:
>>
>> # if IS_IN (libc)
>> #  define JMP_SYSCALL_CANCEL HIDDEN_JUMPTARGET(__syscall_cancel)
>> # else
>> #  define JMP_SYSCALL_CANCEL __syscall_cancel
>> # endif
>>
>> # undef PSEUDO
>> # define PSEUDO(name, syscall_name, args)                               \
>> ENTRY (name);                                                           \
>>         SINGLE_THREAD_P(16);                                            \
>>         cbnz    w16, L(pseudo_cancel);                                  \
>>         DO_CALL (syscall_name, args);                                   \
>>         b       L(pseudo_finish);                                       \
>> L(pseudo_cancel):                                                       \
>>         stp     x29, x30, [sp, -16]!;                                   \
>>         cfi_def_cfa_offset (16);                                        \
>>         cfi_offset (29, -16);                                           \
>>         cfi_offset (30, -8);                                            \
>>         add     x29, sp, 0;                                             \
>>         cfi_def_cfa_register (29);                                      \
>>         mov     x6, x5;                                                 \
>>         mov     x5, x4;                                                 \
>>         mov     x4, x3;                                                 \
>>         mov     x3, x2;                                                 \
>>         mov     x2, x1;                                                 \
>>         mov     x1, x0;                                                 \
>>         mov     x0, SYS_ify (syscall_name);                             \
>>         bl      JMP_SYSCALL_CANCEL;                                     \
>>         ldp     x29, x30, [sp], 16;                                     \
>>         cfi_restore (30);                                               \
>>         cfi_restore (29);                                               \
>>         cfi_def_cfa (31, 0);                                            \
>> L(pseudo_finish):                                                       \
>>         cmn     x0, 4095;                                               \
>>         b.cs    L(syscall_error);
>>
>> # undef PSEUDO_END
>> # define PSEUDO_END(name)                                               \
>>         SYSCALL_ERROR_HANDLER;                                          \
>>         cfi_endproc;                                                    \
>>         .size   name, .-name;
>>
>> And debug/tst-backtrace{5-6} work as intended as well. What do you think?
>>
> 
> is it ok to remove the __<syscall>_nocancel internal symbols?
> 
> otherwise it looks good.

Yes, the idea of 34caaafd1ae38c9295325a1da491d75a92b205b0 is exactly to
remove __<syscall>_nocancel usage.

> 
> thanks.
>
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S b/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S
new file mode 100644
index 0000000..33c51c3
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/syscall_cancel.S
@@ -0,0 +1,75 @@ 
+/* Cancellable syscall wrapper - aarch64 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 [x0],
+					long int nr   [x1],
+					long int arg1 [x2],
+					long int arg2 [x3],
+					long int arg3 [x4],
+					long int arg4 [x5],
+					long int arg5 [x6],
+					long int arg6 [x7])  */
+
+ENTRY (__syscall_cancel_arch)
+
+        stp     x29, x30, [sp, -16]!
+        cfi_def_cfa_offset (16)
+        cfi_offset (29, -16)
+        cfi_offset (30, -8)
+        add     x29, sp, 0
+        cfi_def_cfa_register (29)
+
+	.globl __syscall_cancel_arch_start
+	.type  __syscall_cancel_arch_start,@function
+__syscall_cancel_arch_start:
+
+	/* if (*cancelhandling & CANCELED_BITMASK)
+	     __syscall_do_cancel()  */
+	ldr	w0, [x0]
+	tbnz    w0, 2, 1f
+
+	/* Issue a 6 argument syscall, the nr [x1] being the syscall
+	   number.  */
+	mov	x8, x1
+	mov	x0, x2
+	mov	x1, x3
+	mov	x2, x4
+	mov	x3, x5
+	mov	x4, x6
+	mov	x5, x7
+	svc	0x0
+
+	.globl __syscall_cancel_arch_end
+	.type  __syscall_cancel_arch_end,@function
+__syscall_cancel_arch_end:
+
+        ldp     x29, x30, [sp], 16
+        cfi_remember_state
+        cfi_restore (30)
+        cfi_restore (29)
+        cfi_def_cfa (31, 0)
+	ret
+
+1:
+	cfi_restore_state
+	b	__syscall_do_cancel
+
+END (__syscall_cancel_arch)
+libc_hidden_def (__syscall_cancel_arch)
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
index 36e8e39..ba91268 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h
@@ -20,42 +20,50 @@ 
 #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
+# endif
+
 # undef PSEUDO
 # define PSEUDO(name, syscall_name, args)				\
-	.section ".text";						\
-ENTRY (__##syscall_name##_nocancel);					\
-.Lpseudo_nocancel:							\
-	DO_CALL (syscall_name, args);					\
-.Lpseudo_finish:							\
-	cmn	x0, 4095;						\
-	b.cs	.Lsyscall_error;					\
-	.subsection 2;							\
-	.size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
+        .section ".text";                                               \
+ENTRY (__##syscall_name##_nocancel);                                    \
+L(pseudo_nocancel):							\
+        DO_CALL (syscall_name, args);                                   \
+L(pseudo_finish):							\
+        cmn     x0, 4095;                                               \
+        b.cs    L(syscall_error);					\
+        .subsection 2;							\
+        .size __##syscall_name##_nocancel,.-__##syscall_name##_nocancel; \
 ENTRY (name);								\
 	SINGLE_THREAD_P(16);						\
-	cbz	w16, .Lpseudo_nocancel;					\
-	/* Setup common stack frame no matter the number of args.	\
-	   Also save the first arg, since it's basically free.  */	\
-	stp	x30, x0, [sp, -64]!;					\
-	cfi_adjust_cfa_offset (64);					\
-	cfi_rel_offset (x30, 0);					\
-	DOCARGS_##args;		/* save syscall args around CENABLE.  */ \
-	CENABLE;							\
-	mov	x16, x0;	/* save mask around syscall.  */	\
-	UNDOCARGS_##args;	/* restore syscall args.  */		\
-	DO_CALL (syscall_name, args);					\
-	str	x0, [sp, 8];	/* save result around CDISABLE.  */	\
-	mov	x0, x16;	/* restore mask for CDISABLE.  */	\
-	CDISABLE;							\
-	/* Break down the stack frame, restoring result at once.  */	\
-	ldp	x30, x0, [sp], 64;					\
-	cfi_adjust_cfa_offset (-64);					\
-	cfi_restore (x30);						\
-	b	.Lpseudo_finish;					\
+	cbz	w16, L(pseudo_nocancel);				\
+        stp     x29, x30, [sp, -16]!;					\
+        cfi_def_cfa_offset (16);					\
+        cfi_offset (29, -16);						\
+        cfi_offset (30, -8);						\
+	add	x29, sp, 0;						\
+	cfi_def_cfa_register (29);					\
+	mov	x6, x5;							\
+	mov	x5, x4;							\
+	mov	x4, x3;							\
+	mov	x3, x2;							\
+	mov	x2, x1;							\
+	mov	x1, x0;							\
+	mov	x0, SYS_ify (syscall_name);				\
+	bl	JMP_SYSCALL_CANCEL;					\
+        ldp     x29, x30, [sp], 16;					\
+        cfi_restore (30);                                              \
+        cfi_restore (29);						\
+        cfi_def_cfa (31, 0);						\
+	b       L(pseudo_finish);					\
 	cfi_endproc;							\
 	.size	name, .-name;						\
 	.previous
@@ -65,35 +73,10 @@  ENTRY (name);								\
 	SYSCALL_ERROR_HANDLER;						\
 	cfi_endproc
 
-# define DOCARGS_0
-# define DOCARGS_1
-# define DOCARGS_2	str x1, [sp, 16]
-# define DOCARGS_3	stp x1, x2, [sp, 16]
-# define DOCARGS_4	DOCARGS_3; str x3, [sp, 32]
-# define DOCARGS_5	DOCARGS_3; stp x3, x4, [sp, 32]
-# define DOCARGS_6	DOCARGS_5; str x5, [sp, 48]
-
-# define UNDOCARGS_0
-# define UNDOCARGS_1	ldr x0, [sp, 8]
-# define UNDOCARGS_2	ldp x0, x1, [sp, 8]
-# define UNDOCARGS_3	UNDOCARGS_1; ldp x1, x2, [sp, 16]
-# define UNDOCARGS_4	UNDOCARGS_2; ldp x2, x3, [sp, 24]
-# define UNDOCARGS_5	UNDOCARGS_3; ldp x3, x4, [sp, 32]
-# define UNDOCARGS_6	UNDOCARGS_4; ldp x4, x5, [sp, 40]
-
 # if IS_IN (libpthread)
-#  define CENABLE	bl __pthread_enable_asynccancel
-#  define CDISABLE	bl __pthread_disable_asynccancel
 #  define __local_multiple_threads __pthread_multiple_threads
 # elif IS_IN (libc)
-#  define CENABLE	bl __libc_enable_asynccancel
-#  define CDISABLE	bl __libc_disable_asynccancel
 #  define __local_multiple_threads __libc_multiple_threads
-# elif IS_IN (librt)
-#  define CENABLE	bl __librt_enable_asynccancel
-#  define CDISABLE	bl __librt_disable_asynccancel
-# else
-#  error Unsupported library
 # endif
 
 # if IS_IN (libpthread) || IS_IN (libc)
@@ -131,4 +114,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.pc;
+}
 #endif
diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index fe94a50..5f73b9e 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -199,6 +199,14 @@ 
 # undef INTERNAL_SYSCALL_ERRNO
 # define INTERNAL_SYSCALL_ERRNO(val, err)	(-(val))
 
+#undef SYSCALL_CANCEL_ERROR
+#define SYSCALL_CANCEL_ERROR(__val) \
+  ((unsigned long) (__val) >= (unsigned long) -4095)
+
+#undef SYSCALL_CANCEL_ERRNO
+#define SYSCALL_CANCEL_ERRNO(__val) \
+  (-(__val))
+
 # define LOAD_ARGS_0()				\
   register long _x0 asm ("x0");
 # define LOAD_ARGS_1(x0)			\