[PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

Tulio Magno Quites Machado Filho
The tunables framework needs to make syscall early during process
initialization, before the TCB is available for consumption.  This
behavior conflicts with powerpc{|64|64le}'s lock elision code, that
tries to abort transactions before a syscall when lock elision is
available and enabled.

This patch adds the macro EARLY_INTERNAL_SYSCALL in order to let early
syscalls happen without depending on the TCB initialization for
powerpc{|64|64le}.  Other architectures are redirected to INTERNAL_SYSCALL.

Tested on powerpc{|64|64le}, s390x and x86_64.

2018-01-11  Tulio Magno Quites Machado Filho  <[hidden email]>

        [BZ #22685]
        * sysdeps/unix/sysdep.h (__EARLY_INTERNAL_SYSCALL0,
        __EARLY_INTERNAL_SYSCALL1, __EARLY_INTERNAL_SYSCALL2,
        __EARLY_INTERNAL_SYSCALL3, __EARLY_INTERNAL_SYSCALL4,
        __EARLY_INTERNAL_SYSCALL5, __EARLY_INTERNAL_SYSCALL6,
        __EARLY_INTERNAL_SYSCALL7, EARLY_INTERNAL_SYSCALL_CALL): New macros
        (EARLY_INTERNAL_SYSCALL): New macro.  Redirect to
        INTERNAL_SYSCALL by default.
        * sysdeps/unix/sysv/linux/not-errno.h (__access_noerrno):
        Replace INTERNAL_SYSCALL_CALL with EARLY_INTERNAL_SYSCALL_CALL.
        * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
        (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): New macros.
        * sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
        (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): Likewise.

Signed-off-by: Tulio Magno Quites Machado Filho <[hidden email]>
---
 sysdeps/unix/sysdep.h                              | 25 +++++++++++++++
 sysdeps/unix/sysv/linux/not-errno.h                |  4 +--
 sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h | 36 +++++++++++++++++++++-
 sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h | 34 +++++++++++++++++++-
 4 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysdep.h b/sysdeps/unix/sysdep.h
index aac9303..689272e 100644
--- a/sysdeps/unix/sysdep.h
+++ b/sysdeps/unix/sysdep.h
@@ -57,6 +57,31 @@
 #define INTERNAL_SYSCALL_CALL(...) \
   __INTERNAL_SYSCALL_DISP (__INTERNAL_SYSCALL, __VA_ARGS__)
 
+#define __EARLY_INTERNAL_SYSCALL0(name, err) \
+  EARLY_INTERNAL_SYSCALL (name, err, 0)
+#define __EARLY_INTERNAL_SYSCALL1(name, err, a1) \
+  EARLY_INTERNAL_SYSCALL (name, err, 1, a1)
+#define __EARLY_INTERNAL_SYSCALL2(name, err, a1, a2) \
+  EARLY_INTERNAL_SYSCALL (name, err, 2, a1, a2)
+#define __EARLY_INTERNAL_SYSCALL3(name, err, a1, a2, a3) \
+  EARLY_INTERNAL_SYSCALL (name, err, 3, a1, a2, a3)
+#define __EARLY_INTERNAL_SYSCALL4(name, err, a1, a2, a3, a4) \
+  EARLY_INTERNAL_SYSCALL (name, err, 4, a1, a2, a3, a4)
+#define __EARLY_INTERNAL_SYSCALL5(name, err, a1, a2, a3, a4, a5) \
+  EARLY_INTERNAL_SYSCALL (name, err, 5, a1, a2, a3, a4, a5)
+#define __EARLY_INTERNAL_SYSCALL6(name, err, a1, a2, a3, a4, a5, a6) \
+  EARLY_INTERNAL_SYSCALL (name, err, 6, a1, a2, a3, a4, a5, a6)
+#define __EARLY_INTERNAL_SYSCALL7(name, err, a1, a2, a3, a4, a5, a6, a7) \
+  EARLY_INTERNAL_SYSCALL (name, err, 7, a1, a2, a3, a4, a5, a6, a7)
+
+/* It is similar to INTERNAL_SYSCALL_CALL, but it is reserved to system calls
+   during process initialization, when internal structures may not be
+   available, e.g. TCB on powerpc.  */
+#define EARLY_INTERNAL_SYSCALL_CALL(...) \
+  __INTERNAL_SYSCALL_DISP (__EARLY_INTERNAL_SYSCALL, __VA_ARGS__)
+#define EARLY_INTERNAL_SYSCALL(name, err, nr, args...) \
+  INTERNAL_SYSCALL (name, err, nr, args)
+
 #define __INLINE_SYSCALL0(name) \
   INLINE_SYSCALL (name, 0)
 #define __INLINE_SYSCALL1(name, a1) \
diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h
index 106ba5c..a033b7b 100644
--- a/sysdeps/unix/sysv/linux/not-errno.h
+++ b/sysdeps/unix/sysv/linux/not-errno.h
@@ -25,9 +25,9 @@ __access_noerrno (const char *pathname, int mode)
   int res;
   INTERNAL_SYSCALL_DECL (err);
 #ifdef __NR_access
-  res = INTERNAL_SYSCALL_CALL (access, err, pathname, mode);
+  res = EARLY_INTERNAL_SYSCALL_CALL (access, err, pathname, mode);
 #else
-  res = INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, pathname, mode);
+  res = EARLY_INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, pathname, mode);
 #endif
   if (INTERNAL_SYSCALL_ERROR_P (res, err))
     return INTERNAL_SYSCALL_ERRNO (res, err);
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
index f7277d5..efc6cbc 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
@@ -89,7 +89,10 @@
    On powerpc a system call basically clobbers the same registers like a
    function call, with the exception of LR (which is needed for the
    "sc; bnslr+" sequence) and CR (where only CR0.SO is clobbered to signal
-   an error return status).  */
+   an error return status).
+
+   Notice it requires the TCB to be allocated and completely set in order to
+   abort transactions before the syscall.  */
 
 # undef INTERNAL_SYSCALL_DECL
 # define INTERNAL_SYSCALL_DECL(err) long int err __attribute__ ((unused))
