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

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)
Related show

Commit Message

Adhemerval Zanella Dec. 11, 2017, 7:06 p.m.
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

Comments

Phil Blundell Dec. 11, 2017, 8:16 p.m. | #1
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.
Andreas Schwab Dec. 11, 2017, 8:29 p.m. | #2
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."
Phil Blundell Dec. 11, 2017, 9:03 p.m. | #3
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.
Joseph Myers Dec. 11, 2017, 11:57 p.m. | #4
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
Adhemerval Zanella Dec. 12, 2017, 12:28 p.m. | #5
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.
Joseph Myers Dec. 12, 2017, 1:51 p.m. | #6
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
Adhemerval Zanella Dec. 12, 2017, 2:02 p.m. | #7
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

Patch

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)