[08/19] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683)
Commit Message
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
Comments
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.
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.
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".
@@ -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 */
new file mode 100644
@@ -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)