[v2] x86-64: Align child stack to 16 bytes [BZ #27902]
Checks
Commit Message
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.
Comments
* 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
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
>
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
>
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.
>
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.
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.
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
@@ -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)
new file mode 100644
@@ -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>
@@ -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