Re: [PATCH v2] x86-64: Align child stack to 16 bytes [BZ #27902]

["H.J. Lu" <hjl.tools@gmail.com>]

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.