[PATCH Shepherd] Factor out a public CALL-IN-FORK.

DoneSubmitted by Attila Lendvai.
Details
5 participants
  • Attila Lendvai
  • Christine Lemmer-Webber
  • Liliana Marie Prikler
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Severity
normal
A
A
Attila Lendvai wrote on 1 Mar 08:06 +0100
(address . guix-patches@gnu.org)(name . Attila Lendvai)(address . attila@lendvai.name)
20220301070615.21028-1-attila@lendvai.name
This enables service implementations to easily inject code that is run before
their service is started. One such example is calling setrlimit from a start
action to set NOFILE (the open files limit), before the service is exec'ed and
thus inherits this value from the parent process, i.e. from Shepherd.

* modules/shepherd/service.scm (fork-and-call): New function.
(fork+exec-command): Use the above.
---
modules/shepherd/service.scm | 51 ++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 22 deletions(-)

Toggle diff (80 lines)
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index ad8608b..8d5e30f 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -79,6 +79,7 @@
             make-forkexec-constructor
             make-kill-destructor
             exec-command
+            fork-and-call
             fork+exec-command
             default-pid-file-timeout
             read-pid-file
@@ -883,19 +884,8 @@ false."
   ;; Signals that the shepherd process handles.
   (list SIGCHLD SIGINT SIGHUP SIGTERM))
 
