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

  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
  

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".