@@ -124,6 +127,37 @@
 # define INTERNAL_SYSCALL(name, err, nr, args...) \
   INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, ##args)
 
+/* Similar to INTERNAL_SYSCALL, but reserved to early process initialization
+   without requiring the TCB to allocated and completely set.  */
+# undef EARLY_INTERNAL_SYSCALL
+# define EARLY_INTERNAL_SYSCALL_NCS(name, err, nr, args...) \
+  ({ \
+    register long int r0  __asm__ ("r0"); \
+    register long int r3  __asm__ ("r3"); \
+    register long int r4  __asm__ ("r4"); \
+    register long int r5  __asm__ ("r5"); \
+    register long int r6  __asm__ ("r6"); \
+    register long int r7  __asm__ ("r7"); \
+    register long int r8  __asm__ ("r8"); \
+    register long int r9  __asm__ ("r9"); \
+    register long int r10 __asm__ ("r10"); \
+    register long int r11 __asm__ ("r11"); \
+    register long int r12 __asm__ ("r12"); \
+    LOADARGS_##nr(name, args); \
+    __asm__ __volatile__ \
+      ("sc   \n\t" \
+       "mfcr %0" \
+       : "=&r" (r0), \
+ "=&r" (r3), "=&r" (r4), "=&r" (r5),  "=&r" (r6),  "=&r" (r7), \
+ "=&r" (r8), "=&r" (r9), "=&r" (r10), "=&r" (r11), "=&r" (r12) \
+       : ASM_INPUT_##nr \
+       : "cr0", "ctr", "memory"); \
+    err = r0; \
+    (int) r3; \
+  })
+# define EARLY_INTERNAL_SYSCALL(name, err, nr, args...) \
+  EARLY_INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, ##args)
+
 # undef INTERNAL_SYSCALL_ERROR_P
 # define INTERNAL_SYSCALL_ERROR_P(val, err) \
   ((void) (val), __builtin_expect ((err) & (1 << 28), 0))
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
index 0fc179a..d431fdb 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
@@ -118,7 +118,10 @@
    call. This use is for internal calls that do not need to handle errors
    normally. It will never touch errno. This returns just what the kernel
    gave back in the non-error (CR0.SO cleared) case, otherwise (CR0.SO set)