-(define* (fork+exec-command command
-                            #:key
-                            (user #f)
-                            (group #f)
-                            (supplementary-groups '())
-                            (log-file #f)
-                            (directory (default-service-directory))
-                            (file-creation-mask #f)
-                            (create-session? #t)
-                            (environment-variables
-                             (default-environment-variables)))
-  "Spawn a process that executed COMMAND as per 'exec-command', and return
-its PID."
+(define* (fork-and-call thunk)
+  "Call THUNK in a fork."
   ;; Install the SIGCHLD handler if this is the first fork+exec-command call.
   (unless %sigchld-handler-installed?
     (sigaction SIGCHLD handle-SIGCHLD SA_NOCLDSTOP)
@@ -916,17 +906,34 @@ its PID."
             ;; process.
             (unblock-signals %precious-signals)
 
-            (exec-command command
-                          #:user user
-                          #:group group
-                          #:supplementary-groups supplementary-groups
-                          #:log-file log-file
-                          #:directory directory
-                          #:file-creation-mask file-creation-mask
-                          #:create-session? create-session?
-                          #:environment-variables environment-variables))
+            (thunk))
           pid))))
 
+(define* (fork+exec-command command
+                            #:key
+                            (user #f)
+                            (group #f)
+                            (supplementary-groups '())
+                            (log-file #f)
+                            (directory (default-service-directory))
+                            (file-creation-mask #f)
+                            (create-session? #t)
+                            (environment-variables
+                             (default-environment-variables)))
+  "Spawn a process that executed COMMAND as per 'exec-command', and return
+its PID."
+  (fork-and-call
+   (lambda ()
+     (exec-command command
+                   #:user user
+                   #:group group
+                   #:supplementary-groups supplementary-groups
+                   #:log-file log-file
+                   #:directory directory
+                   #:file-creation-mask file-creation-mask
+                   #:create-session? create-session?
+                   #:environment-variables environment-variables))))
+
 (define* (make-forkexec-constructor command
                                     #:key
                                     (user #f)
-- 
2.34.0
A
A
Attila Lendvai wrote on 1 Mar 08:29 +0100
[PATCH v2] Factor out a public FORK-AND-CALL.
(address . 54205@debbugs.gnu.org)(name . Attila Lendvai)(address . attila@lendvai.name)
20220301072927.26525-1-attila@lendvai.name
This enables service implementations to easily inject code that is run before
their service is started. One such example is calling setrlimit from a start
action to set NOFILE (the open files limit), before the service is exec'ed and
inherits this value from the parent process, i.e. from Shepherd.

* modules/shepherd/service.scm (fork-and-call): New function.
(fork+exec-command): Use the above.
---

v2: fixes the commit message.

modules/shepherd/service.scm | 51 ++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 22 deletions(-)

Toggle diff (80 lines)
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index ad8608b..8d5e30f 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -79,6 +79,7 @@
             make-forkexec-constructor
             make-kill-destructor
             exec-command
+            fork-and-call
             fork+exec-command
             default-pid-file-timeout
             read-pid-file
@@ -883,19 +884,8 @@ false."
   ;; Signals that the shepherd process handles.
   (list SIGCHLD SIGINT SIGHUP SIGTERM))
 
-(define* (fork+exec-command command
-                            #:key
-                            (user #f)
-                            (group #f)
-                            (supplementary-groups '())
-                            (log-file #f)
-                            (directory (default-service-directory))
-                            (file-creation-mask #f)
-                            (create-session? #t)
-                            (environment-variables
-                             (default-environment-variables)))
-  "Spawn a process that executed COMMAND as per 'exec-command', and return
-its PID."
+(define* (fork-and-call thunk)
+  "Call THUNK in a fork."
   ;; Install the SIGCHLD handler if this is the first fork+exec-command call.
   (unless %sigchld-handler-installed?
     (sigaction SIGCHLD handle-SIGCHLD SA_NOCLDSTOP)
@@ -916,17 +906,34 @@ its PID."
             ;; process.
             (unblock-signals %precious-signals)
 
-            (exec-command command
-                          #:user user
-                          #:group group
-                          #:supplementary-groups supplementary-groups
-                          #:log-file log-file
-                          #:directory directory
-                          #:file-creation-mask file-creation-mask
-                          #:create-session? create-session?
-                          #:environment-variables environment-variables))
+            (thunk))
           pid))))
 
+(define* (fork+exec-command command
+                            #:key
+                            (user #f)
+                            (group #f)
+                            (supplementary-groups '())
+                            (log-file #f)
+                            (directory (default-service-directory))
+                            (file-creation-mask #f)
+                            (create-session? #t)
+                            (environment-variables
+                             (default-environment-variables)))
+  "Spawn a process that executed COMMAND as per 'exec-command', and return
+its PID."
+  (fork-and-call
+   (lambda ()
+     (exec-command command
+                   #:user user
+                   #:group group
+                   #:supplementary-groups supplementary-groups
+                   #:log-file log-file
+                   #:directory directory
+                   #:file-creation-mask file-creation-mask
+                   #:create-session? create-session?
+                   #:environment-variables environment-variables))))
+
 (define* (make-forkexec-constructor command
                                     #:key
                                     (user #f)
-- 
2.34.0
L
L
Liliana Marie Prikler wrote on 1 Mar 13:01 +0100
ee29e53f40c550969d3a8a046d6f8dda64598a97.camel@ist.tugraz.at
Am Dienstag, dem 01.03.2022 um 08:29 +0100 schrieb Attila Lendvai:
Toggle quote (5 lines)
> This enables service implementations to easily inject code that is
> run before their service is started.  One such example is calling
> setrlimit from a start action to set NOFILE (the open files limit),
> before the service is exec'ed and inherits this value from the parent
> process, i.e. from Shepherd.
In general, I think such capabilities should be added to exec-command,
rather than resorting to a lambda. It takes a little while to realize
that call-in-fork, fork-and-call or whatever you want to name it is in
fact not pure evil; mainly because shepherd could in its stead already
invoke any lambda you throw at it. That being said, one should always
be aware that this child process runs with the full permissions of
shepherd, which you normally don't want to do for a service.

Toggle quote (5 lines)
> [...]
> +(define* (fork-and-call thunk)
> +  "Call THUNK in a fork."
>    ;; Install the SIGCHLD handler if this is the first fork+exec-
> command call.
This docstring, as well as the procedure name only describe what is
done with thunk in the crudest terms. What's more, I don't think it
makes too much sense to restrict ourselves to thunks if we already run
arbitrary code anyway.

In my opinion, it ought to be
Toggle quote (21 lines)
> +(define* (fork+apply proc . args)
> + "Spawn a process that calls PROC with ARGS and return its PID."
>    (unless %sigchld-handler-installed?
>      (sigaction SIGCHLD handle-SIGCHLD SA_NOCLDSTOP)
> @@ -916,17 +906,34 @@ its PID."
>              ;; process.
>              (unblock-signals %precious-signals)
>  
> -            (exec-command command
> -                          #:user user
> -                          #:group group
> -                          #:supplementary-groups supplementary-
> groups
> -                          #:log-file log-file
> -                          #:directory directory
> -                          #:file-creation-mask file-creation-mask
> -                          #:create-session? create-session?
> -                          #:environment-variables environment-
> variables))
> +            (apply proc args))
>            pid))))
WDYT?
 
Toggle quote (14 lines)
> +(define* (fork+exec-command command
> +                            #:key
> +                            (user #f)
> +                            (group #f)
> +                            (supplementary-groups '())
> +                            (log-file #f)
> +                            (directory (default-service-directory))
> +                            (file-creation-mask #f)
> +                            (create-session? #t)
> +                            (environment-variables
> +                             (default-environment-variables)))
> +  "Spawn a process that executed COMMAND as per 'exec-command', and
> return
> +its PID."
This is just copypasta from a previous mistake, but
s/executed/executes/.

Cheers
M
M
Maxime Devos wrote on 1 Mar 13:47 +0100
Re: [bug#54205] [PATCH Shepherd] Factor out a public CALL-IN-FORK.
cf4063f4844520b276e2ee51e375c036707cd265.camel@telenet.be
Attila Lendvai schreef op di 01-03-2022 om 08:06 [+0100]:
Toggle quote (4 lines)
> their service is started.  One such example is calling setrlimit from a start
> action to set NOFILE (the open files limit), before the service is exec'ed and
> thus inherits this value from the parent process, i.e. from Shepherd.

'fork+exec-command' already accepts a 'environment-variables' and
'file-creation-mask', how about adding an 'open-file-limit' argument?
To me, that seems more declarative and less fragile than having
to call 'call-in-fork' manually in a 'start' procedure (*).

Support for other rlimits can be added on an as-needed basis.
Alternatively, the argument could be generalised to a more general
'rlimit' argument:

#:rlimits
`((,RLIMIT_AS ,SOFT ,HARD)
(,RLIMIT_NPROC ,SOFT ,HARD)
(,RLIMIT_NOFILE ,SOFT ,HARD))

WDYT?

Greetings,
Maxime.

(*) E.g., one of the ideas for making shepherd faster, was using some
kind of multi-threading. Forking when multi-threading is ill-defined
(see POSIX) though, so some kind of zygote process + IPC might be
necessary
nice explanation on zygote processes, the bits about software updates
can be ignored here).
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh4VzRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7k8SAP4gZaaQE/hNFfzGvxdqI+eKxCqc
jXMNaJTOYqkDqloFzwEAo7Eqqzbabm3nzRrq7tfYfRuU63LWiq+0/EBDo5rP4wM=
=jRlT
-----END PGP SIGNATURE-----


A
A
Attila Lendvai wrote on 1 Mar 14:04 +0100
Re: [PATCH v2] Factor out a public FORK-AND-CALL.
(name . Liliana Marie Prikler)(address . liliana.prikler@ist.tugraz.at)(address . 54205@debbugs.gnu.org)
vLp1kLQb-QdCr3XCYBkbjP27y67IPcL-x2n3AmZFwzbmcIl57X-iO-LH-FDlfvhOetPKEPscDuxdhCPYtUNpcfrcxto1x1-OIr-TAyhSN1Y=@lendvai.name
Toggle quote (9 lines)
> In general, I think such capabilities should be added to exec-command,
> rather than resorting to a lambda. It takes a little while to realize
> that call-in-fork, fork-and-call or whatever you want to name it is in
> fact not pure evil; mainly because shepherd could in its stead already
> invoke any lambda you throw at it. That being said, one should always
> be aware that this child process runs with the full permissions of
> shepherd, which you normally don't want to do for a service.


does the above mean that you're concerned about the security implications? if
so, then i don't understand, because Guile already allows calling/accessing
private functions/symbols, and thus this change doesn't really increase the
(already enormous) attack surface in the guile codebase.

it does increase the shoot-oneself-in-the-foot-surface a little bit, though.

it's worth pointing out, though, that trusting a channel, and adding a shepherd
service defined by it to the machine's config, is essentially giving root access
to the channel author. and this is already the case, prior to my change.

BTW, can i not already simply pass 0, or "root" as #:user to EXEC-COMMAND?


Toggle quote (8 lines)
> In my opinion, it ought to be
>
> > +(define* (fork+apply proc . args)
> [...]
>
> WDYT?


makes sense, i'll update the patch... but given the feedback from the two of
you, should i?

i think i'll abandon this, and implement Maxime's #:rlimits suggestion.

i'm not sure how much better that will be, but at least it won't make future
threading harder, and allows me to make progress with my project.

if anyone prefers the FORK+APPLY version, then do speak up!

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“An atheist doesn't have to be someone who thinks he has a proof that there can't be a god. He only has to be someone who believes that the evidence on the God question is at a similar level to the evidence on the werewolf question.”
— John McCarthy (1927–2011), father of Lisp
L
L
Liliana Marie Prikler wrote on 1 Mar 15:01 +0100
(name . Attila Lendvai)(address . attila@lendvai.name)(address . 54205@debbugs.gnu.org)
240241970295ff5351378c915461eea180cc79d5.camel@ist.tugraz.at
Am Dienstag, dem 01.03.2022 um 13:04 +0000 schrieb Attila Lendvai:
Toggle quote (15 lines)
> > In general, I think such capabilities should be added to exec-
> > command, rather than resorting to a lambda. It takes a little while
> > to realize that call-in-fork, fork-and-call or whatever you want to
> > name it is in fact not pure evil; mainly because shepherd could in
> > its stead already invoke any lambda you throw at it. That being
> > said, one should always be aware that this child process runs with
> > the full permissions of shepherd, which you normally don't want to
> > do for a service.
>
>
> does the above mean that you're concerned about the security
> implications? if so, then i don't understand, because Guile already
> allows calling/accessing private functions/symbols, and thus this
> change doesn't really increase the (already enormous) attack surface
> in the guile codebase.
This attack surface is less enormous if you consider the average case
of a shepherd service in which the arguments to fork+exec-command are
already evaluated by the time the procedure is call and thus both
"sane" within and without the fork. Most of the time people are not
too conscious about the fact that shepherd can already run arbitrary
Guile code as part of actions and you typically only use that to its
fullest extent when you're trying to do something real clever.

Toggle quote (10 lines)
> it does increase the shoot-oneself-in-the-foot-surface a little bit,
> though.
>
> it's worth pointing out, though, that trusting a channel, and adding
> a shepherd service defined by it to the machine's config, is
> essentially giving root access to the channel author. and this is
> already the case, prior to my change.
>
> BTW, can i not already simply pass 0, or "root" as #:user to EXEC-
> COMMAND?
Only if you're already root, i.e. this won't work for user shepherds,
which can't become root (easily). On the other hand, I did get my user
shepherd to launch pkexec commands, so that's that.

Toggle quote (19 lines)
>
> > In my opinion, it ought to be
> >
> > > +(define* (fork+apply proc . args)
> > [...]
> >
> > WDYT?
>
> makes sense, i'll update the patch... but given the feedback from the
> two of you, should i?
>
> i think i'll abandon this, and implement Maxime's #:rlimits
> suggestion.
>
> i'm not sure how much better that will be, but at least it won't make
> future threading harder, and allows me to make progress with my
> project.
>
> if anyone prefers the FORK+APPLY version, then do speak up!
FWIW Maxime's complaint would also hold w.r.t. fork+exec-command, which
would then be implemented in terms of fork+apply, so assuming that
fork+exec-command still exists after the switch to multiple threads,
we'd have to patch at least one location either way. fork+apply could
make it so that less hacks are required overall to make all forking
behaviour inside shepherd services as intended, but that's so far only
a theoretical claim with no evidence to back it up.

I think the real question is what you are trying to achieve here. If
you only want to add rlimits, that's an exec-command thing. If you
instead wanted to spawn a Guile function within a sandbox (rather than
a completely new command), that would require something along the lines
of fork+apply at least under the hood. With the things you've
described, I don't think it makes sense (yet) to export fork+apply, but
it might still make sense to refactor fork+exec-command under the hood.

Cheers
C
C
Christine Lemmer-Webber wrote on 1 Mar 18:14 +0100
Re: [bug#54205] [PATCH v2] Factor out a public FORK-AND-CALL.
(name . Liliana Marie Prikler)(address . liliana.prikler@ist.tugraz.at)
87y21tk2yw.fsf@dustycloud.org
Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:

Toggle quote (24 lines)
> Am Dienstag, dem 01.03.2022 um 13:04 +0000 schrieb Attila Lendvai:
>> > In general, I think such capabilities should be added to exec-
>> > command, rather than resorting to a lambda. It takes a little while
>> > to realize that call-in-fork, fork-and-call or whatever you want to
>> > name it is in fact not pure evil; mainly because shepherd could in
>> > its stead already invoke any lambda you throw at it. That being
>> > said, one should always be aware that this child process runs with
>> > the full permissions of shepherd, which you normally don't want to
>> > do for a service.
>>
>>
>> does the above mean that you're concerned about the security
>> implications? if so, then i don't understand, because Guile already
>> allows calling/accessing private functions/symbols, and thus this
>> change doesn't really increase the (already enormous) attack surface
>> in the guile codebase.
> This attack surface is less enormous if you consider the average case
> of a shepherd service in which the arguments to fork+exec-command are
> already evaluated by the time the procedure is call and thus both
> "sane" within and without the fork. Most of the time people are not
> too conscious about the fact that shepherd can already run arbitrary
> Guile code as part of actions and you typically only use that to its
> fullest extent when you're trying to do something real clever.

In general this would be improved if we move Guix in general, and the
Shepherd services in particular, to an object capability based security
model.

It's on my TODO to lay out a sketch for how this could happen, assuming
there's support for it in the community (which I don't expect to go one
way or another until a plan is laid out to talk about).
L
L
Ludovic Courtès wrote on 2 Mar 17:05 +0100
Re: bug#54205: [PATCH Shepherd] Factor out a public CALL-IN-FORK.
(name . Maxime Devos)(address . maximedevos@telenet.be)
87lexs1gs9.fsf_-_@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (10 lines)
> Attila Lendvai schreef op di 01-03-2022 om 08:06 [+0100]:
>> their service is started.  One such example is calling setrlimit from a start
>> action to set NOFILE (the open files limit), before the service is exec'ed and
>> thus inherits this value from the parent process, i.e. from Shepherd.
>
> 'fork+exec-command' already accepts a 'environment-variables' and
> 'file-creation-mask', how about adding an 'open-file-limit' argument?
> To me, that seems more declarative and less fragile than having
> to call 'call-in-fork' manually in a 'start' procedure (*).

Seconded.

Toggle quote (11 lines)
> Support for other rlimits can be added on an as-needed basis.
> Alternatively, the argument could be generalised to a more general
> 'rlimit' argument:
>
> #:rlimits
> `((,RLIMIT_AS ,SOFT ,HARD)
> (,RLIMIT_NPROC ,SOFT ,HARD)
> (,RLIMIT_NOFILE ,SOFT ,HARD))
>
> WDYT?

This interface brings more flexibility, I’m all for it.

Toggle quote (3 lines)
> (*) E.g., one of the ideas for making shepherd faster, was using some
> kind of multi-threading. Forking when multi-threading is ill-defined

I think what we need is concurrency, not POSIX threads. IOW, we can
achieve the concurrency we need without resorting to POSIX threads, for
example using Fibers on a single POSIX thread.

Thanks,
Ludo’.
M
M
Maxime Devos wrote on 2 Mar 19:21 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)
b738e9d8e0ca95ac6ca3fa160c296184de8ddab9.camel@telenet.be
Ludovic Courtès schreef op wo 02-03-2022 om 17:05 [+0100]:
Toggle quote (4 lines)
> I think what we need is concurrency, not POSIX threads.  IOW, we can
> achieve the concurrency we need without resorting to POSIX threads, for
> example using Fibers on a single POSIX thread.

guile-fibers uses threads internally, e.g. in (fibers interrupts).
Interrupts can theoretically be avoided, but that has a downside
that if a start procedure goes into infinite loop (while forgetting to
sleep), the whole shepherd would hang.

I'm not saying that we need POSIX threads per-se -- I find
'choice-operation', 'perform-operation', the channel operations and
Fibers conditions much more convenient than the (lack of) POSIX
equivalents, but I'd prefer avoiding the assumption of single-threading
where feasible, to make it ourselves not harder than necessary in the
future, in case it turns out we need POSIX threading somewhere (even if
only as an implementation detail).

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYh+1mhccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7gmQAQD4VnFmox1iVeL7BooUkGDLkxus
jAqzpyn4eRhC25N30AEAn/ljEQuxDDlnauqTSLJUrUu+i3W7Q90JqxSZhYzNjAk=
=WTNF
-----END PGP SIGNATURE-----


A
A
Attila Lendvai wrote on 3 Mar 09:04 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)
hZFNTyQIwzrwyspkxLI5yZ6W--ibf9ofl7RhYFz2enTHsbqqm9OLu2HSsxXwYCx1atDABjGBStwDBlDJbstjThDiyujUmDNqKDyII2H2q_s=@lendvai.name
Toggle quote (15 lines)
> > Support for other rlimits can be added on an as-needed basis.
> >
> > Alternatively, the argument could be generalised to a more general
> > 'rlimit' argument:
> >
> > #:rlimits
> > `((,RLIMIT_AS ,SOFT ,HARD)
> > (,RLIMIT_NPROC ,SOFT ,HARD)
> > (,RLIMIT_NOFILE ,SOFT ,HARD))
> >
> > WDYT?
>
> This interface brings more flexibility, I’m all for it.


FTR, i've filed this as a patch (with a test!).


--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“It is a miracle that curiosity survives formal education. It is a very grave mistake to think that the enjoyment of seeing and searching can be promoted by means of coercion and a sense of duty.”
— Albert Einstein (1879–1955), 'Autobiographical Notes' (1949), slightly paraphrased
L
L
Ludovic Courtès wrote on 21 Mar 14:03 +0100
control message for bug #54205
(address . control@debbugs.gnu.org)
8735jblap4.fsf@gnu.org
tags 54205 + wontfix
quit
L
L
Ludovic Courtès wrote on 21 Mar 14:03 +0100
Re: bug#54205: [PATCH Shepherd] Factor out a public CALL-IN-FORK.
(name . Attila Lendvai)(address . attila@lendvai.name)
874k3rlaph.fsf_-_@gnu.org
Attila Lendvai <attila@lendvai.name> skribis:

Toggle quote (4 lines)
> FTR, i've filed this as a patch (with a test!).
>
> https://issues.guix.gnu.org/54215

Awesome, closing this one!

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 54205@debbugs.gnu.org