Avoiding threads in the daemon

Avoiding threads in the daemon

[Ludovic Courtès]

Nix commit 524f89 changed libstore to use fork + unshare instead of
clone(2).  The problem is that, in doing so, it also removed use of
CLONE_NEWPID and thus, (1) the build process no longer has PID 1, and
(2) build processes end up in the global PID space.

Adding CLONE_NEWPID to the unshare(2) call appears to break things (for
instance, future calls to pthread_create by that process fail with
EINVAL, other calls to clone(2) fail with ENOMEN) which may be why
CLONE_NEWPID isn’t used here.

The stated reason for this commit is this:

 commit 524f89f1399724e596f61faba2c6861b1bb7b9c5
 Author: Eelco Dolstra <eelco.dolstra@logicblox.com>
 Date:   Thu Aug 21 14:08:09 2014 +0200

     Use unshare() instead of clone()

     It turns out that using clone() to start a child process is unsafe in
     a multithreaded program. It can cause the initialisation of a build
     child process to hang in setgroups(), as seen several times in the
     build farm:

     The reason is that Glibc thinks that the other threads of the parent
     exist in the child, so in setxid_mark_thread() it tries to get a futex
     that has been acquired by another thread just before the clone(). With
     fork(), Glibc runs pthread_atfork() handlers that take care of this
     (in particular, __reclaim_stacks()). But clone() doesn't do that.

     Fortunately, we can use fork()+unshare() instead of clone() to set up
     private namespaces.

     See also https://www.mail-archive.com/lxc-devel@lists.linuxcontainers.org/msg03434.html.

The more general issue is that fork should not be used in a
multi-threaded process, unless the child immediately calls exec* after
fork (POSIX clearly specifies that if a multi-threaded program forks,
the child must only call functions that are async-signal-safe.)  IOW,
the daemon should not use threads in the first place.

Thus, I think Nix commit 49fe95 (which introduces monitor-fd.hh, which
uses std::thread just for convenience) should be reverted, along with
the subsequent commits to that file; then commit 524f89 can be reverted.

WDYT?

Thanks,
Ludo’.
_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Eelco Dolstra]

Hi,

On 18/12/14 17:32, Ludovic Courtès wrote:

> Thus, I think Nix commit 49fe95 (which introduces monitor-fd.hh, which
> uses std::thread just for convenience) should be reverted, along with
> the subsequent commits to that file; then commit 524f89 can be reverted.

I really don't want to get rid of threads because they're useful and I want to
use them more in the future (e.g. build.cc would be much simpler if it used
threads rather than the current event-driven approach; nix-daemon could handle
client connections with a thread rather than a process; etc.).

I see a few ways to get PID namespaces back:

* Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT) (after
which the intermediate process can exit).

* Call setuid/setgid via syscall() to bypass the locking in the Glibc wrappers.
However, there might be other problematic functions so this is not a great solution.

* Get the Glibc folks to provide a way to run at-fork handlers with clone().

Clearly the first option is the easiest.

-- 
Eelco Dolstra | LogicBlox, Inc. | http://nixos.org/~eelco/
_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Shea Levy]

Can't you unshare in the parent then setns back after fork?



> On Dec 19, 2014, at 18:20, Eelco Dolstra <eelco.dolstra@logicblox.com> wrote:
> 
> Hi,
> 
>> On 18/12/14 17:32, Ludovic Courtès wrote:
>> 
>> Thus, I think Nix commit 49fe95 (which introduces monitor-fd.hh, which
>> uses std::thread just for convenience) should be reverted, along with
>> the subsequent commits to that file; then commit 524f89 can be reverted.
> 
> I really don't want to get rid of threads because they're useful and I want to
> use them more in the future (e.g. build.cc would be much simpler if it used
> threads rather than the current event-driven approach; nix-daemon could handle
> client connections with a thread rather than a process; etc.).
> 
> I see a few ways to get PID namespaces back:
> 
> * Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT) (after
> which the intermediate process can exit).
> 
> * Call setuid/setgid via syscall() to bypass the locking in the Glibc wrappers.
> However, there might be other problematic functions so this is not a great solution.
> 
> * Get the Glibc folks to provide a way to run at-fork handlers with clone().
> 
> Clearly the first option is the easiest.
> 
> -- 
> Eelco Dolstra | LogicBlox, Inc. | http://nixos.org/~eelco/
> _______________________________________________
> nix-dev mailing list
> nix-dev@lists.science.uu.nl
> http://lists.science.uu.nl/mailman/listinfo/nix-dev
_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Eelco Dolstra]

Hi,

On 19/12/14 19:41, Shea Levy wrote:

> Can't you unshare in the parent then setns back after fork?

In a multi-threaded program, that sounds incredibly racy :-)

(Though it's not clear to me whether unshare() works on the current process or
the current thread. Manpage says process...)

