* [PATCH] x86-64: Align child stack to 16 bytes @ 2021-05-23 17:29 H.J. Lu 2021-05-23 17:57 ` Andreas Schwab 0 siblings, 1 reply; 9+ messages in thread From: H.J. Lu @ 2021-05-23 17:29 UTC (permalink / raw) To: libc-alpha In the x86-64 clone wrapper, align child stack to 16 bytes per the x86-64 psABI. --- sysdeps/unix/sysv/linux/Makefile | 2 +- sysdeps/unix/sysv/linux/tst-misalign-clone.c | 97 ++++++++++++++++++++ sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + 3 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 9bcd38a732..283b2d0bd8 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -327,5 +327,5 @@ CFLAGS-gai.c += -DNEED_NETLINK endif ifeq ($(subdir),nptl) -tests += tst-align-clone tst-getpid1 +tests += tst-align-clone tst-misalign-clone tst-getpid1 endif diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c new file mode 100644 index 0000000000..3eb91b9b1f --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c @@ -0,0 +1,97 @@ +/* Copyright (C) 2021 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 + <https://www.gnu.org/licenses/>. */ + +#include <sched.h> +#include <stdbool.h> +#include <stdint.h> +#include <stdio.h> +#include <string.h> +#include <sys/wait.h> +#include <unistd.h> +#include <tst-stack-align.h> +#include <stackinfo.h> +#include <support/xunistd.h> + +static int +f (void *arg) +{ + bool ok = true; + + puts ("in f"); + + if (TEST_STACK_ALIGN ()) + ok = false; + + return ok ? 0 : 1; +} + +static int +do_test (void) +{ + bool ok = true; + + puts ("in main"); + + if (TEST_STACK_ALIGN ()) + ok = false; + +#ifdef __ia64__ +# define STACK_SIZE (256 * 1024) +#else +# define STACK_SIZE (128 * 1024) +#endif + + /* NB: Try to force misaligned child stack. */ + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); + +#ifdef __ia64__ + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, + size_t __child_stack_size, int __flags, + void *__arg, ...); + pid_t p = __clone2 (f, st, sizeof (st), 0, 0); +#else +# if _STACK_GROWS_DOWN + pid_t p = clone (f, st + sizeof (st), 0, 0); +# elif _STACK_GROWS_UP + pid_t p = clone (f, st, 0, 0); +# else +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" +# endif +#endif + if (p == -1) + { + printf("clone failed: %m\n"); + return 1; + } + + int e; + xwaitpid (p, &e, __WCLONE); + if (!WIFEXITED (e)) + { + if (WIFSIGNALED (e)) + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); + else + puts ("did not terminate correctly"); + return 1; + } + if (WEXITSTATUS (e) != 0) + ok = false; + + return ok ? 0 : 1; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S index 31ac12da0c..5f52ce7813 100644 --- a/sysdeps/unix/sysv/linux/x86_64/clone.S +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S @@ -57,6 +57,9 @@ ENTRY (__clone) testq %rsi,%rsi /* no NULL stack pointers */ jz SYSCALL_ERROR_LABEL + /* Align stack to 16 bytes per the x86-64 psABI. */ + andq $-16, %rsi + /* Insert the argument onto the new stack. */ subq $16,%rsi movq %rcx,8(%rsi) -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86-64: Align child stack to 16 bytes 2021-05-23 17:29 [PATCH] x86-64: Align child stack to 16 bytes H.J. Lu @ 2021-05-23 17:57 ` Andreas Schwab 2021-05-23 18:40 ` [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] H.J. Lu 0 siblings, 1 reply; 9+ messages in thread From: Andreas Schwab @ 2021-05-23 17:57 UTC (permalink / raw) To: H.J. Lu via Libc-alpha On Mai 23 2021, H.J. Lu via Libc-alpha wrote: > + /* NB: Try to force misaligned child stack. */ > + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); If you want a misaligned pointer, you need to start with a known alignment. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-23 17:57 ` Andreas Schwab @ 2021-05-23 18:40 ` H.J. Lu 2021-05-24 12:07 ` Florian Weimer 2021-05-24 14:19 ` Carlos O'Donell 0 siblings, 2 replies; 9+ messages in thread From: H.J. Lu @ 2021-05-23 18:40 UTC (permalink / raw) To: Andreas Schwab; +Cc: H.J. Lu via Libc-alpha [-- Attachment #1: Type: text/plain, Size: 399 bytes --] On Sun, May 23, 2021 at 10:57 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > > On Mai 23 2021, H.J. Lu via Libc-alpha wrote: > > > + /* NB: Try to force misaligned child stack. */ > > + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); > > If you want a misaligned pointer, you need to start with a known > alignment. > Fixed. Here is the v2 patch. OK for master? Thanks. -- H.J. [-- Attachment #2: v2-0001-x86-64-Align-child-stack-to-16-bytes-BZ-27902.patch --] [-- Type: text/x-patch, Size: 4450 bytes --] From ae2dc0fe317b93d3ac1d62b2d191d45e9bee0cea Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Sun, 23 May 2021 10:25:10 -0700 Subject: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] In the x86-64 clone wrapper, align child stack to 16 bytes per the x86-64 psABI. --- sysdeps/unix/sysv/linux/Makefile | 2 +- sysdeps/unix/sysv/linux/tst-misalign-clone.c | 99 ++++++++++++++++++++ sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + 3 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 70c3b3f8a3..d355b49033 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -109,7 +109,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \ tst-timerfd tst-ppoll \ tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \ - tst-ntp_gettimex tst-sigtimedwait + tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone # Test for the symbol version of fcntl that was replaced in glibc 2.28. ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes) diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c new file mode 100644 index 0000000000..070a457d64 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c @@ -0,0 +1,99 @@ +/* Copyright (C) 2021 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 + <https://www.gnu.org/licenses/>. */ + +#include <sched.h> +#include <stdbool.h> +#include <stdint.h> +#include <stdio.h> +#include <string.h> +#include <sys/wait.h> +#include <unistd.h> +#include <libc-pointer-arith.h> +#include <tst-stack-align.h> +#include <stackinfo.h> +#include <support/xunistd.h> + +static int +f (void *arg) +{ + bool ok = true; + + puts ("in f"); + + if (TEST_STACK_ALIGN ()) + ok = false; + + return ok ? 0 : 1; +} + +static int +do_test (void) +{ + bool ok = true; + + puts ("in main"); + + if (TEST_STACK_ALIGN ()) + ok = false; + +#ifdef __ia64__ +# define STACK_SIZE (256 * 1024) +#else +# define STACK_SIZE (128 * 1024) +#endif + + char st[STACK_SIZE + 1]; + /* NB: Align child stack to 1 byte. */ + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; + +#ifdef __ia64__ + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, + size_t __child_stack_size, int __flags, + void *__arg, ...); + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); +#else +# if _STACK_GROWS_DOWN + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); +# elif _STACK_GROWS_UP + pid_t p = clone (f, stack, 0, 0); +# else +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" +# endif +#endif + if (p == -1) + { + printf("clone failed: %m\n"); + return 1; + } + + int e; + xwaitpid (p, &e, __WCLONE); + if (!WIFEXITED (e)) + { + if (WIFSIGNALED (e)) + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); + else + puts ("did not terminate correctly"); + return 1; + } + if (WEXITSTATUS (e) != 0) + ok = false; + + return ok ? 0 : 1; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S index 31ac12da0c..5f52ce7813 100644 --- a/sysdeps/unix/sysv/linux/x86_64/clone.S +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S @@ -57,6 +57,9 @@ ENTRY (__clone) testq %rsi,%rsi /* no NULL stack pointers */ jz SYSCALL_ERROR_LABEL + /* Align stack to 16 bytes per the x86-64 psABI. */ + andq $-16, %rsi + /* Insert the argument onto the new stack. */ subq $16,%rsi movq %rcx,8(%rsi) -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-23 18:40 ` [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] H.J. Lu @ 2021-05-24 12:07 ` Florian Weimer 2021-05-24 12:53 ` H.J. Lu 2021-05-24 14:19 ` Carlos O'Donell 1 sibling, 1 reply; 9+ messages in thread From: Florian Weimer @ 2021-05-24 12:07 UTC (permalink / raw) To: H.J. Lu via Libc-alpha; +Cc: Andreas Schwab, H.J. Lu * H. J. Lu via Libc-alpha: > +static int > +f (void *arg) > +{ > + bool ok = true; > + > + puts ("in f"); > + > + if (TEST_STACK_ALIGN ()) > + ok = false; > + > + return ok ? 0 : 1; > +} Is it okay to call libc functions from the callback because it is after a fork-style clone (without any flags) that inherits the TCB? Otherwise there are going to be problems with the stack protector at least. > + char st[STACK_SIZE + 1]; > + /* NB: Align child stack to 1 byte. */ > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; > + > +#ifdef __ia64__ > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > + size_t __child_stack_size, int __flags, > + void *__arg, ...); > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > +#else > +# if _STACK_GROWS_DOWN > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > +# elif _STACK_GROWS_UP > + pid_t p = clone (f, stack, 0, 0); > +# else > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > +# endif > +#endif I think the (mis)alignment step has to be among the _STACK_GROWS_UP/_STACK_GROWS_DOWN part. Thanks, Florian ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-24 12:07 ` Florian Weimer @ 2021-05-24 12:53 ` H.J. Lu 0 siblings, 0 replies; 9+ messages in thread From: H.J. Lu @ 2021-05-24 12:53 UTC (permalink / raw) To: Florian Weimer; +Cc: H.J. Lu via Libc-alpha, Andreas Schwab On Mon, May 24, 2021 at 5:07 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu via Libc-alpha: > > > +static int > > +f (void *arg) > > +{ > > + bool ok = true; > > + > > + puts ("in f"); > > + > > + if (TEST_STACK_ALIGN ()) > > + ok = false; > > + > > + return ok ? 0 : 1; > > +} > > Is it okay to call libc functions from the callback because it is after > a fork-style clone (without any flags) that inherits the TCB? Otherwise > there are going to be problems with the stack protector at least. I don't see why libc functions can't be used. I copied the test from tst-align-clone.c. I only changed the child stack alignment. > > + char st[STACK_SIZE + 1]; > > + /* NB: Align child stack to 1 byte. */ > > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; > > + > > +#ifdef __ia64__ > > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > > + size_t __child_stack_size, int __flags, > > + void *__arg, ...); > > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > > +#else > > +# if _STACK_GROWS_DOWN > > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > > +# elif _STACK_GROWS_UP > > + pid_t p = clone (f, stack, 0, 0); > > +# else > > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > > +# endif > > +#endif > > I think the (mis)alignment step has to be among the > _STACK_GROWS_UP/_STACK_GROWS_DOWN part. Since the bottom of stack is an odd address and STACK_SIZE is even, both the bottom of stack and the top of stack are odd addresses. There is no need to do it separately. > Thanks, > Florian > -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-23 18:40 ` [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] H.J. Lu 2021-05-24 12:07 ` Florian Weimer @ 2021-05-24 14:19 ` Carlos O'Donell 2021-05-24 14:55 ` Noah Goldstein 2021-05-24 18:46 ` H.J. Lu 1 sibling, 2 replies; 9+ messages in thread From: Carlos O'Donell @ 2021-05-24 14:19 UTC (permalink / raw) To: H.J. Lu, Andreas Schwab; +Cc: H.J. Lu via Libc-alpha On 5/23/21 2:40 PM, H.J. Lu via Libc-alpha wrote: > On Sun, May 23, 2021 at 10:57 AM Andreas Schwab <schwab@linux-m68k.org> wrote: >> >> On Mai 23 2021, H.J. Lu via Libc-alpha wrote: >> >>> + /* NB: Try to force misaligned child stack. */ >>> + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); >> >> If you want a misaligned pointer, you need to start with a known >> alignment. >> > > Fixed. > > Here is the v2 patch. OK for master? Please post v3. See review below. > From ae2dc0fe317b93d3ac1d62b2d191d45e9bee0cea Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Sun, 23 May 2021 10:25:10 -0700 > Subject: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] > > In the x86-64 clone wrapper, align child stack to 16 bytes per the > x86-64 psABI. > --- > sysdeps/unix/sysv/linux/Makefile | 2 +- > sysdeps/unix/sysv/linux/tst-misalign-clone.c | 99 ++++++++++++++++++++ > sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + > 3 files changed, 103 insertions(+), 1 deletion(-) > create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > index 70c3b3f8a3..d355b49033 100644 > --- a/sysdeps/unix/sysv/linux/Makefile > +++ b/sysdeps/unix/sysv/linux/Makefile > @@ -109,7 +109,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ > tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \ > tst-timerfd tst-ppoll \ > tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \ > - tst-ntp_gettimex tst-sigtimedwait > + tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone OK. Add two tests. > # Test for the symbol version of fcntl that was replaced in glibc 2.28. > ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes) > diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > new file mode 100644 > index 0000000000..070a457d64 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > @@ -0,0 +1,99 @@ > +/* Copyright (C) 2021 Free Software Foundation, Inc. Add one line test description. > + 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <sched.h> > +#include <stdbool.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/wait.h> > +#include <unistd.h> > +#include <libc-pointer-arith.h> > +#include <tst-stack-align.h> > +#include <stackinfo.h> > +#include <support/xunistd.h> > + > +static int > +f (void *arg) Please give this a real name e.g. check_func_align > +{ > + bool ok = true; > + > + puts ("in f"); > + > + if (TEST_STACK_ALIGN ()) > + ok = false; > + > + return ok ? 0 : 1; > +} OK. This is a fork-style clone for which everything should work including calling puts() from libc.so. This answers Florian's question in his review. > + > +static int > +do_test (void) > +{ > + bool ok = true; > + > + puts ("in main"); s/in main/in do_test/g > + > + if (TEST_STACK_ALIGN ()) > + ok = false; > + > +#ifdef __ia64__ > +# define STACK_SIZE (256 * 1024) > +#else > +# define STACK_SIZE (128 * 1024) > +#endif > + > + char st[STACK_SIZE + 1]; > + /* NB: Align child stack to 1 byte. */ > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; OK. Fixes Andreas' review. > + > +#ifdef __ia64__ > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > + size_t __child_stack_size, int __flags, > + void *__arg, ...); > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > +#else > +# if _STACK_GROWS_DOWN > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > +# elif _STACK_GROWS_UP > + pid_t p = clone (f, stack, 0, 0); OK. Misaligned in both directions. Answers Florian's question. The the _STACK_GROWS_UP part is only for hppa. > +# else > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > +# endif > +#endif > + if (p == -1) > + { > + printf("clone failed: %m\n"); > + return 1; > + } Please use TEST_VERIFY* e.g. /* Clone must not fail. */ TEST_VERIFY_EXIT (p != -1); Or if you really want a specific message FAIL_EXIT. e.g. if (p == -1) FAIL_EXIT1 ("clone failed: %m\n"); > + > + int e; > + xwaitpid (p, &e, __WCLONE); > + if (!WIFEXITED (e)) > + { > + if (WIFSIGNALED (e)) > + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); Add the extra information here if you want. > + else Remove the else. > + puts ("did not terminate correctly"); > + return 1; Then FAIL_EXIT1 ("Process did not termiante correctly"); > + } > + if (WEXITSTATUS (e) != 0) > + ok = false; > + > + return ok ? 0 : 1; If (WEXITSTATUS (e) != 0) FAIL_EXIT1 (""); return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S > index 31ac12da0c..5f52ce7813 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/clone.S > +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S > @@ -57,6 +57,9 @@ ENTRY (__clone) > testq %rsi,%rsi /* no NULL stack pointers */ > jz SYSCALL_ERROR_LABEL > > + /* Align stack to 16 bytes per the x86-64 psABI. */ > + andq $-16, %rsi OK. Interesting to see this was missing, but maybe we never get this situation in production. > + > /* Insert the argument onto the new stack. */ > subq $16,%rsi > movq %rcx,8(%rsi) > -- > 2.31.1 > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-24 14:19 ` Carlos O'Donell @ 2021-05-24 14:55 ` Noah Goldstein 2021-05-24 18:46 ` H.J. Lu 2021-05-24 18:46 ` H.J. Lu 1 sibling, 1 reply; 9+ messages in thread From: Noah Goldstein @ 2021-05-24 14:55 UTC (permalink / raw) To: Carlos O'Donell; +Cc: H.J. Lu, Andreas Schwab, H.J. Lu via Libc-alpha On Mon, May 24, 2021 at 10:49 AM Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org> wrote: > > On 5/23/21 2:40 PM, H.J. Lu via Libc-alpha wrote: > > On Sun, May 23, 2021 at 10:57 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > >> > >> On Mai 23 2021, H.J. Lu via Libc-alpha wrote: > >> > >>> + /* NB: Try to force misaligned child stack. */ > >>> + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); > >> > >> If you want a misaligned pointer, you need to start with a known > >> alignment. > >> > > > > Fixed. > > > > Here is the v2 patch. OK for master? > > Please post v3. See review below. > > > From ae2dc0fe317b93d3ac1d62b2d191d45e9bee0cea Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" <hjl.tools@gmail.com> > > Date: Sun, 23 May 2021 10:25:10 -0700 > > Subject: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] > > > > In the x86-64 clone wrapper, align child stack to 16 bytes per the > > x86-64 psABI. > > --- > > sysdeps/unix/sysv/linux/Makefile | 2 +- > > sysdeps/unix/sysv/linux/tst-misalign-clone.c | 99 ++++++++++++++++++++ > > sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + > > 3 files changed, 103 insertions(+), 1 deletion(-) > > create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c > > > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > > index 70c3b3f8a3..d355b49033 100644 > > --- a/sysdeps/unix/sysv/linux/Makefile > > +++ b/sysdeps/unix/sysv/linux/Makefile > > @@ -109,7 +109,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ > > tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \ > > tst-timerfd tst-ppoll \ > > tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \ > > - tst-ntp_gettimex tst-sigtimedwait > > + tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone > > OK. Add two tests. > > > # Test for the symbol version of fcntl that was replaced in glibc 2.28. > > ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes) > > diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > new file mode 100644 > > index 0000000000..070a457d64 > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > @@ -0,0 +1,99 @@ > > +/* Copyright (C) 2021 Free Software Foundation, Inc. > > Add one line test description. > > > + 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 > > + <https://www.gnu.org/licenses/>. */ > > + > > +#include <sched.h> > > +#include <stdbool.h> > > +#include <stdint.h> > > +#include <stdio.h> > > +#include <string.h> > > +#include <sys/wait.h> > > +#include <unistd.h> > > +#include <libc-pointer-arith.h> > > +#include <tst-stack-align.h> > > +#include <stackinfo.h> > > +#include <support/xunistd.h> > > + > > +static int > > +f (void *arg) > > Please give this a real name e.g. check_func_align > > > +{ > > + bool ok = true; > > + > > + puts ("in f"); > > + > > + if (TEST_STACK_ALIGN ()) > > + ok = false; > > + > > + return ok ? 0 : 1; > > +} > > OK. This is a fork-style clone for which everything should work including > calling puts() from libc.so. This answers Florian's question in his review. > > > + > > +static int > > +do_test (void) > > +{ > > + bool ok = true; > > + > > + puts ("in main"); > > s/in main/in do_test/g > > > + > > + if (TEST_STACK_ALIGN ()) > > + ok = false; > > + > > +#ifdef __ia64__ > > +# define STACK_SIZE (256 * 1024) > > +#else > > +# define STACK_SIZE (128 * 1024) > > +#endif > > + > > + char st[STACK_SIZE + 1]; > > + /* NB: Align child stack to 1 byte. */ > > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; > > OK. Fixes Andreas' review. > > > + > > +#ifdef __ia64__ > > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > > + size_t __child_stack_size, int __flags, > > + void *__arg, ...); > > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > > +#else > > +# if _STACK_GROWS_DOWN > > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > > +# elif _STACK_GROWS_UP > > + pid_t p = clone (f, stack, 0, 0); > > OK. Misaligned in both directions. Answers Florian's question. > The the _STACK_GROWS_UP part is only for hppa. > > > +# else > > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > > +# endif > > +#endif > > > + if (p == -1) > > + { > > + printf("clone failed: %m\n"); > > + return 1; > > + } > > Please use TEST_VERIFY* e.g. > > /* Clone must not fail. */ > TEST_VERIFY_EXIT (p != -1); > > Or if you really want a specific message FAIL_EXIT. > > e.g. > > if (p == -1) > FAIL_EXIT1 ("clone failed: %m\n"); > > > + > > + int e; > > + xwaitpid (p, &e, __WCLONE); > > + if (!WIFEXITED (e)) > > + { > > + if (WIFSIGNALED (e)) > > + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); > > Add the extra information here if you want. > > > + else > > Remove the else. > > > + puts ("did not terminate correctly"); > > + return 1; > > Then FAIL_EXIT1 ("Process did not termiante correctly"); > > > + } > > > + if (WEXITSTATUS (e) != 0) > > + ok = false; > > + > > + return ok ? 0 : 1; > > If (WEXITSTATUS (e) != 0) > FAIL_EXIT1 (""); > > return 0; > > > > +} > > + > > +#include <support/test-driver.c> > > diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S > > index 31ac12da0c..5f52ce7813 100644 > > --- a/sysdeps/unix/sysv/linux/x86_64/clone.S > > +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S > > @@ -57,6 +57,9 @@ ENTRY (__clone) > > testq %rsi,%rsi /* no NULL stack pointers */ Can you remove the test and use flags set by andq $-16, %rsi? > > jz SYSCALL_ERROR_LABEL > > > > + /* Align stack to 16 bytes per the x86-64 psABI. */ > > + andq $-16, %rsi > > OK. Interesting to see this was missing, but maybe we never get this situation in production. > > > + > > /* Insert the argument onto the new stack. */ > > subq $16,%rsi > > movq %rcx,8(%rsi) Lower latency to do: movq %rcx, -8(%rsi) subq $16, %rsi > > -- > > 2.31.1 > > > > > -- > Cheers, > Carlos. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-24 14:55 ` Noah Goldstein @ 2021-05-24 18:46 ` H.J. Lu 0 siblings, 0 replies; 9+ messages in thread From: H.J. Lu @ 2021-05-24 18:46 UTC (permalink / raw) To: Noah Goldstein Cc: Carlos O'Donell, Andreas Schwab, H.J. Lu via Libc-alpha On Mon, May 24, 2021 at 7:55 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > On Mon, May 24, 2021 at 10:49 AM Carlos O'Donell via Libc-alpha > <libc-alpha@sourceware.org> wrote: > > > > On 5/23/21 2:40 PM, H.J. Lu via Libc-alpha wrote: > > > On Sun, May 23, 2021 at 10:57 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > > >> > > >> On Mai 23 2021, H.J. Lu via Libc-alpha wrote: > > >> > > >>> + /* NB: Try to force misaligned child stack. */ > > >>> + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); > > >> > > >> If you want a misaligned pointer, you need to start with a known > > >> alignment. > > >> > > > > > > Fixed. > > > > > > Here is the v2 patch. OK for master? > > > > Please post v3. See review below. > > > > > From ae2dc0fe317b93d3ac1d62b2d191d45e9bee0cea Mon Sep 17 00:00:00 2001 > > > From: "H.J. Lu" <hjl.tools@gmail.com> > > > Date: Sun, 23 May 2021 10:25:10 -0700 > > > Subject: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] > > > > > > In the x86-64 clone wrapper, align child stack to 16 bytes per the > > > x86-64 psABI. > > > --- > > > sysdeps/unix/sysv/linux/Makefile | 2 +- > > > sysdeps/unix/sysv/linux/tst-misalign-clone.c | 99 ++++++++++++++++++++ > > > sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + > > > 3 files changed, 103 insertions(+), 1 deletion(-) > > > create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c > > > > > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > > > index 70c3b3f8a3..d355b49033 100644 > > > --- a/sysdeps/unix/sysv/linux/Makefile > > > +++ b/sysdeps/unix/sysv/linux/Makefile > > > @@ -109,7 +109,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ > > > tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \ > > > tst-timerfd tst-ppoll \ > > > tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \ > > > - tst-ntp_gettimex tst-sigtimedwait > > > + tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone > > > > OK. Add two tests. > > > > > # Test for the symbol version of fcntl that was replaced in glibc 2.28. > > > ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes) > > > diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > > new file mode 100644 > > > index 0000000000..070a457d64 > > > --- /dev/null > > > +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > > @@ -0,0 +1,99 @@ > > > +/* Copyright (C) 2021 Free Software Foundation, Inc. > > > > Add one line test description. > > > > > + 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 > > > + <https://www.gnu.org/licenses/>. */ > > > + > > > +#include <sched.h> > > > +#include <stdbool.h> > > > +#include <stdint.h> > > > +#include <stdio.h> > > > +#include <string.h> > > > +#include <sys/wait.h> > > > +#include <unistd.h> > > > +#include <libc-pointer-arith.h> > > > +#include <tst-stack-align.h> > > > +#include <stackinfo.h> > > > +#include <support/xunistd.h> > > > + > > > +static int > > > +f (void *arg) > > > > Please give this a real name e.g. check_func_align > > > > > +{ > > > + bool ok = true; > > > + > > > + puts ("in f"); > > > + > > > + if (TEST_STACK_ALIGN ()) > > > + ok = false; > > > + > > > + return ok ? 0 : 1; > > > +} > > > > OK. This is a fork-style clone for which everything should work including > > calling puts() from libc.so. This answers Florian's question in his review. > > > > > + > > > +static int > > > +do_test (void) > > > +{ > > > + bool ok = true; > > > + > > > + puts ("in main"); > > > > s/in main/in do_test/g > > > > > + > > > + if (TEST_STACK_ALIGN ()) > > > + ok = false; > > > + > > > +#ifdef __ia64__ > > > +# define STACK_SIZE (256 * 1024) > > > +#else > > > +# define STACK_SIZE (128 * 1024) > > > +#endif > > > + > > > + char st[STACK_SIZE + 1]; > > > + /* NB: Align child stack to 1 byte. */ > > > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; > > > > OK. Fixes Andreas' review. > > > > > + > > > +#ifdef __ia64__ > > > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > > > + size_t __child_stack_size, int __flags, > > > + void *__arg, ...); > > > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > > > +#else > > > +# if _STACK_GROWS_DOWN > > > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > > > +# elif _STACK_GROWS_UP > > > + pid_t p = clone (f, stack, 0, 0); > > > > OK. Misaligned in both directions. Answers Florian's question. > > The the _STACK_GROWS_UP part is only for hppa. > > > > > +# else > > > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > > > +# endif > > > +#endif > > > > > + if (p == -1) > > > + { > > > + printf("clone failed: %m\n"); > > > + return 1; > > > + } > > > > Please use TEST_VERIFY* e.g. > > > > /* Clone must not fail. */ > > TEST_VERIFY_EXIT (p != -1); > > > > Or if you really want a specific message FAIL_EXIT. > > > > e.g. > > > > if (p == -1) > > FAIL_EXIT1 ("clone failed: %m\n"); > > > > > + > > > + int e; > > > + xwaitpid (p, &e, __WCLONE); > > > + if (!WIFEXITED (e)) > > > + { > > > + if (WIFSIGNALED (e)) > > > + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); > > > > Add the extra information here if you want. > > > > > + else > > > > Remove the else. > > > > > + puts ("did not terminate correctly"); > > > + return 1; > > > > Then FAIL_EXIT1 ("Process did not termiante correctly"); > > > > > + } > > > > > + if (WEXITSTATUS (e) != 0) > > > + ok = false; > > > + > > > + return ok ? 0 : 1; > > > > If (WEXITSTATUS (e) != 0) > > FAIL_EXIT1 (""); > > > > return 0; > > > > > > > +} > > > + > > > +#include <support/test-driver.c> > > > diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S > > > index 31ac12da0c..5f52ce7813 100644 > > > --- a/sysdeps/unix/sysv/linux/x86_64/clone.S > > > +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S > > > @@ -57,6 +57,9 @@ ENTRY (__clone) > > > testq %rsi,%rsi /* no NULL stack pointers */ > > Can you remove the test and use flags set by andq $-16, %rsi? Fixed. > > > jz SYSCALL_ERROR_LABEL > > > > > > + /* Align stack to 16 bytes per the x86-64 psABI. */ > > > + andq $-16, %rsi > > > > OK. Interesting to see this was missing, but maybe we never get this situation in production. > > > > > + > > > /* Insert the argument onto the new stack. */ > > > subq $16,%rsi > > > movq %rcx,8(%rsi) > > Lower latency to do: > movq %rcx, -8(%rsi) > subq $16, %rsi > Fixed. Thanks. -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] 2021-05-24 14:19 ` Carlos O'Donell 2021-05-24 14:55 ` Noah Goldstein @ 2021-05-24 18:46 ` H.J. Lu 1 sibling, 0 replies; 9+ messages in thread From: H.J. Lu @ 2021-05-24 18:46 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Andreas Schwab, H.J. Lu via Libc-alpha On Mon, May 24, 2021 at 7:19 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 5/23/21 2:40 PM, H.J. Lu via Libc-alpha wrote: > > On Sun, May 23, 2021 at 10:57 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > >> > >> On Mai 23 2021, H.J. Lu via Libc-alpha wrote: > >> > >>> + /* NB: Try to force misaligned child stack. */ > >>> + char st[STACK_SIZE + 4] __attribute__ ((aligned (1))); > >> > >> If you want a misaligned pointer, you need to start with a known > >> alignment. > >> > > > > Fixed. > > > > Here is the v2 patch. OK for master? > > Please post v3. See review below. > > > From ae2dc0fe317b93d3ac1d62b2d191d45e9bee0cea Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" <hjl.tools@gmail.com> > > Date: Sun, 23 May 2021 10:25:10 -0700 > > Subject: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] > > > > In the x86-64 clone wrapper, align child stack to 16 bytes per the > > x86-64 psABI. > > --- > > sysdeps/unix/sysv/linux/Makefile | 2 +- > > sysdeps/unix/sysv/linux/tst-misalign-clone.c | 99 ++++++++++++++++++++ > > sysdeps/unix/sysv/linux/x86_64/clone.S | 3 + > > 3 files changed, 103 insertions(+), 1 deletion(-) > > create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c > > > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > > index 70c3b3f8a3..d355b49033 100644 > > --- a/sysdeps/unix/sysv/linux/Makefile > > +++ b/sysdeps/unix/sysv/linux/Makefile > > @@ -109,7 +109,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ > > tst-tgkill tst-sysvsem-linux tst-sysvmsg-linux tst-sysvshm-linux \ > > tst-timerfd tst-ppoll \ > > tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \ > > - tst-ntp_gettimex tst-sigtimedwait > > + tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone > > OK. Add two tests. > > > # Test for the symbol version of fcntl that was replaced in glibc 2.28. > > ifeq ($(have-GLIBC_2.27)$(build-shared),yesyes) > > diff --git a/sysdeps/unix/sysv/linux/tst-misalign-clone.c b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > new file mode 100644 > > index 0000000000..070a457d64 > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c > > @@ -0,0 +1,99 @@ > > +/* Copyright (C) 2021 Free Software Foundation, Inc. > > Add one line test description. > > > + 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 > > + <https://www.gnu.org/licenses/>. */ > > + > > +#include <sched.h> > > +#include <stdbool.h> > > +#include <stdint.h> > > +#include <stdio.h> > > +#include <string.h> > > +#include <sys/wait.h> > > +#include <unistd.h> > > +#include <libc-pointer-arith.h> > > +#include <tst-stack-align.h> > > +#include <stackinfo.h> > > +#include <support/xunistd.h> > > + > > +static int > > +f (void *arg) > > Please give this a real name e.g. check_func_align I changed it to check_stack_alignment. > > +{ > > + bool ok = true; > > + > > + puts ("in f"); > > + > > + if (TEST_STACK_ALIGN ()) > > + ok = false; > > + > > + return ok ? 0 : 1; > > +} > > OK. This is a fork-style clone for which everything should work including > calling puts() from libc.so. This answers Florian's question in his review. > > > + > > +static int > > +do_test (void) > > +{ > > + bool ok = true; > > + > > + puts ("in main"); > > s/in main/in do_test/g Fixed. > > + > > + if (TEST_STACK_ALIGN ()) > > + ok = false; > > + > > +#ifdef __ia64__ > > +# define STACK_SIZE (256 * 1024) > > +#else > > +# define STACK_SIZE (128 * 1024) > > +#endif > > + > > + char st[STACK_SIZE + 1]; > > + /* NB: Align child stack to 1 byte. */ > > + char *stack = PTR_ALIGN_UP (&st[0], 2) + 1; > > OK. Fixes Andreas' review. > > > + > > +#ifdef __ia64__ > > + extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base, > > + size_t __child_stack_size, int __flags, > > + void *__arg, ...); > > + pid_t p = __clone2 (f, stack, STACK_SIZE, 0, 0); > > +#else > > +# if _STACK_GROWS_DOWN > > + pid_t p = clone (f, stack + STACK_SIZE, 0, 0); > > +# elif _STACK_GROWS_UP > > + pid_t p = clone (f, stack, 0, 0); > > OK. Misaligned in both directions. Answers Florian's question. > The the _STACK_GROWS_UP part is only for hppa. > > > +# else > > +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP" > > +# endif > > +#endif > > > + if (p == -1) > > + { > > + printf("clone failed: %m\n"); > > + return 1; > > + } > > Please use TEST_VERIFY* e.g. > > /* Clone must not fail. */ > TEST_VERIFY_EXIT (p != -1); Fixed. > Or if you really want a specific message FAIL_EXIT. > > e.g. > > if (p == -1) > FAIL_EXIT1 ("clone failed: %m\n"); > > > + > > + int e; > > + xwaitpid (p, &e, __WCLONE); > > + if (!WIFEXITED (e)) > > + { > > + if (WIFSIGNALED (e)) > > + printf ("died from signal %s\n", strsignal (WTERMSIG (e))); > > Add the extra information here if you want. > > > + else > > Remove the else. Fixed. > > + puts ("did not terminate correctly"); > > + return 1; > > Then FAIL_EXIT1 ("Process did not termiante correctly"); Fixed. > > + } > > > + if (WEXITSTATUS (e) != 0) > > + ok = false; > > + > > + return ok ? 0 : 1; > > If (WEXITSTATUS (e) != 0) > FAIL_EXIT1 (""); > > return 0; Fixed. > > > +} > > + > > +#include <support/test-driver.c> > > diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S > > index 31ac12da0c..5f52ce7813 100644 > > --- a/sysdeps/unix/sysv/linux/x86_64/clone.S > > +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S > > @@ -57,6 +57,9 @@ ENTRY (__clone) > > testq %rsi,%rsi /* no NULL stack pointers */ > > jz SYSCALL_ERROR_LABEL > > > > + /* Align stack to 16 bytes per the x86-64 psABI. */ > > + andq $-16, %rsi > > OK. Interesting to see this was missing, but maybe we never get this situation in production. Looks like it. > > + > > /* Insert the argument onto the new stack. */ > > subq $16,%rsi > > movq %rcx,8(%rsi) > > -- > > 2.31.1 > > > Thanks. -- H.J. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-05-24 18:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-23 17:29 [PATCH] x86-64: Align child stack to 16 bytes H.J. Lu 2021-05-23 17:57 ` Andreas Schwab 2021-05-23 18:40 ` [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902] H.J. Lu 2021-05-24 12:07 ` Florian Weimer 2021-05-24 12:53 ` H.J. Lu 2021-05-24 14:19 ` Carlos O'Donell 2021-05-24 14:55 ` Noah Goldstein 2021-05-24 18:46 ` H.J. Lu 2021-05-24 18:46 ` H.J. Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).