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: 4936 bytes --]

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).

> I feel like using something like PR_SET_PDEATHSIG might be a
> safer option in case pasta crashes without going through passt_exit.

Oh, good idea.  I forgot PR_SET_PDEATHSIG was a thing.

> And if
> you don't want to propagate that into the user command we could unset it
> right again before the exec as well.
> 
> > +
> >   	/* Make sure we don't leave the pcap file truncated */
> >   	if (pcap_fd != -1 && fsync(pcap_fd))
> >   		warn_perror("Failed to flush pcap file, it might be truncated");
> 
> -- 
> Paul Holzinger
> 

-- 
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 --]