| Message ID | 1513019223-7603-9-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | nptl: Fix Race conditions in pthread cancellation (BZ#12683) | expand |
On Mon, 2017-12-11 at 17:06 -0200, Adhemerval Zanella wrote: > +1: > + mov lr, pc > + b __syscall_do_cancel > + .fnend > +END (__syscall_cancel_arch) I'm not sure I quite understand what's going on here. Where is __syscall_do_cancel() supposed to be returning to? p.
On Dez 11 2017, Phil Blundell <pb@pbcl.net> wrote: > On Mon, 2017-12-11 at 17:06 -0200, Adhemerval Zanella wrote: >> +1: >> + mov lr, pc >> + b __syscall_do_cancel >> + .fnend >> +END (__syscall_cancel_arch) > > I'm not sure I quite understand what's going on here. Where is > __syscall_do_cancel() supposed to be returning to? It never returns. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
On Mon, 2017-12-11 at 21:29 +0100, Andreas Schwab wrote: > On Dez 11 2017, Phil Blundell <pb@pbcl.net> wrote: > > > On Mon, 2017-12-11 at 17:06 -0200, Adhemerval Zanella wrote: > > > +1: > > > + mov lr, pc > > > + b __syscall_do_cancel > > > + .fnend > > > +END (__syscall_cancel_arch) > > > > I'm not sure I quite understand what's going on here. Where is > > __syscall_do_cancel() supposed to be returning to? > > It never returns. Right, that's sort of what I thought. So why set up lr here? p.
On Mon, 11 Dec 2017, Adhemerval Zanella wrote: > +ENTRY (__syscall_cancel_arch) > + .fnstart > + mov ip,sp > + stmfd sp!, {r4,r5,r6,r7,lr} > + .save {r4,r5,r6,r7,lr} > + > + cfi_adjust_cfa_offset (20) > + cfi_rel_offset (lr, 16) I'd expect the saves of other registers to be described in the CFI as well (that is, the .debug_frame CFI which is generated by cfi_* on ARM). > + .globl __syscall_cancel_arch_end > +__syscall_cancel_arch_end: > + ldmfd sp!, {r4,r5,r6,r7,lr} > + cfi_adjust_cfa_offset (-20); Then, I'd expect cfi_restore calls here. > +1: > + mov lr, pc > + b __syscall_do_cancel And this code is jumped to from a position where the stack has been adjusted, but the CFI at this point reflects a state where it has been restored. So you need a fresh set of CFI directives to make the CFI accurate in this part of the function. (I haven't checked whether any other architecture versions of this function might have similar CFI issues.) Also, it looks like you jump to the C function __syscall_do_cancel with a stack adjustment that is not a multiple of 8. I think the AAPCS requirement for the stack pointer to be a multiple of 8 at public interfaces applies to such a jump to a C function, so you need to save and restore an additional register to preserve alignment. -- Joseph S. Myers joseph@codesourcery.com
On 11/12/2017 21:57, Joseph Myers wrote: > On Mon, 11 Dec 2017, Adhemerval Zanella wrote: > >> +ENTRY (__syscall_cancel_arch) >> + .fnstart >> + mov ip,sp >> + stmfd sp!, {r4,r5,r6,r7,lr} >> + .save {r4,r5,r6,r7,lr} >> + >> + cfi_adjust_cfa_offset (20) >> + cfi_rel_offset (lr, 16) > > I'd expect the saves of other registers to be described in the CFI as well > (that is, the .debug_frame CFI which is generated by cfi_* on ARM). > >> + .globl __syscall_cancel_arch_end >> +__syscall_cancel_arch_end: >> + ldmfd sp!, {r4,r5,r6,r7,lr} >> + cfi_adjust_cfa_offset (-20); > > Then, I'd expect cfi_restore calls here. > >> +1: >> + mov lr, pc >> + b __syscall_do_cancel > > And this code is jumped to from a position where the stack has been > adjusted, but the CFI at this point reflects a state where it has been > restored. So you need a fresh set of CFI directives to make the CFI > accurate in this part of the function. (I haven't checked whether any > other architecture versions of this function might have similar CFI > issues.) > > Also, it looks like you jump to the C function __syscall_do_cancel with a > stack adjustment that is not a multiple of 8. I think the AAPCS > requirement for the stack pointer to be a multiple of 8 at public > interfaces applies to such a jump to a C function, so you need to save and > restore an additional register to preserve alignment. > I used the generic version as base for ARM one with the expected flags required for correctly unwind to work, -fexcept and -fasynchronous-unwind-tables (which pointed that I need to add -fasynchronous-unwind-tables on generic syscall_cancel CFLAGS as well). Adjusting ARM to build the generic version I see the following assembly being generated (I had to add -marm) with GCC 5: --- .type __GI___syscall_cancel_arch, %function __GI___syscall_cancel_arch: .fnstart .LFB41: push {r4, r5, r7, lr} .save {r4, r5, r7, lr} .syntax divided .global __syscall_cancel_arch_start .type __syscall_cancel_arch_start,%function __syscall_cancel_arch_start: .arm .syntax unified ldr ip, [r0] tst ip, #4 bne .L5 mov r0, r2 add r2, sp, #16 mov r7, r1 mov r1, r3 ldm r2, {r2, r3, r4, r5} .syntax divided swi 0x0 @ syscall nr .global __syscall_cancel_arch_end .type __syscall_cancel_arch_end,%function __syscall_cancel_arch_end: .arm .syntax unified pop {r4, r5, r7, pc} .L5: bl __syscall_do_cancel(PLT) .fnend --- So I am not sure we need all the CFI directives to enable cancellation work on ARM (at least current syscall wrappers which use C version of {INLINE,INTERNAL}_SYSCALL are not generating them). The generate code seems correct, does not shown any regression on nptl testcases and it is slight better than my version so I think we can set ARM to use -marm on syscall_cancel.{o,os} built and remove the arch specific assembly.
On Tue, 12 Dec 2017, Adhemerval Zanella wrote: > So I am not sure we need all the CFI directives to enable cancellation work > on ARM (at least current syscall wrappers which use C version of > {INLINE,INTERNAL}_SYSCALL are not generating them). The cfi_* on ARM are for debugging, not for cancellation (and so to get corresponding output from the compiler you'd need to compile with -g). ARM uses ".cfi_sections .debug_frame". -- Joseph S. Myers joseph@codesourcery.com
On 12/12/2017 11:51, Joseph Myers wrote: > On Tue, 12 Dec 2017, Adhemerval Zanella wrote: > >> So I am not sure we need all the CFI directives to enable cancellation work >> on ARM (at least current syscall wrappers which use C version of >> {INLINE,INTERNAL}_SYSCALL are not generating them). > > The cfi_* on ARM are for debugging, not for cancellation (and so to get > corresponding output from the compiler you'd need to compile with -g). > ARM uses ".cfi_sections .debug_frame". > Right, I think we can let the compiler generate the correct info in such cases. Below there is an updated patch that removes the arch specific implementation. I need to adjust the syscall_cancel from 02/19 patch to uses a '%' mark for function symbol declaration (the version I sent uses a '@' which ARM gas interprets as a inline comment). --- This patch adds the ARM modifications required for the BZ#12683. It basically adds the required ucontext_get_pc function and adjust the generic syscall_cancel build. For ARM we need to build syscall_cancel in ARM mode (-marm) to avoid INTERNAL_SYSCALL to issue the syscall through the helper gate __libc_do_syscall (which invalidates the mark checks on SIGCANCEL handler). Checked on arm-linux-gnueabihf. * sysdeps/unix/sysv/linux/arm/Makefile (CFLAGS-syscall_cancel.c): New rule. * sysdeps/unix/sysv/linux/arm/sigcontextinfo.h (ucontext_get_pc): New function. Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/arm/Makefile | 3 +++ sysdeps/unix/sysv/linux/arm/sigcontextinfo.h | 12 ++++++++++++ 3 files changed, 20 insertions(+) diff --git a/sysdeps/unix/sysv/linux/arm/Makefile b/sysdeps/unix/sysv/linux/arm/Makefile index 4adc35d..8f01b52 100644 --- a/sysdeps/unix/sysv/linux/arm/Makefile +++ b/sysdeps/unix/sysv/linux/arm/Makefile @@ -30,6 +30,9 @@ endif ifeq ($(subdir),nptl) libpthread-sysdep_routines += libc-do-syscall libpthread-shared-only-routines += libc-do-syscall + +# INLINE_SYSCALL uses the helper __libc_do_syscall in thumb mode. +CFLAGS-syscall_cancel.c += -marm endif ifeq ($(subdir),resolv) diff --git a/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h b/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h index d3313af..8132a95 100644 --- a/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h +++ b/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h @@ -16,6 +16,10 @@ License along with the GNU C Library. If not, see <http://www.gnu.org/licenses/>. */ +#ifndef _SIGCONTEXTINFO_H +#define _SIGCONTEXTINFO_H + +#include <stdint.h> #include <sys/ucontext.h> #define SIGCONTEXT siginfo_t *_si, ucontext_t * @@ -46,3 +50,11 @@ (act)->sa_flags |= SA_SIGINFO; \ (sigaction) (sig, act, oact); \ }) + +static inline uintptr_t +ucontext_get_pc (const ucontext_t *uc) +{ + return uc->uc_mcontext.arm_pc; +} + +#endif /* _SIGCONTEXTINFO_H */ -- 2.7.4
diff --git a/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h b/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h index d3313af..8132a95 100644 --- a/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h +++ b/sysdeps/unix/sysv/linux/arm/sigcontextinfo.h @@ -16,6 +16,10 @@ License along with the GNU C Library. If not, see <http://www.gnu.org/licenses/>. */ +#ifndef _SIGCONTEXTINFO_H +#define _SIGCONTEXTINFO_H + +#include <stdint.h> #include <sys/ucontext.h> #define SIGCONTEXT siginfo_t *_si, ucontext_t * @@ -46,3 +50,11 @@ (act)->sa_flags |= SA_SIGINFO; \ (sigaction) (sig, act, oact); \ }) + +static inline uintptr_t +ucontext_get_pc (const ucontext_t *uc) +{ + return uc->uc_mcontext.arm_pc; +} + +#endif /* _SIGCONTEXTINFO_H */ 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..5ae1412 --- /dev/null +++ b/sysdeps/unix/sysv/linux/arm/syscall_cancel.S @@ -0,0 +1,69 @@ +/* Cancellable syscall wrapper. Linux/arm 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 [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]) */ + + .syntax unified + +ENTRY (__syscall_cancel_arch) + .fnstart + mov ip,sp + stmfd sp!, {r4,r5,r6,r7,lr} + .save {r4,r5,r6,r7,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,r7,lr} + cfi_adjust_cfa_offset (-20); + bx lr + +1: + mov lr, pc + b __syscall_do_cancel + .fnend +END (__syscall_cancel_arch) +libc_hidden_def (__syscall_cancel_arch)
This patch adds the ARM modifications required for the BZ#12683. It basically adds the required ucontext_get_pc function and a arch specific syscall_cancel implementation. ARM requires an arch-specific syscall_cancel implementation because INTERNAL_SYSCALL_NCS may call an auxiliary symbol (__libc_do_syscall) for thumb which invalides the check on sigcancel_handler. Checked on arm-linux-gnueabihf. * sysdeps/unix/sysv/linux/arm/syscall_cancel.S: New file. * sysdeps/unix/sysv/linux/arm/sigcontextinfo.h (ucontext_get_pc): New function. Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> --- ChangeLog | 4 ++ sysdeps/unix/sysv/linux/arm/sigcontextinfo.h | 12 +++++ sysdeps/unix/sysv/linux/arm/syscall_cancel.S | 69 ++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 sysdeps/unix/sysv/linux/arm/syscall_cancel.S -- 2.7.4