-   the negation of the return value in the kernel gets reverted.  */
+   the negation of the return value in the kernel gets reverted.
+
+   Notice it requires the TCB to be allocated and completely set in order to
+   abort transactions before the syscall.  */
 
 #undef INTERNAL_SYSCALL
 #define INTERNAL_SYSCALL_NCS(name, err, nr, args...) \
@@ -148,6 +151,35 @@
 #define INTERNAL_SYSCALL(name, err, nr, args...) \
   INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)
 
+/* Similar to INTERNAL_SYSCALL, but reserved to early process initialization
+   without requiring the TCB to allocated and completely set.  */
+#undef EARLY_INTERNAL_SYSCALL
+#define EARLY_INTERNAL_SYSCALL_NCS(name, err, nr, args...) \
+  ({ \
+    register long int r0  __asm__ ("r0"); \
+    register long int r3  __asm__ ("r3"); \
+    register long int r4  __asm__ ("r4"); \
+    register long int r5  __asm__ ("r5"); \
+    register long int r6  __asm__ ("r6"); \
+    register long int r7  __asm__ ("r7"); \
+    register long int r8  __asm__ ("r8"); \
+    LOADARGS_##nr (name, ##args); \
+    __asm__ __volatile__ \
+      ("sc\n\t" \
+       "mfcr  %0\n\t" \
+       "0:" \
+       : "=&r" (r0), \
+         "=&r" (r3), "=&r" (r4), "=&r" (r5), \
+         "=&r" (r6), "=&r" (r7), "=&r" (r8) \
+       : ASM_INPUT_##nr \
+       : "r9", "r10", "r11", "r12", \
+         "cr0", "ctr", "memory"); \
+    err = r0; \
+    r3; \
+  })
+#define EARLY_INTERNAL_SYSCALL(name, err, nr, args...) \
+  EARLY_INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)
+
 #undef INTERNAL_SYSCALL_DECL
 #define INTERNAL_SYSCALL_DECL(err) long int err __attribute__ ((unused))
 
--
2.9.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

Aurelien Jarno
On 2018-01-11 17:27, Tulio Magno Quites Machado Filho wrote:

