[07/08] nptl: aarch64: Fix Race conditions in pthread cancellation (BZ#12683)
Commit Message
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
On 26/06/15 20:45, Adhemerval Zanella wrote:
> +/* 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)
> +
you save things on the stack here ...
> + .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
> +
... 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).
> --- 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.
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?
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.
thanks.
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.
>
new file mode 100644
@@ -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)
@@ -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
@@ -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) \