[PATCH v3 18/21] nptl: s390: Fix Race conditions in pthread cancellation (BZ#12683)

Stefan Liebler stli@linux.ibm.com
Thu Oct 17 15:01:00 GMT 2019 

On 10/17/19 3:53 PM, Adhemerval Zanella wrote:
> 
> 
> On 16/10/2019 12:46, Stefan Liebler wrote:
>> Hi Adhemerval,
>>
>> I've added some notes below to the s390-64 file, but the same applies also for s390-32. I've also attached a diff.
>>
>> I've also recognized that a call starting from e.g. write () involves various shuffling of the argument registers at each level:
>> write (ARGS in r2-r4)
>> -> __syscall_cancel (r2=nr, ARGS in r3-r6 and two stack-slots)
>> --> __syscall_cancel_arch (r2=*ch, r3=nr, ARGS in r4-r6 and three stack-slots)
>> ---> "syscall-instruction" (ARGS in r2-r7)
>>
>> Just as a quick idea (I don't know if there are other limitations), those shuffling instructions could perhaps be omitted if the nr / ch arguments of the __syscall_cancel / __syscall_cancel_arch functions would be the last arguments instead of the first ones.
>> I assume that also other archs could benefit from such an ordering.
> 
> Thanks, I have applied your changes.  Indeed for some architectures the
> syscall_cancel.S might not be the most optimized one, I used the reference
> C implementation as base and gcc might not generate the best code in some
> cases.
> 
The implementation in syscall_cancel.S contains just one of the three 
parts of the mentioned register move instructions. Those needs also to 
be generated in write() and __syscall_cancel().
My point was, if we could preserve the values passed in registers to 
e.g. write() in the same registers until the syscall is invoked in 
__syscall_cancel_arch, we won't need to move them from registers to 
registers at all:
write (ARGS in r2-r4)
-> __syscall_cancel (ARGS in r2-r6 and first stack-slot, nr in second 
stack-slot)
--> __syscall_cacnel_arch (ARGS in r2-r6 and first stack-slot, nr in 
second stack-slot, ch in third stack-slot)
---> "syscall-instruction" (ARGS in r2-r7)

>>
>> Bye,
>> Stefan
>>
>> On 10/14/19 10:56 PM, Adhemerval Zanella wrote:
>>> This patch adds the s390 modifications required for the BZ#12683 fix
>>> by adding the arch-specific cancellation syscall bridge.
>>>
>>> Checked on s390-linux-gnu and s390x-linux-gnu.
>>> ---
>>>    .../sysv/linux/s390/s390-32/syscall_cancel.S  | 83 +++++++++++++++++++
>>>    .../sysv/linux/s390/s390-64/syscall_cancel.S  | 83 +++++++++++++++++++
>>>    2 files changed, 166 insertions(+)
>>>    create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>>    create mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>> new file mode 100644
>>> index 0000000000..3c934addbd
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/s390/s390-32/syscall_cancel.S
>>> @@ -0,0 +1,83 @@
>>> +/* Cancellable syscall wrapper.  Linux/s390 version.
>>> +   Copyright (C) 2019 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 __syscall_cancel_arch (int *cancelhandling,
>>> +                   __syscall_arg_t nr,
>>> +                   __syscall_arg_t arg1,
>>> +                   __syscall_arg_t arg2,
>>> +                   __syscall_arg_t arg3,
>>> +                   __syscall_arg_t arg4,
>>> +                   __syscall_arg_t arg5,
>>> +                   __syscall_arg_t arg6)  */
>>> +
>>> +ENTRY (__syscall_cancel_arch)
>>> +    stm    %r6,%r15,24(%r15)
>>> +    cfi_offset (6, -72)
>>> +    cfi_offset (7, -68)
>>> +    cfi_offset (8, -64)
>>> +    cfi_offset (9, -60)
>>> +    cfi_offset (10, -56)
>>> +    cfi_offset (11, -52)
>>> +    cfi_offset (12, -48)
>>> +    cfi_offset (13, -44)
>>> +    cfi_offset (14, -40)
>>> +    cfi_offset (15, -36)
>>> +    ahi    %r15, -96
>>> +    cfi_def_cfa_offset (192)
>>> +
>>> +    .globl __syscall_cancel_arch_start
>>> +    .type  __syscall_cancel_arch_start,@function
>>> +__syscall_cancel_arch_start:
>>> +    l    %r0, 0(%r2)
>>> +    tml    %r0, 4
>>> +    jne    1f
>>> +    lr    %r1, %r3
>>> +    lr    %r2, %r4
>>> +    lr    %r3, %r5
>>> +    lr    %r4, %r6
>>> +    l    %r5, 192(%r15)
>>> +    l    %r6, 196(%r15)
>>> +    l    %r7, 200(%r15)
>>> +    svc    0
>>> +
>>> +    .globl __syscall_cancel_arch_end
>>> +    .type  __syscall_cancel_arch_end,@function
>>> +__syscall_cancel_arch_end:
>>> +    l    %r4, 152(%r15)
>>> +    lm    %r6, %r15, 120(%r15)
>>> +    cfi_remember_state
>>> +    cfi_restore (15)
>>> +    cfi_restore (14)
>>> +    cfi_restore (13)
>>> +    cfi_restore (12)
>>> +    cfi_restore (11)
>>> +    cfi_restore (10)
>>> +    cfi_restore (9)
>>> +    cfi_restore (8)
>>> +    cfi_restore (7)
>>> +    cfi_restore (6)
>>> +    cfi_def_cfa_offset (96)
>>> +    br    %r4
>>> +
>>> +1:
>>> +    cfi_restore_state
>>> +    brasl    %r14, __syscall_do_cancel
>>> +END (__syscall_cancel_arch)
>>> +libc_hidden_def (__syscall_cancel_arch)
>>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>> new file mode 100644
>>> index 0000000000..3480020fbb
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/s390/s390-64/syscall_cancel.S
>>> @@ -0,0 +1,83 @@
>>> +/* Cancellable syscall wrapper.  Linux/s390x version.
>>> +   Copyright (C) 2019 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 __syscall_cancel_arch (int *cancelhandling,
>>> +                   __syscall_arg_t nr,
>>> +                   __syscall_arg_t arg1,
>>> +                   __syscall_arg_t arg2,
>>> +                   __syscall_arg_t arg3,
>>> +                   __syscall_arg_t arg4,
>>> +                   __syscall_arg_t arg5,
>>> +                   __syscall_arg_t arg6)  */
>>> +
>>> +ENTRY (__syscall_cancel_arch)
>>> +    stmg    %r6, %r15, 48(%r15)
>> Please omit the spaces between the operands to have the same style as below (the same applies to multiple instructions in the s390-32 file.
>>
> 
> Ack.
> 
>>> +    cfi_offset (6, -112)
>> Please use e.g. %r6 instead of just 6 for the cfi statements here and below for cfi_restore.
>>> +    cfi_offset (7, -104)
>>> +    cfi_offset (8, -96)
>>> +    cfi_offset (9, -88)
>>> +    cfi_offset (10, -80)
>>> +    cfi_offset (11, -72)
>>> +    cfi_offset (12, -64)
>>> +    cfi_offset (13, -56)
>>> +    cfi_offset (14, -48)
>>> +    cfi_offset (15, -40)
>>> +    aghi    %r15,-160
>>> +    cfi_def_cfa_offset (320)
>> I think the new stack frame is not needed at all.  The kernel does not clobber any registers and also does not need an own stack frame.
>> (I've recognized that syscall.S also contains an extra frame and I will work on a patch)
>>
>> Then r8-r15 are not clobbered here and we just have to store and restore r6 and r7.
> 
> ack.
> 
>>> +
>>> +    .globl __syscall_cancel_arch_start
>>> +    .type  __syscall_cancel_arch_start,@function
>>> +__syscall_cancel_arch_start:
>> Could you add the comments like e.g. done in the x86_64 patch?
>>      /* if (*cancelhandling & CANCELED_BITMASK)
>>           __syscall_do_cancel()  */
> 
> Certainly, I also changes to use TCB_CANCELED_BITMASK from tcb-offsets.h
> instead of hard-code the values.
> 
>>> +    l    %r0,0(%r2)
>>> +    tmll    %r0,4
>> We can combine l and tmll here:
>> tm 3(%r2),4
>>
> 
> Ack.
> 
>>> +    jne    1f
>>      /* Issue a 6 argument syscall, the nr [%r1] being the syscall
>>         number.  */
>>> +    lgr    %r1,%r3
>>> +    lgr    %r2,%r4
>>> +    lgr    %r3,%r5
>>> +    lgr    %r4,%r6
>>> +    lg    %r5,320(%r15)
>>> +    lg    %r6,328(%r15)
>>> +    lg    %r7,336(%r15)
>> We can use lmg in order to load r5-r7.
> 
> Ack.
> 
>>> +    svc    0
>>> +
>>> +    .globl __syscall_cancel_arch_end
>>> +    .type  __syscall_cancel_arch_end,@function
>>> +__syscall_cancel_arch_end:
>>> +    lg    %r4,272(%r15)
>>> +    lmg    %r6,%r15,208(%r15)
>>> +    cfi_remember_state
>>> +    cfi_restore (15)
>>> +    cfi_restore (14)
>>> +    cfi_restore (13)
>>> +    cfi_restore (12)
>>> +    cfi_restore (11)
>>> +    cfi_restore (10)
>>> +    cfi_restore (9)
>>> +    cfi_restore (8)
>>> +    cfi_restore (7)
>>> +    cfi_restore (6)
>>> +    cfi_def_cfa_offset (160)
>>> +    br    %r4
>>> +
>>> +1:
>>> +    cfi_restore_state
>>> +    brasl    %r14, __syscall_do_cancel
>> We can just use jg __syscall_do_cancel for this not-returning-tail-call.
>> (For the s390-32 part: Both instructions are z900 instructions, but as gcc has removed support for g5 and g6, this is okay at this point of time)
>>> +END (__syscall_cancel_arch)
>>> +libc_hidden_def (__syscall_cancel_arch)
>>>
>>
> 
> Ack.
>

More information about the Libc-alpha mailing list