> The tunables framework needs to make syscall early during process
> initialization, before the TCB is available for consumption.  This
> behavior conflicts with powerpc{|64|64le}'s lock elision code, that
> tries to abort transactions before a syscall when lock elision is
> available and enabled.
>
> This patch adds the macro EARLY_INTERNAL_SYSCALL in order to let early
> syscalls happen without depending on the TCB initialization for
> powerpc{|64|64le}.  Other architectures are redirected to INTERNAL_SYSCALL.
>
> Tested on powerpc{|64|64le}, s390x and x86_64.
>
> 2018-01-11  Tulio Magno Quites Machado Filho  <[hidden email]>
>
> [BZ #22685]
> * sysdeps/unix/sysdep.h (__EARLY_INTERNAL_SYSCALL0,
> __EARLY_INTERNAL_SYSCALL1, __EARLY_INTERNAL_SYSCALL2,
> __EARLY_INTERNAL_SYSCALL3, __EARLY_INTERNAL_SYSCALL4,
> __EARLY_INTERNAL_SYSCALL5, __EARLY_INTERNAL_SYSCALL6,
> __EARLY_INTERNAL_SYSCALL7, EARLY_INTERNAL_SYSCALL_CALL): New macros
> (EARLY_INTERNAL_SYSCALL): New macro.  Redirect to
> INTERNAL_SYSCALL by default.
> * sysdeps/unix/sysv/linux/not-errno.h (__access_noerrno):
> Replace INTERNAL_SYSCALL_CALL with EARLY_INTERNAL_SYSCALL_CALL.
> * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
> (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): New macros.
> * sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
> (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): Likewise.
>
> Signed-off-by: Tulio Magno Quites Machado Filho <[hidden email]>
> ---
>  sysdeps/unix/sysdep.h                              | 25 +++++++++++++++
>  sysdeps/unix/sysv/linux/not-errno.h                |  4 +--
>  sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h | 36 +++++++++++++++++++++-
>  sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h | 34 +++++++++++++++++++-
>  4 files changed, 95 insertions(+), 4 deletions(-)

Thanks for the patch, I have just tested it and I confirm it fixes the
issue.

Tested-by: Aurelien Jarno <[hidden email]>

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[hidden email]                 http://www.aurel32.net
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

Adhemerval Zanella-2


On 11/01/2018 19:36, Aurelien Jarno wrote:

> On 2018-01-11 17:27, Tulio Magno Quites Machado Filho wrote:
>> The tunables framework needs to make syscall early during process
>> initialization, before the TCB is available for consumption.  This
>> behavior conflicts with powerpc{|64|64le}'s lock elision code, that
>> tries to abort transactions before a syscall when lock elision is
>> available and enabled.
>>
>> This patch adds the macro EARLY_INTERNAL_SYSCALL in order to let early
>> syscalls happen without depending on the TCB initialization for
>> powerpc{|64|64le}.  Other architectures are redirected to INTERNAL_SYSCALL.
>>
>> Tested on powerpc{|64|64le}, s390x and x86_64.

I am not really understanding why exactly this is failing because the only
object that currently uses __access_noerrno, dl-tunables.os, is built with
-DMODULE_NAME=rtld and thus ABORT_TRANSACTION should be an empty statement.
 

>>
>> 2018-01-11  Tulio Magno Quites Machado Filho  <[hidden email]>
>>
>> [BZ #22685]
>> * sysdeps/unix/sysdep.h (__EARLY_INTERNAL_SYSCALL0,
>> __EARLY_INTERNAL_SYSCALL1, __EARLY_INTERNAL_SYSCALL2,
>> __EARLY_INTERNAL_SYSCALL3, __EARLY_INTERNAL_SYSCALL4,
>> __EARLY_INTERNAL_SYSCALL5, __EARLY_INTERNAL_SYSCALL6,
>> __EARLY_INTERNAL_SYSCALL7, EARLY_INTERNAL_SYSCALL_CALL): New macros
>> (EARLY_INTERNAL_SYSCALL): New macro.  Redirect to
>> INTERNAL_SYSCALL by default.
>> * sysdeps/unix/sysv/linux/not-errno.h (__access_noerrno):
>> Replace INTERNAL_SYSCALL_CALL with EARLY_INTERNAL_SYSCALL_CALL.
>> * sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h
>> (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): New macros.
>> * sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
>> (EARLY_INTERNAL_SYSCALL_NCS, EARLY_INTERNAL_SYSCALL): Likewise.
>>
>> Signed-off-by: Tulio Magno Quites Machado Filho <[hidden email]>
>> ---
>>  sysdeps/unix/sysdep.h                              | 25 +++++++++++++++
>>  sysdeps/unix/sysv/linux/not-errno.h                |  4 +--
>>  sysdeps/unix/sysv/linux/powerpc/powerpc32/sysdep.h | 36 +++++++++++++++++++++-
>>  sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h | 34 +++++++++++++++++++-
>>  4 files changed, 95 insertions(+), 4 deletions(-)
>
> Thanks for the patch, I have just tested it and I confirm it fixes the
> issue.
>
> Tested-by: Aurelien Jarno <[hidden email]>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

Florian Weimer-5
On 01/11/2018 11:40 PM, Adhemerval Zanella wrote:
> I am not really understanding why exactly this is failing because the only
> object that currently uses __access_noerrno, dl-tunables.os, is built with
> -DMODULE_NAME=rtld and thus ABORT_TRANSACTION should be an empty statement.

elf/dl-tunables.o uses it as well, and is built with -DMODULE_NAME=libc.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] powerpc: Fix syscalls during early process initialization [BZ #22685]

Adhemerval Zanella-2


On 11/01/2018 21:42, Florian Weimer wrote:
> On 01/11/2018 11:40 PM, Adhemerval Zanella wrote:
>> I am not really understanding why exactly this is failing because the only
>> object that currently uses __access_noerrno, dl-tunables.os, is built with
>> -DMODULE_NAME=rtld and thus ABORT_TRANSACTION should be an empty statement.
>
> elf/dl-tunables.o uses it as well, and is built with -DMODULE_NAME=libc.
Right, and if I recall correctly thread register is an undefined position at
the time. The only regard I have is adding some specific syscall tinkering
due a very specific arch/os requirement. Wouldn't be better to just
reimplement __access_noerrno/not-errno.h for powerpc?
Reply | Threaded
Open this post in threaded view
|

[PATCHv2] powerpc: Fix syscalls during early process initialization [BZ #22685]

Tulio Magno Quites Machado Filho
Changes since v1:
 - Re-implemented the patch to re-define a powerpc-specific
   __access_noerrno.

--- 8< ---

The tunables framework needs to execute syscall early in process
initialization, before the TCB is available for consumption.  This
behavior conflicts with powerpc{|64|64le}'s lock elision code, that
checks the TCB before trying to abort transactions immediately before
executing a syscall.

This patch adds a powerpc-specific implementation of __access_noerrno
that does not abort transactions before the executing syscall.

Tested on powerpc{|64|64le}.

2018-01-12  Tulio Magno Quites Machado Filho  <[hidden email]>

        [BZ #22685]
        * sysdeps/unix/sysv/linux/powerpc/not-errno.h: New file.  Reuse
        Linux code, but remove the code that aborts transactions.

Signed-off-by: Tulio Magno Quites Machado Filho <[hidden email]>
Tested-by: Aurelien Jarno <[hidden email]>
---
 sysdeps/unix/sysv/linux/powerpc/not-errno.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/not-errno.h

diff --git a/sysdeps/unix/sysv/linux/powerpc/not-errno.h b/sysdeps/unix/sysv/linux/powerpc/not-errno.h
new file mode 100644
index 0000000..642cb8a
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/not-errno.h
@@ -0,0 +1,25 @@
+/* Syscall wrapper that do not set errno.  Linux powerpc version.
+   Copyright (C) 2018 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/>.  */
+
+/* __access_noerrno is used during process initialization in elf/dl-tunables.c
+   before the TCB is initialized, prohibiting the usage of
+   ABORT_TRANSACTION.  */
+#undef ABORT_TRANSACTION
+#define ABORT_TRANSACTION
+
+#include "sysdeps/unix/sysv/linux/not-errno.h"
--
2.9.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2] powerpc: Fix syscalls during early process initialization [BZ #22685]

Florian Weimer-5
On 01/12/2018 05:07 PM, Tulio Magno Quites Machado Filho wrote:
> +/* __access_noerrno is used during process initialization in elf/dl-tunables.c
> +   before the TCB is initialized, prohibiting the usage of
> +   ABORT_TRANSACTION.  */
> +#undef ABORT_TRANSACTION
> +#define ABORT_TRANSACTION
> +
> +#include "sysdeps/unix/sysv/linux/not-errno.h"

This #define isn't properly scoped, so I really don't like this approach
for a header file.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2] powerpc: Fix syscalls during early process initialization [BZ #22685]

Tulio Magno Quites Machado Filho
Florian Weimer <[hidden email]> writes:

> On 01/12/2018 05:07 PM, Tulio Magno Quites Machado Filho wrote:
>> +/* __access_noerrno is used during process initialization in elf/dl-tunables.c
>> +   before the TCB is initialized, prohibiting the usage of
>> +   ABORT_TRANSACTION.  */
>> +#undef ABORT_TRANSACTION
>> +#define ABORT_TRANSACTION
>> +
>> +#include "sysdeps/unix/sysv/linux/not-errno.h"
>
> This #define isn't properly scoped, so I really don't like this approach
> for a header file.

Do you think that checking for TUNABLES_INTERNAL would be enough?

--
Tulio Magno

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv2] powerpc: Fix syscalls during early process initialization [BZ #22685]

Florian Weimer-5
On 01/12/2018 05:40 PM, Tulio Magno Quites Machado Filho wrote:

> Florian Weimer <[hidden email]> writes:
>
>> On 01/12/2018 05:07 PM, Tulio Magno Quites Machado Filho wrote:
>>> +/* __access_noerrno is used during process initialization in elf/dl-tunables.c
>>> +   before the TCB is initialized, prohibiting the usage of
>>> +   ABORT_TRANSACTION.  */
>>> +#undef ABORT_TRANSACTION
>>> +#define ABORT_TRANSACTION
>>> +
>>> +#include "sysdeps/unix/sysv/linux/not-errno.h"
>>
>> This #define isn't properly scoped, so I really don't like this approach
>> for a header file.
>
> Do you think that checking for TUNABLES_INTERNAL would be enough?

I'm not sure what you mean.

If you move the current definition of ABORT_TRANSACTION to
ABORT_TRANSACTION_IMPL, you could perhaps do

#undef ABORT_TRANSACTION
#define ABORT_TRANSACTION

#include "sysdeps/unix/sysv/linux/not-errno.h"

#undef ABORT_TRANSACTION
#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL

(with comments, obviously).

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

[PATCHv3] powerpc: Fix syscalls during early process initialization [BZ #22685]

Tulio Magno Quites Machado Filho
Changes since v2:
 - Limited the scope of the changes to ABORT_TRANSACTION.

Changes since v1:
 - Re-implemented the patch to re-define a powerpc-specific
   __access_noerrno.

--- 8< ---

The tunables framework needs to execute syscall early in process
initialization, before the TCB is available for consumption.  This
behavior conflicts with powerpc{|64|64le}'s lock elision code, that
checks the TCB before trying to abort transactions immediately before
executing a syscall.

This patch adds a powerpc-specific implementation of __access_noerrno
that does not abort transactions before the executing syscall.

Tested on powerpc{|64|64le}.

2018-01-12  Tulio Magno Quites Machado Filho  <[hidden email]>

        [BZ #22685]
        * sysdeps/powerpc/powerpc32/sysdep.h (ABORT_TRANSACTION_IMPL): Renamed
        from ABORT_TRANSACTION.
        (ABORT_TRANSACTION): Redirect to ABORT_TRANSACTION_IMPL.
        * sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION,
        ABORT_TRANSACTION_IMPL): Likewise.
        * sysdeps/unix/sysv/linux/powerpc/not-errno.h: New file.  Reuse
        Linux code, but remove the code that aborts transactions.

Signed-off-by: Tulio Magno Quites Machado Filho <[hidden email]>
Tested-by: Aurelien Jarno <[hidden email]>
---
 sysdeps/powerpc/powerpc32/sysdep.h          |  5 +++--
 sysdeps/powerpc/powerpc64/sysdep.h          |  5 +++--
 sysdeps/unix/sysv/linux/powerpc/not-errno.h | 30 +++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/powerpc/not-errno.h

diff --git a/sysdeps/powerpc/powerpc32/sysdep.h b/sysdeps/powerpc/powerpc32/sysdep.h
index 62c018f..8e32a2a 100644
--- a/sysdeps/powerpc/powerpc32/sysdep.h
+++ b/sysdeps/powerpc/powerpc32/sysdep.h
@@ -91,7 +91,7 @@ GOT_LABEL: ;      \
   ASM_SIZE_DIRECTIVE(name)
 
 #if ! IS_IN(rtld)
-# define ABORT_TRANSACTION \
+# define ABORT_TRANSACTION_IMPL \
     cmpwi    2,0; \
     beq      1f; \
     lwz      0,TM_CAPABLE(2); \
@@ -102,8 +102,9 @@ GOT_LABEL: ;      \
     .align 4; \
 1:
 #else
-# define ABORT_TRANSACTION
+# define ABORT_TRANSACTION_IMPL
 #endif
+#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
 
 #define DO_CALL(syscall)      \
     ABORT_TRANSACTION      \
diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
index 26b0885..2df1d9b 100644
--- a/sysdeps/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/powerpc/powerpc64/sysdep.h
@@ -264,7 +264,7 @@ LT_LABELSUFFIX(name,_name_end): ; \
   END_2(name)
 
 #if !IS_IN(rtld)
-# define ABORT_TRANSACTION \
+# define ABORT_TRANSACTION_IMPL \
     cmpdi    13,0; \
     beq      1f; \
     lwz      0,TM_CAPABLE(13); \
@@ -275,8 +275,9 @@ LT_LABELSUFFIX(name,_name_end): ; \
     .p2align 4; \
 1:
 #else
-# define ABORT_TRANSACTION
+# define ABORT_TRANSACTION_IMPL
 #endif
+#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
 
 #define DO_CALL(syscall) \
     ABORT_TRANSACTION \
diff --git a/sysdeps/unix/sysv/linux/powerpc/not-errno.h b/sysdeps/unix/sysv/linux/powerpc/not-errno.h
new file mode 100644
index 0000000..27da21b
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/powerpc/not-errno.h
@@ -0,0 +1,30 @@
+/* Syscall wrapper that do not set errno.  Linux powerpc version.
+   Copyright (C) 2018 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/>.  */
+
+/* __access_noerrno is used during process initialization in elf/dl-tunables.c
+   before the TCB is initialized, prohibiting the usage of
+   ABORT_TRANSACTION.  */
+#undef ABORT_TRANSACTION
+#define ABORT_TRANSACTION
+
+#include "sysdeps/unix/sysv/linux/not-errno.h"
+
+/* Recover ABORT_TRANSACTION's previous value, in order to not affect
+   other syscalls.  */
+#undef ABORT_TRANSACTION
+#define ABORT_TRANSACTION ABORT_TRANSACTION_IMPL
--
2.9.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHv3] powerpc: Fix syscalls during early process initialization [BZ #22685]

Aurelien Jarno
On 2018-01-12 18:52, Tulio Magno Quites Machado Filho wrote:

> Changes since v2:
>  - Limited the scope of the changes to ABORT_TRANSACTION.
>
> Changes since v1:
>  - Re-implemented the patch to re-define a powerpc-specific
>    __access_noerrno.
>
> --- 8< ---
>
> The tunables framework needs to execute syscall early in process
> initialization, before the TCB is available for consumption.  This
> behavior conflicts with powerpc{|64|64le}'s lock elision code, that
> checks the TCB before trying to abort transactions immediately before
> executing a syscall.
>
> This patch adds a powerpc-specific implementation of __access_noerrno
> that does not abort transactions before the executing syscall.
>
> Tested on powerpc{|64|64le}.
>
> 2018-01-12  Tulio Magno Quites Machado Filho  <[hidden email]>
>
> [BZ #22685]
> * sysdeps/powerpc/powerpc32/sysdep.h (ABORT_TRANSACTION_IMPL): Renamed
> from ABORT_TRANSACTION.
> (ABORT_TRANSACTION): Redirect to ABORT_TRANSACTION_IMPL.
> * sysdeps/powerpc/powerpc64/sysdep.h (ABORT_TRANSACTION,
> ABORT_TRANSACTION_IMPL): Likewise.
> * sysdeps/unix/sysv/linux/powerpc/not-errno.h: New file.  Reuse
> Linux code, but remove the code that aborts transactions.

Thanks for this new version. It is indeed much simpler than the initial
version. I confirm it fixes the issue and does not introduce regressions
in the testsuite.

Tested-by: Aurelien Jarno <[hidden email]>

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[hidden email]                 http://www.aurel32.net