-- 
Eelco Dolstra | LogicBlox, Inc. | http://nixos.org/~eelco/

Re: Avoiding threads in the daemon

[Ludovic Courtès]

Hi,

Eelco Dolstra <eelco.dolstra@logicblox.com> skribis:

> On 18/12/14 17:32, Ludovic Courtès wrote:
>
>> Thus, I think Nix commit 49fe95 (which introduces monitor-fd.hh, which
>> uses std::thread just for convenience) should be reverted, along with
>> the subsequent commits to that file; then commit 524f89 can be reverted.
>
> I really don't want to get rid of threads because they're useful and I want to
> use them more in the future (e.g. build.cc would be much simpler if it used
> threads rather than the current event-driven approach; nix-daemon could handle
> client connections with a thread rather than a process; etc.).

Hmm, I’m not convinced about the whole threads-for-convenience approach
à la Java.  (I think the ideal is a single-threaded event loop; of
course we want to avoid IoC, and this is where FRP or monads come in.)

> I see a few ways to get PID namespaces back:
>
> * Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT) (after
> which the intermediate process can exit).

Hmm.

> * Call setuid/setgid via syscall() to bypass the locking in the Glibc wrappers.
> However, there might be other problematic functions so this is not a great solution.

Surely.

> * Get the Glibc folks to provide a way to run at-fork handlers with clone().

Hmm.

> Clearly the first option is the easiest.

Yeah.

I think threads are a can of worms anyway and may cause other problems
eventually.

Thanks,
Ludo’.
_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Luca Bruno]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

On Fri, Dec 19, 2014 at 10:31 PM, Ludovic Courtès <ludo@gnu.org> wrote:

>
> Hmm, I’m not convinced about the whole threads-for-convenience approach
> à la Java.  (I think the ideal is a single-threaded event loop; of
> course we want to avoid IoC, and this is where FRP or monads come in.)
>

It is instead for this case. It's not that you have to handle thousands of
connections. Threads simplify a lot compared to an event loop in C/C++.
Don't be fooled by the trends, it doesn't apply here.

[-- Attachment #1.2: Type: text/html, Size: 928 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Alexander Kjeldaas]

[-- Attachment #1.1: Type: text/plain, Size: 1611 bytes --]

On Fri, Dec 19, 2014 at 7:20 PM, Eelco Dolstra <eelco.dolstra@logicblox.com>
wrote:

> Hi,
>
> On 18/12/14 17:32, Ludovic Courtès wrote:
>
> > Thus, I think Nix commit 49fe95 (which introduces monitor-fd.hh, which
> > uses std::thread just for convenience) should be reverted, along with
> > the subsequent commits to that file; then commit 524f89 can be reverted.
>
> I really don't want to get rid of threads because they're useful and I
> want to
> use them more in the future (e.g. build.cc would be much simpler if it used
> threads rather than the current event-driven approach; nix-daemon could
> handle
> client connections with a thread rather than a process; etc.).
>
> I see a few ways to get PID namespaces back:
>
> * Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT)
> (after
> which the intermediate process can exit).
>
> * Call setuid/setgid via syscall() to bypass the locking in the Glibc
> wrappers.
> However, there might be other problematic functions so this is not a great
> solution.
>
> * Get the Glibc folks to provide a way to run at-fork handlers with
> clone().
>
> Clearly the first option is the easiest.
>
>
There is a 4th solution - use a fork()-service. What you do is you fork() a
specific child, the fork()-service, when you start your process, before any
mutex is held and while you control the full state of your program.  You
then communicate with this service using pipes, or your favorite IPC
mechanism.  The fork()-service never starts any thread, and can safely fork
off any child you need.

Alexander

[-- Attachment #1.2: Type: text/html, Size: 2073 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev

Re: Avoiding threads in the daemon

[Eelco Dolstra]

Hi,

On 19/12/14 19:20, Eelco Dolstra wrote:

> I see a few ways to get PID namespaces back:
> 
> * Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT) (after
> which the intermediate process can exit).

This has been implemented in bd0f362d2fad1dd5f28e762011888b5eabd21280.

-- 
Eelco Dolstra | LogicBlox, Inc. | http://nixos.org/~eelco/

Re: Avoiding threads in the daemon

[Ludovic Courtès]

Eelco Dolstra <eelco.dolstra@logicblox.com> skribis:

> On 19/12/14 19:20, Eelco Dolstra wrote:
>
>> I see a few ways to get PID namespaces back:
>> 
>> * Do a regular fork followed by clone(... | CLONE_NEWPID | CLONE_PARENT) (after
>> which the intermediate process can exit).
>
> This has been implemented in bd0f362d2fad1dd5f28e762011888b5eabd21280.

OK, thank you!

Ludo’.
_______________________________________________
nix-dev mailing list
nix-dev@lists.science.uu.nl
http://lists.science.uu.nl/mailman/listinfo/nix-dev