Re: [PATCH v2 4/9] aarch64: Add the clone3 wrapper

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 02/11/22 09:12, Szabolcs Nagy wrote:
> The 09/30/2022 16:26, Adhemerval Zanella via Libc-alpha wrote:
>> It follow the internal signature:
>>
>>   extern int clone3 (struct clone_args *__cl_args, size_t __size,
>>  int (*__func) (void *__arg), void *__arg);
>>
>> And x86_64 semantics to return EINVAL if either cl_args or func
>> is NULL.  The stack is 16-byte aligned prior executing func.
> 
> "x86_64 semantics" sounds wrong: maybe this should be documented?
> i'd expect 0 cl_args/func to be UB like in most posix apis.

Right, I think it is worth to document the function semantic
properly at least on its internal header (include/clone_internal.h).
H.J also added a new clone3.h headers, which is not currently installed
that I am inclined to just remove it from now.  We might reinstate 
if/when we decide to provide the clone3 as an ABI.

And returning EINVAL for 0 cl_args/func aligns with our exported clone
interface, where EINVAL is also returned for 0 function argument.

> 
> and aligning sp in the child fails if signals are allowed there
> (pthreads does not allow signals now, direct callers might).
> i dont know if that's a concert (or if unaligned stack is
> something we should fix up in clone3).

It was overlooked on initial x86_64 clone3 implementation as well.  I
think it better to just return EINVAL for unaligned stacks and avoid
to change the stack pointer in the created thread.

> 
>>
>> Checked on aarch64-linux-gnu.
>> ---
>>  sysdeps/unix/sysv/linux/aarch64/clone3.S | 90 ++++++++++++++++++++++++
>>  sysdeps/unix/sysv/linux/aarch64/sysdep.h |  2 +
>>  2 files changed, 92 insertions(+)
>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/clone3.S
>>
>> diff --git a/sysdeps/unix/sysv/linux/aarch64/clone3.S b/sysdeps/unix/sysv/linux/aarch64/clone3.S
>> new file mode 100644
>> index 0000000000..dba93430eb
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/aarch64/clone3.S
>> @@ -0,0 +1,90 @@
>> +/* The clone3 syscall wrapper.  Linux/aarch64 version.
>> +   Copyright (C) 2022 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 <sysdep.h>
>> +#define _ERRNO_H        1
>> +#include <bits/errno.h>
>> +
>> +/* The userland implementation is:
>> +   int clone3 (struct clone_args *cl_args, size_t size,
>> +               int (*func)(void *arg), void *arg);
>> +
>> +   the kernel entry is:
>> +   int clone3 (struct clone_args *cl_args, size_t size);
>> +
>> +   The parameters are passed in registers from userland:
>> +   x0: cl_args
>> +   x1: size
>> +   x2: func
>> +   x3: arg  */
>> +
>> +        .text
>> +ENTRY(__clone3)
>> +	PTR_ARG (0)
>> +	PTR_ARG (1)
>> +	PTR_ARG (3)
>> +	PTR_ARG (4)
>> +	/* Save args for the child.  */
>> +	mov	x10, x0		/* cl_args  */
>> +	mov	x11, x2		/* func	 */
>> +	mov	x12, x3		/* args  */
>> +
>> +	/* Sanity check args.  */
>> +	mov	x0, #-EINVAL
>> +	cbz	x10, .Lsyscall_error	/* No NULL cl_args pointer.  */
>> +	cbz	x2, .Lsyscall_error	/* No NULL function pointer.  */
>> +
>> +	/* Do the system call, the kernel expects:
>> +	   x8: system call number
>> +	   x0: cl_args
>> +	   x1: size  */
>> +	mov	x0, x10
>> +	mov	x8, #SYS_ify(clone3)
>> +	svc	0x0
>> +
>> +	cmp	x0, #0
>> +	beq	thread_start
>> +	blt	.Lsyscall_error
>> +	RET
>> +PSEUDO_END (__clone3)
>> +
>> +	.align 4
>> +	.type thread_start, %function
>> +thread_start:
>> +	cfi_startproc
>> +	cfi_undefined (x30)
>> +	mov	x29, 0
>> +
>> +	/* Align sp.  */
>> +	mov	x0, sp
>> +	and	x0, x0, -16
>> +	mov	sp, x0
>> +
>> +	/* Pick the function arg and execute.  */
>> +	mov	x0, x12
>> +	blr	x11
>> +
>> +	/* We are done, pass the return value through x0.  */
>> +	mov	x8, #SYS_ify(exit)
>> +	svc	0x0
>> +	cfi_endproc
>> +	.size thread_start, .-thread_start
>> +
>> +libc_hidden_def (__clone3)
>> +weak_alias (__clone3, clone3)
>> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
>> index f1853e012f..42bb22f5e6 100644
>> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
>> @@ -164,6 +164,8 @@
>>  # define HAVE_CLOCK_GETTIME64_VSYSCALL	"__kernel_clock_gettime"
>>  # define HAVE_GETTIMEOFDAY_VSYSCALL	"__kernel_gettimeofday"
>>  
>> +# define HAVE_CLONE3_WRAPPER		1
>> +
>>  # undef INTERNAL_SYSCALL_RAW
>>  # define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
>>    ({ long _sys_result;						\
>> -- 
>> 2.34.1
>>