This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 07/08] nptl: aarch64: Fix Race conditions in pthread cancellation (BZ#12683)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Szabolcs Nagy <szabolcs dot nagy at arm dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 29 Jun 2015 19:16:11 -0300
- Subject: Re: [PATCH 07/08] nptl: aarch64: Fix Race conditions in pthread cancellation (BZ#12683)
- Authentication-results: sourceware.org; auth=none
- References: <558DABCC dot 3020109 at linaro dot org> <55911ECA dot 2060205 at arm dot com>
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?