[v3] x86-64: Align child stack to 16 bytes [BZ #27902]
Checks
Commit Message
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 | 96 ++++++++++++++++++++
sysdeps/unix/sysv/linux/x86_64/clone.S | 9 +-
3 files changed, 103 insertions(+), 4 deletions(-)
create mode 100644 sysdeps/unix/sysv/linux/tst-misalign-clone.c
Comments
On 5/24/21 2:47 PM, H.J. Lu wrote:
> In the x86-64 clone wrapper, align child stack to 16 bytes per the
> x86-64 psABI.
LGTM. Includes Noah and my recommendations from v2 review. Thanks Noah!
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> sysdeps/unix/sysv/linux/Makefile | 2 +-
> sysdeps/unix/sysv/linux/tst-misalign-clone.c | 96 ++++++++++++++++++++
> sysdeps/unix/sysv/linux/x86_64/clone.S | 9 +-
> 3 files changed, 103 insertions(+), 4 deletions(-)
> 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. 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..60f0ed0839
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-misalign-clone.c
> @@ -0,0 +1,96 @@
> +/* Verify that the clone wrapper properly aligns the child stack.
OK.
> + 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>
> +#include <support/check.h>
> +
> +static int
> +check_stack_alignment (void *arg)
OK. Real name.
> +{
> + bool ok = true;
> +
> + puts ("in f");
> +
> + if (TEST_STACK_ALIGN ())
> + ok = false;
> +
> + return ok ? 0 : 1;
> +}
> +
> +static int
> +do_test (void)
> +{
> + puts ("in do_test");
OK.
> +
> + if (TEST_STACK_ALIGN ())
> + FAIL_EXIT1 ("stack isn't aligned\n");
> +
> +#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 (check_stack_alignment, stack, STACK_SIZE, 0, 0);
> +#else
> +# if _STACK_GROWS_DOWN
> + pid_t p = clone (check_stack_alignment, stack + STACK_SIZE, 0, 0);
> +# elif _STACK_GROWS_UP
> + pid_t p = clone (check_stack_alignment, stack, 0, 0);
> +# else
> +# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
> +# endif
> +#endif
> +
> + /* Clone must not fail. */
> + TEST_VERIFY_EXIT (p != -1);
OK.
> +
> + int e;
> + xwaitpid (p, &e, __WCLONE);
> + if (!WIFEXITED (e))
> + {
> + if (WIFSIGNALED (e))
> + printf ("died from signal %s\n", strsignal (WTERMSIG (e)));
> + FAIL_EXIT1 ("process did not terminate correctly");
OK.
> + }
> +
> + if (WEXITSTATUS (e) != 0)
> + FAIL_EXIT1 ("exit code %d", WEXITSTATUS (e));
OK.
> +
> + 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..7418154c4b 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/clone.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S
> @@ -54,12 +54,15 @@ ENTRY (__clone)
> movq $-EINVAL,%rax
> testq %rdi,%rdi /* no NULL function pointers */
> jz SYSCALL_ERROR_LABEL
> - testq %rsi,%rsi /* no NULL stack pointers */
OK. Remove test resolves Noah's comment.
> - jz SYSCALL_ERROR_LABEL
> +
> + /* Align stack to 16 bytes per the x86-64 psABI. */
> + andq $-16, %rsi
> + jz SYSCALL_ERROR_LABEL /* no NULL stack pointers */
>
> /* Insert the argument onto the new stack. */
> + movq %rcx,-8(%rsi)
> +
> subq $16,%rsi
OK. Includes Noah's suggestion to use movq/subq pair.
> - movq %rcx,8(%rsi)
>
> /* Save the function pointer. It will be popped off in the
> child. */
>
@@ -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,96 @@
+/* Verify that the clone wrapper properly aligns the child stack.
+ 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>
+#include <support/check.h>
+
+static int
+check_stack_alignment (void *arg)
+{
+ bool ok = true;
+
+ puts ("in f");
+
+ if (TEST_STACK_ALIGN ())
+ ok = false;
+
+ return ok ? 0 : 1;
+}
+
+static int
+do_test (void)
+{
+ puts ("in do_test");
+
+ if (TEST_STACK_ALIGN ())
+ FAIL_EXIT1 ("stack isn't aligned\n");
+
+#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 (check_stack_alignment, stack, STACK_SIZE, 0, 0);
+#else
+# if _STACK_GROWS_DOWN
+ pid_t p = clone (check_stack_alignment, stack + STACK_SIZE, 0, 0);
+# elif _STACK_GROWS_UP
+ pid_t p = clone (check_stack_alignment, stack, 0, 0);
+# else
+# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
+# endif
+#endif
+
+ /* Clone must not fail. */
+ TEST_VERIFY_EXIT (p != -1);
+
+ int e;
+ xwaitpid (p, &e, __WCLONE);
+ if (!WIFEXITED (e))
+ {
+ if (WIFSIGNALED (e))
+ printf ("died from signal %s\n", strsignal (WTERMSIG (e)));
+ FAIL_EXIT1 ("process did not terminate correctly");
+ }
+
+ if (WEXITSTATUS (e) != 0)
+ FAIL_EXIT1 ("exit code %d", WEXITSTATUS (e));
+
+ return 0;
+}
+
+#include <support/test-driver.c>
@@ -54,12 +54,15 @@ ENTRY (__clone)
movq $-EINVAL,%rax
testq %rdi,%rdi /* no NULL function pointers */
jz SYSCALL_ERROR_LABEL
- testq %rsi,%rsi /* no NULL stack pointers */
- jz SYSCALL_ERROR_LABEL
+
+ /* Align stack to 16 bytes per the x86-64 psABI. */
+ andq $-16, %rsi
+ jz SYSCALL_ERROR_LABEL /* no NULL stack pointers */
/* Insert the argument onto the new stack. */
+ movq %rcx,-8(%rsi)
+
subq $16,%rsi
- movq %rcx,8(%rsi)
/* Save the function pointer. It will be popped off in the
child. */