[v5,07/22] powerpc: Fix Race conditions in pthread cancellation [BZ#12683]

Message ID 20230410195907.4123869-8-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Fix Race conditions in pthread cancellation [BZ#12683] |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Netto April 10, 2023, 7:58 p.m. UTC
  By adding the required syscall_cancel.S.

Checked on powerpc64le-linux-gnu, powerpc64-linux-gnu and
powerpc-linux-gnu.
---
 sysdeps/powerpc/powerpc32/sysdep.h            |  3 +
 sysdeps/powerpc/powerpc64/sysdep.h            | 19 ++++++
 .../unix/sysv/linux/powerpc/syscall_cancel.S  | 65 +++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/syscall_cancel.S
  

Comments

Paul E. Murphy April 14, 2023, 3:27 p.m. UTC | #1
On 4/10/23 2:58 PM, Adhemerval Zanella via Libc-alpha wrote:
> By adding the required syscall_cancel.S.
> 
> Checked on powerpc64le-linux-gnu, powerpc64-linux-gnu and
> powerpc-linux-gnu.
> ---
Thanks. A couple trivial comments, but otherwise LGTM.

Reviewed-by: Paul E. Murphy <murphyp@linux.ibm.com>

> +ENTRY (__syscall_cancel_arch)
> +
> +	.globl __syscall_cancel_arch_start
> +__syscall_cancel_arch_start:
> +
> +	/* if (*cancelhandling & CANCELED_BITMASK)
> +	     __syscall_do_cancel()  */
> +	lwz     r0,0(r3)
> +	andi.   r0,r0,TCB_CANCELED_BITMASK
> +	bne-    1f
Really trivial opinion nit, the branch hint should be removed.

> +
> +	/* Issue a 6 argument syscall, the nr [r4] being the syscall
> +	   number.  */
> +	mr      r0,r4
> +	mr      r3,r5
> +	mr      r4,r6
> +	mr      r5,r7
> +	mr      r6,r8
> +	mr      r7,r9
> +	mr      r8,r10
> +	sc
For consistency with the other syscall usage, can this also use the 
appropriate wrappers for using scv?

> +
> +	.globl __syscall_cancel_arch_end
> +__syscall_cancel_arch_end:
> +
> +	bnslr+
> +	neg	r3,r3
> +	blr
> +
> +	/* Although the __syscall_do_cancel do not return, we need to stack
> +	   being set correctly for unwind.  */
> +1:
> +	TAIL_CALL_NO_RETURN (__syscall_do_cancel
  
Adhemerval Zanella Netto April 27, 2023, 11:13 a.m. UTC | #2
On 14/04/23 12:27, Paul E Murphy wrote:
> 
> 
> On 4/10/23 2:58 PM, Adhemerval Zanella via Libc-alpha wrote:
>> By adding the required syscall_cancel.S.
>>
>> Checked on powerpc64le-linux-gnu, powerpc64-linux-gnu and
>> powerpc-linux-gnu.
>> ---
> Thanks. A couple trivial comments, but otherwise LGTM.
> 
> Reviewed-by: Paul E. Murphy <murphyp@linux.ibm.com>
> 
>> +ENTRY (__syscall_cancel_arch)
>> +
>> +    .globl __syscall_cancel_arch_start
>> +__syscall_cancel_arch_start:
>> +
>> +    /* if (*cancelhandling & CANCELED_BITMASK)
>> +         __syscall_do_cancel()  */
>> +    lwz     r0,0(r3)
>> +    andi.   r0,r0,TCB_CANCELED_BITMASK
>> +    bne-    1f
> Really trivial opinion nit, the branch hint should be removed.

Ack.

> 
>> +
>> +    /* Issue a 6 argument syscall, the nr [r4] being the syscall
>> +       number.  */
>> +    mr      r0,r4
>> +    mr      r3,r5
>> +    mr      r4,r6
>> +    mr      r5,r7
>> +    mr      r6,r8
>> +    mr      r7,r9
>> +    mr      r8,r10
>> +    sc
> For consistency with the other syscall usage, can this also use the appropriate wrappers for using scv?

That is tricky for similar reasons i386 and i64 have to use the old
syscall mechanism instead of the vDSO: the cancel syscall bridge 
need to have a mark just after the syscall instruction to know
whether it has any side-effects.  Having two syscall mechanism means
essentially two marks depending of the syscall used.

I think it should be possible to two two marks for powerpc, one
for default 'sc' and another for 'svc'. It would require an arch-specific
cancellation-pc-check.h for powerpc.

> 
>> +
>> +    .globl __syscall_cancel_arch_end
>> +__syscall_cancel_arch_end:
>> +
>> +    bnslr+
>> +    neg    r3,r3
>> +    blr
>> +
>> +    /* Although the __syscall_do_cancel do not return, we need to stack
>> +       being set correctly for unwind.  */
>> +1:
>> +    TAIL_CALL_NO_RETURN (__syscall_do_cancel
  

Patch

diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h
index 095a726765..df67e3516a 100644
--- a/sysdeps/powerpc/powerpc32/sysdep.h
+++ b/sysdeps/powerpc/powerpc32/sysdep.h
@@ -104,6 +104,9 @@  GOT_LABEL:			;					      \
 # define JUMPTARGET(name) name
 #endif
 
+#define TAIL_CALL_NO_RETURN(__func) \
+    b __func@local
+
 #if defined SHARED && defined PIC && !defined NO_HIDDEN
 # undef HIDDEN_JUMPTARGET
 # define HIDDEN_JUMPTARGET(name) __GI_##name##@local
diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
index ce92d8b3d2..1815131dc2 100644
--- a/sysdeps/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/powerpc/powerpc64/sysdep.h
@@ -352,6 +352,25 @@  LT_LABELSUFFIX(name,_name_end): ; \
   ENTRY (name);					\
   DO_CALL (SYS_ify (syscall_name))
 
+#ifdef SHARED
+# define TAIL_CALL_NO_RETURN(__func) \
+    b JUMPTARGET(__func)
+#else
+# define TAIL_CALL_NO_RETURN(__func) \
+    .ifdef .Local ## __func; \
+    b .Local ## __func; \
+    .else; \
+.Local ## __func: \
+    mflr 0; \
+    std 0,FRAME_LR_SAVE(1); \
+    stdu 1,-FRAME_MIN_SIZE(1); \
+    cfi_adjust_cfa_offset(FRAME_MIN_SIZE); \
+    cfi_offset(lr,FRAME_LR_SAVE); \
+    bl JUMPTARGET(__func); \
+    nop; \
+    .endif
+#endif
+
 #ifdef SHARED
 #define TAIL_CALL_SYSCALL_ERROR \
     b JUMPTARGET (NOTOC (__syscall_error))
diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall_cancel.S b/sysdeps/unix/sysv/linux/powerpc/syscall_cancel.S
new file mode 100644
index 0000000000..7aa2b77caa
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/syscall_cancel.S
@@ -0,0 +1,65 @@ 
+/* Cancellable syscall wrapper.  Linux/powerpc version.
+   Copyright (C) 2023 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>
+#include <descr-const.h>
+
+/* long int [r3] __syscall_cancel_arch (int *cancelhandling [r3],
+					long int nr   [r4],
+					long int arg1 [r5],
+					long int arg2 [r6],
+					long int arg3 [r7],
+					long int arg4 [r8],
+					long int arg5 [r9],
+					long int arg6 [r10])  */
+
+ENTRY (__syscall_cancel_arch)
+
+	.globl __syscall_cancel_arch_start
+__syscall_cancel_arch_start:
+
+	/* if (*cancelhandling & CANCELED_BITMASK)
+	     __syscall_do_cancel()  */
+	lwz     r0,0(r3)
+	andi.   r0,r0,TCB_CANCELED_BITMASK
+	bne-    1f
+
+	/* Issue a 6 argument syscall, the nr [r4] being the syscall
+	   number.  */
+	mr      r0,r4
+	mr      r3,r5
+	mr      r4,r6
+	mr      r5,r7
+	mr      r6,r8
+	mr      r7,r9
+	mr      r8,r10
+	sc
+
+	.globl __syscall_cancel_arch_end
+__syscall_cancel_arch_end:
+
+	bnslr+
+	neg	r3,r3
+	blr
+
+	/* Although the __syscall_do_cancel do not return, we need to stack
+	   being set correctly for unwind.  */
+1:
+	TAIL_CALL_NO_RETURN (__syscall_do_cancel)
+
+END (__syscall_cancel_arch)