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

[Paul Holzinger <pholzing@redhat.com>]

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.

Another thing I assume the goal here is to only kill the process if we 
didn't exec yet. I wonder how much value there is to have the child 
process outlive pasta. I feel like using something like PR_SET_PDEATHSIG 
might be a safer option in case pasta crashes without going through 
passt_exit. 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