[v2] syscalls/ioctl_ns0[156]: align stack and wait for child

Test crashes (SIGBUS) when using child stack have been observed for
ioctl_ns01. Align stack to 64 bytes for all testcases using clone,
which should work for any arch.

Add SIGCHLD to clone flags, so that LTP library can reap all children
and check their return code.  Also check ltp_clone() return value.

Suppress warning for unused *arg in child().

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/ioctl/ioctl_ns01.c |  9 +++++----
 testcases/kernel/syscalls/ioctl/ioctl_ns05.c |  8 +++++---
 testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 11 +++++++----
 3 files changed, 17 insertions(+), 11 deletions(-)

On Tue, Jun 11, 2019 at 6:25 PM Jan Stancek <jstancek@redhat.com> wrote:

> Test crashes (SIGBUS) when using child stack have been observed for
> ioctl_ns01. Align stack to 64 bytes for all testcases using clone,
> which should work for any arch.


> Add SIGCHLD to clone flags, so that LTP library can reap all children
> and check their return code.  Also check ltp_clone() return value.
>
> Suppress warning for unused *arg in child().
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
>

Reviewed-by: Li Wang <liwang@redhat.com>

---
>  testcases/kernel/syscalls/ioctl/ioctl_ns01.c |  9 +++++----
>  testcases/kernel/syscalls/ioctl/ioctl_ns05.c |  8 +++++---
>  testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 11 +++++++----
>  3 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c
> b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c
> index dfde4da6c5d6..a6ff57d4cbf9 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c
> @@ -23,7 +23,7 @@
>
>  #define STACK_SIZE (1024 * 1024)
>
> -static char child_stack[STACK_SIZE];
> +static char child_stack[STACK_SIZE] __attribute__((aligned(64)));
>
>  static void setup(void)
>  {
> @@ -53,7 +53,7 @@ static void test_ns_get_parent(void)
>         }
>  }
>
> -static int child(void *arg)
> +static int child(void *arg LTP_ATTRIBUTE_UNUSED)
>  {
>         test_ns_get_parent();
>         return 0;
> @@ -63,8 +63,9 @@ static void run(void)
>  {
>         test_ns_get_parent();
>
> -       ltp_clone(CLONE_NEWPID, &child, 0,
> -               STACK_SIZE, child_stack);
> +       if (ltp_clone(CLONE_NEWPID | SIGCHLD, &child, 0,
> +               STACK_SIZE, child_stack) == -1)
> +               tst_brk(TBROK | TERRNO, "ltp_clone failed");
>  }
>
>  static struct tst_test test = {
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns05.c
> b/testcases/kernel/syscalls/ioctl/ioctl_ns05.c
> index a8dee07a1154..685a5f683b25 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_ns05.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns05.c
> @@ -22,7 +22,7 @@
>
>  #define STACK_SIZE (1024 * 1024)
>
> -static char child_stack[STACK_SIZE];
> +static char child_stack[STACK_SIZE] __attribute__((aligned(64)));
>
>  static void setup(void)
>  {
> @@ -32,7 +32,7 @@ static void setup(void)
>                 tst_res(TCONF, "namespace not available");
>  }
>
> -static int child(void *arg)
> +static int child(void *arg LTP_ATTRIBUTE_UNUSED)
>  {
>         if (getpid() != 1)
>                 tst_res(TFAIL, "child should think its pid is 1");
> @@ -44,8 +44,10 @@ static int child(void *arg)
>
>  static void run(void)
>  {
> -       pid_t pid = ltp_clone(CLONE_NEWPID, &child, 0,
> +       pid_t pid = ltp_clone(CLONE_NEWPID | SIGCHLD, &child, 0,
>                 STACK_SIZE, child_stack);
> +       if (pid == -1)
> +               tst_brk(TBROK | TERRNO, "ltp_clone failed");
>
>         char child_namespace[20];
>         int my_fd, child_fd, parent_fd;
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns06.c
> b/testcases/kernel/syscalls/ioctl/ioctl_ns06.c
> index 805a0a072e2f..bf5800434723 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_ns06.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns06.c
> @@ -23,7 +23,7 @@
>
>  #define STACK_SIZE (1024 * 1024)
>
> -static char child_stack[STACK_SIZE];
> +static char child_stack[STACK_SIZE] __attribute__((aligned(64)));
>
>  static void setup(void)
>  {
> @@ -33,7 +33,7 @@ static void setup(void)
>                 tst_res(TCONF, "namespace not available");
>  }
>
> -static int child(void *arg)
> +static int child(void *arg LTP_ATTRIBUTE_UNUSED)
>  {
>         TST_CHECKPOINT_WAIT(0);
>         return 0;
> @@ -41,10 +41,13 @@ static int child(void *arg)
>
>  static void run(void)
>  {
> -       pid_t pid = ltp_clone(CLONE_NEWUSER, &child, 0,
> -               STACK_SIZE, child_stack);
>         char child_namespace[20];
>
> +       pid_t pid = ltp_clone(CLONE_NEWUSER | SIGCHLD, &child, 0,
> +               STACK_SIZE, child_stack);
> +       if (pid == -1)
> +               tst_brk(TBROK | TERRNO, "ltp_clone failed");
> +
>         sprintf(child_namespace, "/proc/%i/ns/user", pid);
>         int my_fd, child_fd, parent_fd;
>
> --
> 1.8.3.1
>
>

Hi!
> Test crashes (SIGBUS) when using child stack have been observed for
> ioctl_ns01. Align stack to 64 bytes for all testcases using clone,
> which should work for any arch.

Looking at the rest of the test it seems that all of them use malloc()
to allocate the child stack and depends on the libc to align the
buffers, maybe it would be easier to change these tests to use malloc()
as well.

> Add SIGCHLD to clone flags, so that LTP library can reap all children
> and check their return code.  Also check ltp_clone() return value.
> 
> Suppress warning for unused *arg in child().

The rest is OK.

----- Original Message -----
> Hi!
> > Test crashes (SIGBUS) when using child stack have been observed for
> > ioctl_ns01. Align stack to 64 bytes for all testcases using clone,
> > which should work for any arch.
> 
> Looking at the rest of the test it seems that all of them use malloc()
> to allocate the child stack and depends on the libc to align the
> buffers, maybe it would be easier to change these tests to use malloc()
> as well.

Default alignment is not enough:
  Alignment:                              2 * sizeof(size_t) (default)
       (i.e., 8 byte alignment with 4byte size_t). This suffices for
       nearly all current machines and C compilers. However, you can
       define MALLOC_ALIGNMENT to be wider than this if necessary.

I'm guessing most of tests cross M_MMAP_THRESHOLD, and get page alignment
from mmap. But should we rely on that?

How about posix_memalign()?

> 
> > Add SIGCHLD to clone flags, so that LTP library can reap all children
> > and check their return code.  Also check ltp_clone() return value.
> > 
> > Suppress warning for unused *arg in child().
> 
> The rest is OK.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>

Hi!
> Default alignment is not enough:
>   Alignment:                              2 * sizeof(size_t) (default)
>        (i.e., 8 byte alignment with 4byte size_t). This suffices for
>        nearly all current machines and C compilers. However, you can
>        define MALLOC_ALIGNMENT to be wider than this if necessary.
> 
> I'm guessing most of tests cross M_MMAP_THRESHOLD, and get page alignment
> from mmap. But should we rely on that?
> 
> How about posix_memalign()?

I guess that we need to fix the ltp_clone_malloc() function as well, we
should probably add a helper to allocate memory to the clone library
which would make use of the posix_memalign() and make use of it
internally as well...