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

Message ID CAMe9rOopEdoNi2dDRMN-DaLC=SyKrJoFgxOZFOzz7oDUY18fHA@mail.gmail.com
State New
Headers show
Series
  • [v2] x86-64: Align child stack to 16 bytes [BZ #27902]
Related show

Commit Message

Florian Weimer via Libc-alpha May 23, 2021, 6:40 p.m.
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.

Comments

Florian Weimer via Libc-alpha May 24, 2021, 12:07 p.m. | #1
* 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
Florian Weimer via Libc-alpha May 24, 2021, 12:53 p.m. | #2
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.
Florian Weimer via Libc-alpha May 24, 2021, 2:19 p.m. | #3
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.
Florian Weimer via Libc-alpha May 24, 2021, 2:55 p.m. | #4
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.

>
Florian Weimer via Libc-alpha May 24, 2021, 6:46 p.m. | #5
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.
Florian Weimer via Libc-alpha May 24, 2021, 6:46 p.m. | #6
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.

Patch

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