From: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
To: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: libc-alpha@sourceware.org, Christian Brauner <brauner@kernel.org>,
"H.J. Lu" <hjl.tools@gmail.com>
Subject: Re: [PATCH v2 4/9] aarch64: Add the clone3 wrapper
Date: Thu, 3 Nov 2022 18:22:16 -0300 [thread overview]
Message-ID: <1a5cc9ec-de78-cb4d-3bd3-7f37dc666f73@linaro.org> (raw)
In-Reply-To: <a354d63c-fd0c-7b51-1a97-48dac20dbbef@linaro.org>
On 03/11/22 13:55, Adhemerval Zanella Netto wrote:
>
>
> On 03/11/22 13:52, Szabolcs Nagy wrote:
>> The 11/03/2022 13:39, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 03/11/22 13:31, Szabolcs Nagy wrote:
>>>> The 11/03/2022 13:22, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 03/11/22 11:01, Szabolcs Nagy wrote:
>>>>>> The 11/03/2022 10:15, Adhemerval Zanella Netto wrote:
>>>>>>> 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.
>>>>>>
>>>>>> ok.
>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> long time ago linux did that on aarch64, but it was removed:
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6d9a52543338603e25e71e0e4942f05dae0dd8a
>>>>>>
>>>>>> i think in clone3 the kernel should have aligned (it knows
>>>>>> the bounds now), doing it in the userspace wrapper is weird
>>>>>> (should we adjust the stack size?). and not doing it at all
>>>>>> makes clone3 hard to use portably (user has to know target
>>>>>> specific pcs requirements).
>>>>>>
>>>>>> not sure what's the best way forward.
>>>>>
>>>>> I think the stack size won't matter much here, at least not from
>>>>> kernel point of view (the resulting stack size will most likely
>>>>> be page aligned anyway). But I think this kernel commit makes a good
>>>>> point that silently adjusting the stack in userland is not the
>>>>> correct approach, I think H.J has done to make it consistent with
>>>>> glibc clone implementation which does it.
>>>>>
>>>>> IMHO the best approach would to just remove the stack alignment,
>>>>> since it incurs the signal handling issue.
>>>>
>>>> current generic clone callers dont align the stack and
>>>> e.g. unaligned pthread custom stack should work.
>>>>
>>>> so we have to do arch specific stack alignment somewhere,
>>>> maybe in pthread_create?
>>>
>>> I am thinking on __clone_internal, where if an unaligned stack is
>>> used it creates a new clone_args struct with adjust arguments. It
>>> can adjust the struct in place (not sure which is better).
>>
>> if the api is not exposed, then i think the arg can be modified
>> in place. (if clone3 api is exposed to users then we should not
>> modify user structs unless the clone3 api contract explicitly
>> allows this.)
>>
>> either aligning in pthread_create or __clone_internal works for me,
>> the target specific clone3 syscall should not in case that gets
>> exposed to users.
>>
>
> The arg modification would be done only internally by __clone_internal,
> if we ever export __clone3 it will not mess with stack alignment (my
> idea is to remove it from x86_64 as well).
All the internal usage of __clone_internal are done with all signal masked,
so aligning the stack is currently safe. However, I still think moving out
the stack alignment of __clone3 is still a net gain: it remove an
implementation detail (block/unblock signals) and simplifies the arch-specific
code.
However it makes a possible libc wrap clunky, the caller will need to know
the ABI stack alignment prior to the call since kernel does not automatically
align the stack.
next prev parent reply other threads:[~2022-11-03 21:22 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-30 19:26 [PATCH v2 0/9] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 1/9] linux: Do not reset signal handler in posix_spawn if it is already SIG_DFL Adhemerval Zanella
2023-01-11 21:06 ` Carlos O'Donell
2022-09-30 19:26 ` [PATCH v2 2/9] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn Adhemerval Zanella
2023-01-11 21:06 ` Carlos O'Donell
2022-09-30 19:26 ` [PATCH v2 3/9] powerpc64le: Add the clone3 wrapper Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 4/9] aarch64: " Adhemerval Zanella
2022-11-02 12:12 ` Szabolcs Nagy
2022-11-03 13:15 ` Adhemerval Zanella Netto
2022-11-03 14:01 ` Szabolcs Nagy
2022-11-03 16:22 ` Adhemerval Zanella Netto
2022-11-03 16:31 ` Szabolcs Nagy
2022-11-03 16:39 ` Adhemerval Zanella Netto
2022-11-03 16:52 ` Szabolcs Nagy
2022-11-03 16:55 ` Adhemerval Zanella Netto
2022-11-03 20:55 ` H.J. Lu
2022-11-03 21:28 ` Adhemerval Zanella Netto
2022-11-03 21:22 ` Adhemerval Zanella Netto [this message]
2022-11-03 21:58 ` H.J. Lu
2022-11-04 12:32 ` Adhemerval Zanella Netto
2022-09-30 19:26 ` [PATCH v2 5/9] s390x: " Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 6/9] riscv: " Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 7/9] arm: " Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 8/9] mips: " Adhemerval Zanella
2022-09-30 19:26 ` [PATCH v2 9/9] Linux: optimize clone3 internal usage Adhemerval Zanella
2023-01-11 21:12 ` Carlos O'Donell
2022-10-27 16:48 ` [PATCH v2 0/9] Optimize posix_spawn signal setup with clone3 Adhemerval Zanella Netto
2023-01-11 21:11 ` Carlos O'Donell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1a5cc9ec-de78-cb4d-3bd3-7f37dc666f73@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=brauner@kernel.org \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
--cc=szabolcs.nagy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).