Re: [PATCH 2/2] pasta: Clean up waiting pasta child on failures

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5416 bytes --]

On Thu, Dec 11, 2025 at 02:49:50PM +0100, Paul Holzinger wrote:
> 
> On 11/12/2025 00:45, David Gibson wrote:
> > On Wed, Dec 10, 2025 at 01:05:10PM +0100, Paul Holzinger wrote:
> > > On 10/12/2025 06:15, David Gibson wrote:
> > > > When pasta is invoked with a command rather than an existing namespace to
> > > > attach to, it spawns a child process to run a shell or other command.  We
> > > > create that process during conf(), since we need the namespace to exist for
> > > > much of our setup.  However, we don't want the specified command to run
> > > > until the pasta network interface is ready for use.  Therefore,
> > > > pasta_spawn_cmd() executing in the child waits before exec()ing.  main()
> > > > signals the child to continue with SIGUSR1 shortly before entering the
> > > > main forwarding loop.
> > > > 
> > > > This has the downside that if we exit due to any kind of failure between
> > > > conf() and the SIGUSR1, the child process will be around waiting
> > > > indefinitely.  The user must manually clean this up.
> > > > 
> > > > Make this cleaner, by having passt_exit() terminate the child, when called
> > > > during this window.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >    passt.c | 1 +
> > > >    pasta.c | 2 ++
> > > >    pasta.h | 1 +
> > > >    util.c  | 5 +++++
> > > >    4 files changed, 9 insertions(+)
> > > > 
> > > > diff --git a/passt.c b/passt.c
> > > > index cf38822f..955c7091 100644
> > > > --- a/passt.c
> > > > +++ b/passt.c
> > > > @@ -431,6 +431,7 @@ int main(int argc, char **argv)
> > > >    	if (pasta_child_pid) {
> > > >    		kill(pasta_child_pid, SIGUSR1);
> > > >    		log_stderr = false;
> > > > +		pasta_child_signalled = true;
> > > >    	}
> > > >    	isolate_postfork(&c);
> > > > diff --git a/pasta.c b/pasta.c
> > > > index 5c693de1..8ac4511f 100644
> > > > --- a/pasta.c
> > > > +++ b/pasta.c
> > > > @@ -54,6 +54,8 @@
> > > >    /* PID of child, in case we created a namespace */
> > > >    int pasta_child_pid;
> > > > +/* Has the child been signalled to start a shell or command */
> > > > +bool pasta_child_signalled;
> > > >    /**
> > > >     * pasta_child_handler() - Exit once shell exits (if we started it), reap clones
> > > > diff --git a/pasta.h b/pasta.h
> > > > index 4b063d13..55028c74 100644
> > > > --- a/pasta.h
> > > > +++ b/pasta.h
> > > > @@ -7,6 +7,7 @@
> > > >    #define PASTA_H
> > > >    extern int pasta_child_pid;
> > > > +extern bool pasta_child_signalled;
> > > >    void pasta_open_ns(struct ctx *c, const char *netns);
> > > >    void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> > > > diff --git a/util.c b/util.c
> > > > index e266c396..4744f09e 100644
> > > > --- a/util.c
> > > > +++ b/util.c
> > > > @@ -35,6 +35,7 @@
> > > >    #include "log.h"
> > > >    #include "pcap.h"
> > > >    #include "epoll_ctl.h"
> > > > +#include "pasta.h"
> > > >    #ifdef HAS_GETRANDOM
> > > >    #include <sys/random.h>
> > > >    #endif
> > > > @@ -1235,6 +1236,10 @@ void abort_with_msg(const char *fmt, ...)
> > > >     */
> > > >    void passt_exit(int status)
> > > >    {
> > > > +	/* If we're starting our own namespace, don't leave it in limbo */
> > > > +	if (pasta_child_pid && !pasta_child_signalled)
> > > > +		kill(pasta_child_pid, SIGTERM);
> > > Any reason not to use SIGKILL? Then there is no doubt if it might be ignored
> > > or not.
> > Eh, only that some log might show KILLs on the assumption that they
> > indicate a bad thing happening, which isn't really the case here.
> > 
> > > Another thing I assume the goal here is to only kill the process if we
> > > didn't exec yet.
> > Yes, that was the goal.
> > 
> > > I wonder how much value there is to have the child process
> > > outlive pasta.
> > That's a good question, and I don't have a strong opinion either way.
> > I leant this way based on two factors:
> > 
> >   - If this happens later, once the child is established, it's possible
> >     the user could have started running something there that remains
> >     valuable even if it loses its network
> > 
> >   - If pasta dies once the child has exec()ed, the symptoms are both
> >     more obvious and easier to clean up: the shell (or whatever) is
> >     right there, running, and can be exited in the normal way
> >     (e.g. running to completion or ^C).  Before the exec() this leaves
> >     a process running non-obviously which has to be spotted and killed
> >     explicitly by the user.
> > 
> > (also note that if we do kill the child after exec(), we definitely
> > want to use SIGTERM not SIGKILL to allow it to clean up gracefully).
> 
> sure, I agree though there is bit of a common issue here. The process is in
> a new pid namespace so it acts as pid 1. That means unless the process adds
> a signal handler explicitly a signal like SIGTERM will get ignored by
> default. When running `sleep` as pid 1 it will ignore SIGTERM and you must
> kill it. We see that all the time with podman containers.

Ah, good point.  I guess my earlier draft only worked because we do,
in fact have a SIGTERM handler.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]