* posix_spawn and vfork @ 2019-05-02 12:18 Florian Weimer 2019-05-02 13:22 ` Adhemerval Zanella 0 siblings, 1 reply; 10+ messages in thread From: Florian Weimer @ 2019-05-02 12:18 UTC (permalink / raw) To: libc-alpha; +Cc: Adhemerval Zanella Why do we use clone instead of vfork in posix_spawn? I think we could use vfork and a call to a separate function for the subprocess code, as a compiler barrier, and therefore avoiding stack clobbering issues. As a result, we can reuse the existing stack and would not have to allocate a new one. If this reasoning is incorrect, I'll gladly write up the explanation as a new comment for sysdeps/unix/sysv/linux/spawni.c. Thanks, Florian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-02 12:18 posix_spawn and vfork Florian Weimer @ 2019-05-02 13:22 ` Adhemerval Zanella 2019-05-04 9:07 ` Florian Weimer 0 siblings, 1 reply; 10+ messages in thread From: Adhemerval Zanella @ 2019-05-02 13:22 UTC (permalink / raw) To: Florian Weimer, libc-alpha On 02/05/2019 09:18, Florian Weimer wrote: > Why do we use clone instead of vfork in posix_spawn? Besides being deprecated by POSIX, the main issue currently it lacks a *proper* way to setup a correct stack frame for helper process. Even with a function, we are relying on undefined semantic that compiler won't clobber the stack. > > I think we could use vfork and a call to a separate function for the > subprocess code, as a compiler barrier, and therefore avoiding stack > clobbering issues. Why circle back to vfork? Current clone usage already provides all the required work avoid all the pitfalls we worked to fix for posix_spawn. > > As a result, we can reuse the existing stack and would not have to > allocate a new one. Is it really a bottleneck you think work of using a deprecated and tricky interface instead? Also, we will need to take of the compatibility handling (maybe_script_execute) which is the main culprit of the stack requirement (besides -fstack-check). One possibility is add a compat symbol and remove maybe_script_execute, but not sure if worth the trouble. > > If this reasoning is incorrect, I'll gladly write up the explanation as > a new comment for sysdeps/unix/sysv/linux/spawni.c. > > Thanks, > Florian > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-02 13:22 ` Adhemerval Zanella @ 2019-05-04 9:07 ` Florian Weimer 2019-05-04 15:59 ` Paul Eggert 2019-05-06 12:34 ` Adhemerval Zanella 0 siblings, 2 replies; 10+ messages in thread From: Florian Weimer @ 2019-05-04 9:07 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha * Adhemerval Zanella: > On 02/05/2019 09:18, Florian Weimer wrote: >> Why do we use clone instead of vfork in posix_spawn? > > Besides being deprecated by POSIX, the main issue currently it lacks > a *proper* way to setup a correct stack frame for helper process. > Even with a function, we are relying on undefined semantic that compiler > won't clobber the stack. We could specify the returns_twice attribute, then the compiler should make it work. I would even go so far as to say that doing this is worthwhile because it increases test coverage for our vfork implementation. I think we need to provide a working vfork. >> As a result, we can reuse the existing stack and would not have to >> allocate a new one. > > Is it really a bottleneck you think work of using a deprecated and > tricky interface instead? Also, we will need to take of the compatibility > handling (maybe_script_execute) which is the main culprit of the stack > requirement (besides -fstack-check). One possibility is add a compat > symbol and remove maybe_script_execute, but not sure if worth the trouble. If we share the stack, like vfork does, all that stack-management code isn't necessary anymore. Thanks, Florian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-04 9:07 ` Florian Weimer @ 2019-05-04 15:59 ` Paul Eggert 2019-05-06 12:41 ` Adhemerval Zanella 2019-05-06 12:34 ` Adhemerval Zanella 1 sibling, 1 reply; 10+ messages in thread From: Paul Eggert @ 2019-05-04 15:59 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella; +Cc: libc-alpha Florian Weimer wrote: > doing this is worthwhile because > it increases test coverage for our vfork implementation. I think we > need to provide a working vfork. Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-04 15:59 ` Paul Eggert @ 2019-05-06 12:41 ` Adhemerval Zanella 2019-05-06 12:50 ` Florian Weimer 0 siblings, 1 reply; 10+ messages in thread From: Adhemerval Zanella @ 2019-05-06 12:41 UTC (permalink / raw) To: Paul Eggert, Florian Weimer; +Cc: libc-alpha On 04/05/2019 12:59, Paul Eggert wrote: > Florian Weimer wrote: > >> doing this is worthwhile because >> it increases test coverage for our vfork implementation. I think we >> need to provide a working vfork. > > Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future. That does not mean we should *promote* the interface, which has a lot of downsides and are tricky to use correctly. If we do want to support a vfork-like interface I would work toward Zack Weinberg suggestion to add a callback 'pre-fork' routine and extend it to make it possible to user supply a stack. Something like: pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize); It will make the required libc handling (disable asynchronous cancellation, block all signals, call the syscall primitive with required flags, restore signal handling, re-enable asynchronous cancellation, handle error state). The stack is optional and using NULL will make it use a automatic allocate one (not sure if issuing a VLA is worth based on stacksize or use a pre-defined value). With some care it would be possible to use on even signal handling (assuming the callback only uses async-signal-safe functions), even on sigaltstack signal handlers. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-06 12:41 ` Adhemerval Zanella @ 2019-05-06 12:50 ` Florian Weimer 2019-05-06 19:58 ` Adhemerval Zanella 0 siblings, 1 reply; 10+ messages in thread From: Florian Weimer @ 2019-05-06 12:50 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Paul Eggert, libc-alpha * Adhemerval Zanella: > On 04/05/2019 12:59, Paul Eggert wrote: >> Florian Weimer wrote: >> >>> doing this is worthwhile because >>> it increases test coverage for our vfork implementation. I think we >>> need to provide a working vfork. >> >> Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future. > > That does not mean we should *promote* the interface, which has a lot > of downsides and are tricky to use correctly. If we do want to support > a vfork-like interface I would work toward Zack Weinberg suggestion to add > a callback 'pre-fork' routine and extend it to make it possible to user > supply a stack. > > Something like: > > pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize); > > It will make the required libc handling (disable asynchronous cancellation, > block all signals, call the syscall primitive with required flags, restore > signal handling, re-enable asynchronous cancellation, handle error state). > The stack is optional and using NULL will make it use a automatic allocate > one (not sure if issuing a VLA is worth based on stacksize or use a > pre-defined value). If we want to go this route, I'd suggest something like this as a building block: pid_t clone_samestack (unsigned int flags, void (*action) (void *) void *closure); and it would be a fatal error if ACTION (CLOSURE) returns. The function would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK. Once we have that, the need for explicit stack management goes away. Thanks, Florian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-06 12:50 ` Florian Weimer @ 2019-05-06 19:58 ` Adhemerval Zanella 2019-05-07 8:51 ` Florian Weimer 0 siblings, 1 reply; 10+ messages in thread From: Adhemerval Zanella @ 2019-05-06 19:58 UTC (permalink / raw) To: Florian Weimer; +Cc: Paul Eggert, libc-alpha On 06/05/2019 09:50, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 04/05/2019 12:59, Paul Eggert wrote: >>> Florian Weimer wrote: >>> >>>> doing this is worthwhile because >>>> it increases test coverage for our vfork implementation. I think we >>>> need to provide a working vfork. >>> >>> Yes; regardless of whether we use vfork to implement posix_spawn, too many programs rely on vfork to withdraw it any time in the foreseeable future. >> >> That does not mean we should *promote* the interface, which has a lot >> of downsides and are tricky to use correctly. If we do want to support >> a vfork-like interface I would work toward Zack Weinberg suggestion to add >> a callback 'pre-fork' routine and extend it to make it possible to user >> supply a stack. >> >> Something like: >> >> pid_t xfork (void *(*pre_fork (void *), void *stack, size_t stacksize); >> >> It will make the required libc handling (disable asynchronous cancellation, >> block all signals, call the syscall primitive with required flags, restore >> signal handling, re-enable asynchronous cancellation, handle error state). >> The stack is optional and using NULL will make it use a automatic allocate >> one (not sure if issuing a VLA is worth based on stacksize or use a >> pre-defined value). > > If we want to go this route, I'd suggest something like this as a > building block: > > pid_t clone_samestack (unsigned int flags, void (*action) (void *) > void *closure); > > and it would be a fatal error if ACTION (CLOSURE) returns. The function > would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK. > > Once we have that, the need for explicit stack management goes away. But would it just a shim wrapper over clone (to handle the required kABI) as vfork currently is or something more sane as posix_spawn to work correctly along libc? Also, would be just an internal interface or a possible extension? Because for a clone wrapper, I think would be simpler to just make using clone instead by allowing it accept a null stack input. --- struct wrapper_argument { void (*action) (void *) fn; void *arg; }; void wrapper (void *input) { struct wrapper_argument *arg = input; arg->fn (arg->arg); abort (); } pid_t clone_samestack (unsigned int flags, void (*action) (void *) void *closure) { int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD; #ifndef __ia64__ return clone (wrapper, NULL, clone_flags, &(struct wrapper_argument) { action, closure })); #else return __clone2 (wrapper, NULL, sizeof stack, &(struct wrapper_argument) { action, closure })); #endif } --- It also wouldn't require potentially another arch-specific assembly implementation. However my main issue with such interface where is reuses the stack from parent process is how safe this construction is. That's why I think the xfork symbol, which either allocates a pre-defined stack or accepts a user-define one (as for sigaltstack) should be safer and play along libc interfaces better (it could either share some code with posix_spawn). ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-06 19:58 ` Adhemerval Zanella @ 2019-05-07 8:51 ` Florian Weimer 2019-05-07 13:32 ` Adhemerval Zanella 0 siblings, 1 reply; 10+ messages in thread From: Florian Weimer @ 2019-05-07 8:51 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Paul Eggert, libc-alpha * Adhemerval Zanella: >> If we want to go this route, I'd suggest something like this as a >> building block: >> >> pid_t clone_samestack (unsigned int flags, void (*action) (void *) >> void *closure); >> >> and it would be a fatal error if ACTION (CLOSURE) returns. The function >> would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK. >> >> Once we have that, the need for explicit stack management goes away. > > But would it just a shim wrapper over clone (to handle the required kABI) > as vfork currently is or something more sane as posix_spawn to work > correctly along libc? Also, would be just an internal interface or > a possible extension? I think it would make sense to start out with the thin wrapper, and then build the safer vfork abstraction on top of that. > Because for a clone wrapper, I think would be simpler to just make using > clone instead by allowing it accept a null stack input. > > --- > struct wrapper_argument > { > void (*action) (void *) fn; > void *arg; > }; > > void wrapper (void *input) > { > struct wrapper_argument *arg = input; > arg->fn (arg->arg); > abort (); > } > > pid_t clone_samestack (unsigned int flags, void (*action) (void *) > void *closure) > { > int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD; > #ifndef __ia64__ > return clone (wrapper, NULL, clone_flags, > &(struct wrapper_argument) { action, closure })); > #else > return __clone2 (wrapper, NULL, sizeof stack, > &(struct wrapper_argument) { action, closure })); > #endif > } > > --- > > It also wouldn't require potentially another arch-specific assembly > implementation. For the x86-64 and i386 implementations, it's not just about removing the NULL check. I think we will need different trampoline functions, with different unwinding data. At that point, we can just rewrite the whole function. > However my main issue with such interface where is reuses the stack from > parent process is how safe this construction is. That's why I think the > xfork symbol, which either allocates a pre-defined stack or accepts a > user-define one (as for sigaltstack) should be safer and play along > libc interfaces better (it could either share some code with posix_spawn). I think it boils down to whether the parent thread is reliably stopped during vfork (if it is not implemented as fork). I have not seen any indication so far that the blocking is unreliable in current (3.x) kernels. In contrast, when sizing the separate stack, you need to make educated guesses about ld.so stack space requirements. Ideally, you would also add a guard page. The new stack may need to be communicated to memory debuggers. And so on. All in all, stack switching seems more risky to me. Thanks, Florian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-07 8:51 ` Florian Weimer @ 2019-05-07 13:32 ` Adhemerval Zanella 0 siblings, 0 replies; 10+ messages in thread From: Adhemerval Zanella @ 2019-05-07 13:32 UTC (permalink / raw) To: Florian Weimer; +Cc: Paul Eggert, libc-alpha On 07/05/2019 05:50, Florian Weimer wrote: > * Adhemerval Zanella: > >>> If we want to go this route, I'd suggest something like this as a >>> building block: >>> >>> pid_t clone_samestack (unsigned int flags, void (*action) (void *) >>> void *closure); >>> >>> and it would be a fatal error if ACTION (CLOSURE) returns. The function >>> would fail with EINVAL if FLAGS contained CLONE_VM without CLONE_VFORK. >>> >>> Once we have that, the need for explicit stack management goes away. >> >> But would it just a shim wrapper over clone (to handle the required kABI) >> as vfork currently is or something more sane as posix_spawn to work >> correctly along libc? Also, would be just an internal interface or >> a possible extension? > > I think it would make sense to start out with the thin wrapper, and then > build the safer vfork abstraction on top of that. > >> Because for a clone wrapper, I think would be simpler to just make using >> clone instead by allowing it accept a null stack input. >> >> --- >> struct wrapper_argument >> { >> void (*action) (void *) fn; >> void *arg; >> }; >> >> void wrapper (void *input) >> { >> struct wrapper_argument *arg = input; >> arg->fn (arg->arg); >> abort (); >> } >> >> pid_t clone_samestack (unsigned int flags, void (*action) (void *) >> void *closure) >> { >> int clone_flags = CLONE_VM | CLONE_VFORK | SIGCHLD; >> #ifndef __ia64__ >> return clone (wrapper, NULL, clone_flags, >> &(struct wrapper_argument) { action, closure })); >> #else >> return __clone2 (wrapper, NULL, sizeof stack, >> &(struct wrapper_argument) { action, closure })); >> #endif >> } >> >> --- >> >> It also wouldn't require potentially another arch-specific assembly >> implementation. > > For the x86-64 and i386 implementations, it's not just about removing > the NULL check. I think we will need different trampoline functions, > with different unwinding data. At that point, we can just rewrite the > whole function. It is still better than add *another* assembly required implementation that would need to be implemented or adjusted for every port. > >> However my main issue with such interface where is reuses the stack from >> parent process is how safe this construction is. That's why I think the >> xfork symbol, which either allocates a pre-defined stack or accepts a >> user-define one (as for sigaltstack) should be safer and play along >> libc interfaces better (it could either share some code with posix_spawn). > > I think it boils down to whether the parent thread is reliably stopped > during vfork (if it is not implemented as fork). I have not seen any > indication so far that the blocking is unreliable in current (3.x) > kernels. My problem is the blurry state the new process is created due the CLONE_VM, where although it stops the calling process, it still needs to handle shared states being modified by standard functions (such as malloc) or even with more complicated states (dynamic loading, dynamic binding, etc). It gets even more complicate when we started to handle more complex scenarios, such cancellation, unwinding, non async-safe functions, etc. > > In contrast, when sizing the separate stack, you need to make educated > guesses about ld.so stack space requirements. Ideally, you would also > add a guard page. The new stack may need to be communicated to memory > debuggers. And so on. All in all, stack switching seems more risky to > me. For generic case I do agree dynamic binding does impose some issues (although golang with split-stack already set some precedent that it somewhat feasible to pick a stack size for this). In any case, I think we are deviating to the point of what would be the real usercases for a vfork-like interface. Currently AFAIK vfork is used mainly as a replacement of fork for performance-wise improvement in process spawning. It is something that I do think it would be better served with posix_spawn and well-thought extension (as we are doing in latest glibc releases). So do you think we will different vfork scenarios that posix_spawn is not a possible fit (even with extended attribute or file actions)? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: posix_spawn and vfork 2019-05-04 9:07 ` Florian Weimer 2019-05-04 15:59 ` Paul Eggert @ 2019-05-06 12:34 ` Adhemerval Zanella 1 sibling, 0 replies; 10+ messages in thread From: Adhemerval Zanella @ 2019-05-06 12:34 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha On 04/05/2019 06:07, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 02/05/2019 09:18, Florian Weimer wrote: >>> Why do we use clone instead of vfork in posix_spawn? >> >> Besides being deprecated by POSIX, the main issue currently it lacks >> a *proper* way to setup a correct stack frame for helper process. >> Even with a function, we are relying on undefined semantic that compiler >> won't clobber the stack. > > We could specify the returns_twice attribute, then the compiler should > make it work. > > I would even go so far as to say that doing this is worthwhile because > it increases test coverage for our vfork implementation. I think we > need to provide a working vfork. The return_twice should indeed help the compiler support, but it would still need to rely in proper compiler compiler support through an extension which is in most places sketchy (it may fail in some non define scenario or with a compiler that does not fully support the attribute). > >>> As a result, we can reuse the existing stack and would not have to >>> allocate a new one. >> >> Is it really a bottleneck you think work of using a deprecated and >> tricky interface instead? Also, we will need to take of the compatibility >> handling (maybe_script_execute) which is the main culprit of the stack >> requirement (besides -fstack-check). One possibility is add a compat >> symbol and remove maybe_script_execute, but not sure if worth the trouble. > > If we share the stack, like vfork does, all that stack-management code > isn't necessary anymore. The current stack allocation thought mmap is only required to handle the compat handling of script without shebang. We can optimize it by moving it to a compat symbol and providing a simple pre-defined buffer with enough stack size so __spawni_child can allocate its automatic variables. I will try to spend some time to work on this this week. > > Thanks, > Florian > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-07 13:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-02 12:18 posix_spawn and vfork Florian Weimer 2019-05-02 13:22 ` Adhemerval Zanella 2019-05-04 9:07 ` Florian Weimer 2019-05-04 15:59 ` Paul Eggert 2019-05-06 12:41 ` Adhemerval Zanella 2019-05-06 12:50 ` Florian Weimer 2019-05-06 19:58 ` Adhemerval Zanella 2019-05-07 8:51 ` Florian Weimer 2019-05-07 13:32 ` Adhemerval Zanella 2019-05-06 12:34 ` Adhemerval Zanella
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).