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

[Carlos O'Donell <carlos@redhat.com>]

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.