[PATCH 0/2] Improved exit()/_exit() handling

[PATCH 0/2] Improved exit()/_exit() handling

[David Gibson]

As discussed on IRC, while working on the forwarding extensions, I
discovered that failures at certain points of initialisation (in
particular tcp_init() or udp_init() can result in a zombie pasta child
process.

Here's a fix.

David Gibson (2):
  treewide: Introduce passt_exit() helper
  pasta: Clean up waiting pasta child on failures

 conf.c       | 11 ++++-------
 log.h        |  9 +++++++--
 passt.c      |  6 +++---
 pasta.c      | 12 ++++++------
 pasta.h      |  1 +
 tap.c        |  6 ++----
 util.c       | 29 +++++++++++++++++++++++------
 util.h       |  1 -
 vhost_user.c |  4 ++--
 9 files changed, 48 insertions(+), 31 deletions(-)

-- 
2.52.0

[PATCH 1/2] treewide: Introduce passt_exit() helper

[David Gibson]

In d0006fa78 ("treewide: use _exit() over exit()"), we replaced use of
the normal exit(3) with direct calls to _exit(2).  That was because glibc
exit(3) made some unexpected futex() calls, which hit our seccomp profile.

We've since had some bugs due to missing the extra cleanup that exit(3)
implemented, for which we've added explicit cleanup calls.  Specifically,
we have fflush() calls in some places to avoid leaving incomplete messages
on stdout/stderr, and in other places fsync_pcap_and_log() to avoid
leaving incomplete log or pcap files.

It's easy to forget these when adding new error paths, so instead,
implement our own passt_exit() wrapper to perform vital cleanup then call
_exit(2).  This also provides an obvious place to add any additional
cleanups we discover we need in future.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c       | 11 ++++-------
 log.h        |  9 +++++++--
 passt.c      |  5 ++---
 pasta.c      | 10 ++++------
 tap.c        |  6 ++----
 util.c       | 24 ++++++++++++++++++------
 util.h       |  1 -
 vhost_user.c |  4 ++--
 8 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/conf.c b/conf.c
index fdc19e81..24b44419 100644
--- a/conf.c
+++ b/conf.c
@@ -826,7 +826,7 @@ static void conf_ip6_local(struct ip6_ctx *ip6)
  * usage() - Print usage, exit with given status code
  * @name:	Executable name
  * @f:		Stream to print usage info to
- * @status:	Status code for _exit()
+ * @status:	Status code for exit(2)
  */
 static void usage(const char *name, FILE *f, int status)
 {
@@ -997,8 +997,7 @@ static void usage(const char *name, FILE *f, int status)
 		"    SPEC is as described for TCP above\n"
 		"    default: none\n");
 
-	(void)fflush(f);
-	_exit(status);
+	passt_exit(status);
 
 pasta_opts:
 
@@ -1052,8 +1051,7 @@ pasta_opts:
 		"  --ns-mac-addr ADDR	Set MAC address on tap interface\n"
 		"  --no-splice		Disable inbound socket splicing\n");
 
-	(void)fflush(f);
-	_exit(status);
+	passt_exit(status);
 }
 
 /**
@@ -1621,8 +1619,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			FPRINTF(stdout,
 				c->mode == MODE_PASTA ? "pasta " : "passt ");
 			FPRINTF(stdout, VERSION_BLOB);
-			(void)fflush(stdout);
-			_exit(EXIT_SUCCESS);
+			passt_exit(EXIT_SUCCESS);
 		case 15:
 			ret = snprintf(c->ip4.ifname_out,
 				       sizeof(c->ip4.ifname_out), "%s", optarg);
diff --git a/log.h b/log.h
index c8473c0d..b7b20672 100644
--- a/log.h
+++ b/log.h
@@ -9,6 +9,11 @@
 #include <stdbool.h>
 #include <syslog.h>
 
+/* This would make more sense in util.h, but because we use it in die(), that
+ * would cause awkward circular reference problems.
+ */
+void passt_exit(int status) __attribute__((noreturn));
+
 #define LOGFILE_SIZE_DEFAULT		(1024 * 1024UL)
 #define LOGFILE_CUT_RATIO		30	/* When full, cut ~30% size */
 #define LOGFILE_SIZE_MIN		(5UL * MAX(BUFSIZ, PAGE_SIZE))
@@ -32,13 +37,13 @@ void logmsg_perror(int pri, const char *format, ...)
 #define die(...)							\
 	do {								\
 		err(__VA_ARGS__);					\
-		_exit(EXIT_FAILURE);					\
+		passt_exit(EXIT_FAILURE);				\
 	} while (0)
 
 #define die_perror(...)							\
 	do {								\
 		err_perror(__VA_ARGS__);				\
-		_exit(EXIT_FAILURE);					\
+		passt_exit(EXIT_FAILURE);				\
 	} while (0)
 
 extern int log_file;
diff --git a/passt.c b/passt.c
index 5ed88d07..cf38822f 100644
--- a/passt.c
+++ b/passt.c
@@ -177,8 +177,7 @@ static void exit_handler(int signal)
 {
 	(void)signal;
 
-	fsync_pcap_and_log();
-	_exit(EXIT_SUCCESS);
+	passt_exit(EXIT_SUCCESS);
 }
 
 /**
@@ -399,7 +398,7 @@ int main(int argc, char **argv)
 	flow_init();
 
 	if ((!c.no_udp && udp_init(&c)) || (!c.no_tcp && tcp_init(&c)))
-		_exit(EXIT_FAILURE);
+		passt_exit(EXIT_FAILURE);
 
 	proto_update_l2_buf(c.guest_mac);
 
diff --git a/pasta.c b/pasta.c
index 674b5542..5c693de1 100644
--- a/pasta.c
+++ b/pasta.c
@@ -70,15 +70,13 @@ void pasta_child_handler(int signal)
 	if (pasta_child_pid &&
 	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
 		if (infop.si_pid == pasta_child_pid) {
-			fsync_pcap_and_log();
-
 			if (infop.si_code == CLD_EXITED)
-				_exit(infop.si_status);
+				passt_exit(infop.si_status);
 
 			/* If killed by a signal, si_status is the number.
 			 * Follow common shell convention of returning it + 128.
 			 */
-			_exit(infop.si_status + 128);
+			passt_exit(infop.si_status + 128);
 
 			/* Nothing to do, detached PID namespace going away */
 		}
@@ -511,7 +509,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd)
 
 		if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) {
 			info("Namespace %s is gone, exiting", c->netns_base);
-			_exit(EXIT_SUCCESS);
+			passt_exit(EXIT_SUCCESS);
 		}
 	}
 }
@@ -539,7 +537,7 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
 			return;
 
 		info("Namespace %s is gone, exiting", c->netns_base);
-		_exit(EXIT_SUCCESS);
+		passt_exit(EXIT_SUCCESS);
 	}
 
 	close(fd);
diff --git a/tap.c b/tap.c
index e3ea61cb..9d1344b2 100644
--- a/tap.c
+++ b/tap.c
@@ -1149,10 +1149,8 @@ void tap_sock_reset(struct ctx *c)
 {
 	info("Client connection closed%s", c->one_off ? ", exiting" : "");
 
-	if (c->one_off) {
-		fsync_pcap_and_log();
-		_exit(EXIT_SUCCESS);
-	}
+	if (c->one_off)
+		passt_exit(EXIT_SUCCESS);
 
 	/* Close the connected socket, wait for a new connection */
 	epoll_del(c->epollfd, c->fd_tap);
diff --git a/util.c b/util.c
index bfeb6191..e266c396 100644
--- a/util.c
+++ b/util.c
@@ -555,7 +555,7 @@ void pidfile_write(int fd, pid_t pid)
 
 	if (write(fd, pid_buf, n) < 0) {
 		perror("PID file write");
-		_exit(EXIT_FAILURE);
+		passt_exit(EXIT_FAILURE);
 	}
 
 	close(fd);
@@ -592,12 +592,12 @@ int __daemon(int pidfile_fd, int devnull_fd)
 
 	if (pid == -1) {
 		perror("fork");
-		_exit(EXIT_FAILURE);
+		passt_exit(EXIT_FAILURE);
 	}
 
 	if (pid) {
 		pidfile_write(pidfile_fd, pid);
-		_exit(EXIT_SUCCESS);
+		passt_exit(EXIT_SUCCESS);
 	}
 
 	if (setsid()				< 0 ||
@@ -605,7 +605,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
 	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
 	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
 	    close(devnull_fd))
-		_exit(EXIT_FAILURE);
+		passt_exit(EXIT_FAILURE);
 
 	return 0;
 }
@@ -1225,17 +1225,29 @@ void abort_with_msg(const char *fmt, ...)
 }
 
 /**
- * fsync_pcap_and_log() - Flush pcap and log files as needed
+ * passt_exit() - Perform vital cleanup and exit
+ *
+ * We don't use exit(3) because on some C library versions it can do unexpected
+ * things that hit our seccomp profile (e.g. futex() calls).  This is a bespoke
+ * wrapper around _exit(2) performing just the cleanup that we need.
  *
  * #syscalls fsync
  */
-void fsync_pcap_and_log(void)
+void passt_exit(int status)
 {
+	/* 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");
 
+	/* Make sure we don't leave an incomplete log */
 	if (log_file != -1)
 		(void)fsync(log_file);
+
+	/* Make sure we dont leave any messages incomplete */
+	(void)fflush(stderr);
+	(void)fflush(stdout);
+	
+	_exit(status);
 }
 
 /**
diff --git a/util.h b/util.h
index f7a941fd..4cbb5da3 100644
--- a/util.h
+++ b/util.h
@@ -242,7 +242,6 @@ int read_all_buf(int fd, void *buf, size_t len);
 int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
 void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
-void fsync_pcap_and_log(void);
 long clamped_scale(long x, long y, long lo, long hi, long f);
 
 /**
diff --git a/vhost_user.c b/vhost_user.c
index aa7c869d..f9a5646c 100644
--- a/vhost_user.c
+++ b/vhost_user.c
@@ -60,7 +60,7 @@ void vu_print_capabilities(void)
 	info("{");
 	info("  \"type\": \"net\"");
 	info("}");
-	_exit(EXIT_SUCCESS);
+	passt_exit(EXIT_SUCCESS);
 }
 
 /**
@@ -1202,7 +1202,7 @@ void vu_control_handler(struct vu_dev *vdev, int fd, uint32_t events)
 	    !vdev->context->migrate_target) {
 		if (vdev->context->migrate_exit) {
 			info("Migration complete, exiting");
-			_exit(EXIT_SUCCESS);
+			passt_exit(EXIT_SUCCESS);
 		}
 
 		info("Migration complete");
-- 
2.52.0

Re: [PATCH 1/2] treewide: Introduce passt_exit() helper

[Stefano Brivio]

On Wed, 10 Dec 2025 16:15:51 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In d0006fa78 ("treewide: use _exit() over exit()"), we replaced use of
> the normal exit(3) with direct calls to _exit(2).  That was because glibc
> exit(3) made some unexpected futex() calls, which hit our seccomp profile.
> 
> We've since had some bugs due to missing the extra cleanup that exit(3)
> implemented, for which we've added explicit cleanup calls.  Specifically,
> we have fflush() calls in some places to avoid leaving incomplete messages
> on stdout/stderr, and in other places fsync_pcap_and_log() to avoid
> leaving incomplete log or pcap files.
> 
> It's easy to forget these when adding new error paths, so instead,
> implement our own passt_exit() wrapper to perform vital cleanup then call
> _exit(2).  This also provides an obvious place to add any additional
> cleanups we discover we need in future.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c       | 11 ++++-------
>  log.h        |  9 +++++++--
>  passt.c      |  5 ++---
>  pasta.c      | 10 ++++------
>  tap.c        |  6 ++----
>  util.c       | 24 ++++++++++++++++++------
>  util.h       |  1 -
>  vhost_user.c |  4 ++--
>  8 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index fdc19e81..24b44419 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -826,7 +826,7 @@ static void conf_ip6_local(struct ip6_ctx *ip6)
>   * usage() - Print usage, exit with given status code
>   * @name:	Executable name
>   * @f:		Stream to print usage info to
> - * @status:	Status code for _exit()
> + * @status:	Status code for exit(2)
>   */
>  static void usage(const char *name, FILE *f, int status)
>  {
> @@ -997,8 +997,7 @@ static void usage(const char *name, FILE *f, int status)
>  		"    SPEC is as described for TCP above\n"
>  		"    default: none\n");
>  
> -	(void)fflush(f);
> -	_exit(status);
> +	passt_exit(status);
>  
>  pasta_opts:
>  
> @@ -1052,8 +1051,7 @@ pasta_opts:
>  		"  --ns-mac-addr ADDR	Set MAC address on tap interface\n"
>  		"  --no-splice		Disable inbound socket splicing\n");
>  
> -	(void)fflush(f);
> -	_exit(status);
> +	passt_exit(status);
>  }
>  
>  /**
> @@ -1621,8 +1619,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			FPRINTF(stdout,
>  				c->mode == MODE_PASTA ? "pasta " : "passt ");
>  			FPRINTF(stdout, VERSION_BLOB);
> -			(void)fflush(stdout);
> -			_exit(EXIT_SUCCESS);
> +			passt_exit(EXIT_SUCCESS);
>  		case 15:
>  			ret = snprintf(c->ip4.ifname_out,
>  				       sizeof(c->ip4.ifname_out), "%s", optarg);
> diff --git a/log.h b/log.h
> index c8473c0d..b7b20672 100644
> --- a/log.h
> +++ b/log.h
> @@ -9,6 +9,11 @@
>  #include <stdbool.h>
>  #include <syslog.h>
>  
> +/* This would make more sense in util.h, but because we use it in die(), that
> + * would cause awkward circular reference problems.
> + */
> +void passt_exit(int status) __attribute__((noreturn));
> +
>  #define LOGFILE_SIZE_DEFAULT		(1024 * 1024UL)
>  #define LOGFILE_CUT_RATIO		30	/* When full, cut ~30% size */
>  #define LOGFILE_SIZE_MIN		(5UL * MAX(BUFSIZ, PAGE_SIZE))
> @@ -32,13 +37,13 @@ void logmsg_perror(int pri, const char *format, ...)
>  #define die(...)							\
>  	do {								\
>  		err(__VA_ARGS__);					\
> -		_exit(EXIT_FAILURE);					\
> +		passt_exit(EXIT_FAILURE);				\
>  	} while (0)
>  
>  #define die_perror(...)							\
>  	do {								\
>  		err_perror(__VA_ARGS__);				\
> -		_exit(EXIT_FAILURE);					\
> +		passt_exit(EXIT_FAILURE);				\
>  	} while (0)
>  
>  extern int log_file;
> diff --git a/passt.c b/passt.c
> index 5ed88d07..cf38822f 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -177,8 +177,7 @@ static void exit_handler(int signal)
>  {
>  	(void)signal;
>  
> -	fsync_pcap_and_log();
> -	_exit(EXIT_SUCCESS);
> +	passt_exit(EXIT_SUCCESS);
>  }
>  
>  /**
> @@ -399,7 +398,7 @@ int main(int argc, char **argv)
>  	flow_init();
>  
>  	if ((!c.no_udp && udp_init(&c)) || (!c.no_tcp && tcp_init(&c)))
> -		_exit(EXIT_FAILURE);
> +		passt_exit(EXIT_FAILURE);
>  
>  	proto_update_l2_buf(c.guest_mac);
>  
> diff --git a/pasta.c b/pasta.c
> index 674b5542..5c693de1 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -70,15 +70,13 @@ void pasta_child_handler(int signal)
>  	if (pasta_child_pid &&
>  	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
>  		if (infop.si_pid == pasta_child_pid) {
> -			fsync_pcap_and_log();
> -
>  			if (infop.si_code == CLD_EXITED)
> -				_exit(infop.si_status);
> +				passt_exit(infop.si_status);
>  
>  			/* If killed by a signal, si_status is the number.
>  			 * Follow common shell convention of returning it + 128.
>  			 */
> -			_exit(infop.si_status + 128);
> +			passt_exit(infop.si_status + 128);
>  
>  			/* Nothing to do, detached PID namespace going away */
>  		}
> @@ -511,7 +509,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd)
>  
>  		if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) {
>  			info("Namespace %s is gone, exiting", c->netns_base);
> -			_exit(EXIT_SUCCESS);
> +			passt_exit(EXIT_SUCCESS);
>  		}
>  	}
>  }
> @@ -539,7 +537,7 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
>  			return;
>  
>  		info("Namespace %s is gone, exiting", c->netns_base);
> -		_exit(EXIT_SUCCESS);
> +		passt_exit(EXIT_SUCCESS);
>  	}
>  
>  	close(fd);
> diff --git a/tap.c b/tap.c
> index e3ea61cb..9d1344b2 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1149,10 +1149,8 @@ void tap_sock_reset(struct ctx *c)
>  {
>  	info("Client connection closed%s", c->one_off ? ", exiting" : "");
>  
> -	if (c->one_off) {
> -		fsync_pcap_and_log();
> -		_exit(EXIT_SUCCESS);
> -	}
> +	if (c->one_off)
> +		passt_exit(EXIT_SUCCESS);
>  
>  	/* Close the connected socket, wait for a new connection */
>  	epoll_del(c->epollfd, c->fd_tap);
> diff --git a/util.c b/util.c
> index bfeb6191..e266c396 100644
> --- a/util.c
> +++ b/util.c
> @@ -555,7 +555,7 @@ void pidfile_write(int fd, pid_t pid)
>  
>  	if (write(fd, pid_buf, n) < 0) {
>  		perror("PID file write");
> -		_exit(EXIT_FAILURE);
> +		passt_exit(EXIT_FAILURE);
>  	}
>  
>  	close(fd);
> @@ -592,12 +592,12 @@ int __daemon(int pidfile_fd, int devnull_fd)
>  
>  	if (pid == -1) {
>  		perror("fork");
> -		_exit(EXIT_FAILURE);
> +		passt_exit(EXIT_FAILURE);
>  	}
>  
>  	if (pid) {
>  		pidfile_write(pidfile_fd, pid);
> -		_exit(EXIT_SUCCESS);
> +		passt_exit(EXIT_SUCCESS);
>  	}
>  
>  	if (setsid()				< 0 ||
> @@ -605,7 +605,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
>  	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
>  	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
>  	    close(devnull_fd))
> -		_exit(EXIT_FAILURE);
> +		passt_exit(EXIT_FAILURE);
>  
>  	return 0;
>  }
> @@ -1225,17 +1225,29 @@ void abort_with_msg(const char *fmt, ...)
>  }
>  
>  /**
> - * fsync_pcap_and_log() - Flush pcap and log files as needed
> + * passt_exit() - Perform vital cleanup and exit
> + *
> + * We don't use exit(3) because on some C library versions it can do unexpected
> + * things that hit our seccomp profile (e.g. futex() calls).  This is a bespoke
> + * wrapper around _exit(2) performing just the cleanup that we need.
>   *
>   * #syscalls fsync
>   */
> -void fsync_pcap_and_log(void)
> +void passt_exit(int status)
>  {
> +	/* 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");
>  
> +	/* Make sure we don't leave an incomplete log */
>  	if (log_file != -1)
>  		(void)fsync(log_file);
> +
> +	/* Make sure we dont leave any messages incomplete */

Nit just if you respin: "don't".

-- 
Stefano

Re: [PATCH 1/2] treewide: Introduce passt_exit() helper

[David Gibson]

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

On Wed, Dec 10, 2025 at 05:18:41PM +0100, Stefano Brivio wrote:
> On Wed, 10 Dec 2025 16:15:51 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In d0006fa78 ("treewide: use _exit() over exit()"), we replaced use of
> > the normal exit(3) with direct calls to _exit(2).  That was because glibc
> > exit(3) made some unexpected futex() calls, which hit our seccomp profile.
> > 
> > We've since had some bugs due to missing the extra cleanup that exit(3)
> > implemented, for which we've added explicit cleanup calls.  Specifically,
> > we have fflush() calls in some places to avoid leaving incomplete messages
> > on stdout/stderr, and in other places fsync_pcap_and_log() to avoid
> > leaving incomplete log or pcap files.
> > 
> > It's easy to forget these when adding new error paths, so instead,
> > implement our own passt_exit() wrapper to perform vital cleanup then call
> > _exit(2).  This also provides an obvious place to add any additional
> > cleanups we discover we need in future.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c       | 11 ++++-------
> >  log.h        |  9 +++++++--
> >  passt.c      |  5 ++---
> >  pasta.c      | 10 ++++------
> >  tap.c        |  6 ++----
> >  util.c       | 24 ++++++++++++++++++------
> >  util.h       |  1 -
> >  vhost_user.c |  4 ++--
> >  8 files changed, 39 insertions(+), 31 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index fdc19e81..24b44419 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -826,7 +826,7 @@ static void conf_ip6_local(struct ip6_ctx *ip6)
> >   * usage() - Print usage, exit with given status code
> >   * @name:	Executable name
> >   * @f:		Stream to print usage info to
> > - * @status:	Status code for _exit()
> > + * @status:	Status code for exit(2)
> >   */
> >  static void usage(const char *name, FILE *f, int status)
> >  {
> > @@ -997,8 +997,7 @@ static void usage(const char *name, FILE *f, int status)
> >  		"    SPEC is as described for TCP above\n"
> >  		"    default: none\n");
> >  
> > -	(void)fflush(f);
> > -	_exit(status);
> > +	passt_exit(status);
> >  
> >  pasta_opts:
> >  
> > @@ -1052,8 +1051,7 @@ pasta_opts:
> >  		"  --ns-mac-addr ADDR	Set MAC address on tap interface\n"
> >  		"  --no-splice		Disable inbound socket splicing\n");
> >  
> > -	(void)fflush(f);
> > -	_exit(status);
> > +	passt_exit(status);
> >  }
> >  
> >  /**
> > @@ -1621,8 +1619,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			FPRINTF(stdout,
> >  				c->mode == MODE_PASTA ? "pasta " : "passt ");
> >  			FPRINTF(stdout, VERSION_BLOB);
> > -			(void)fflush(stdout);
> > -			_exit(EXIT_SUCCESS);
> > +			passt_exit(EXIT_SUCCESS);
> >  		case 15:
> >  			ret = snprintf(c->ip4.ifname_out,
> >  				       sizeof(c->ip4.ifname_out), "%s", optarg);
> > diff --git a/log.h b/log.h
> > index c8473c0d..b7b20672 100644
> > --- a/log.h
> > +++ b/log.h
> > @@ -9,6 +9,11 @@
> >  #include <stdbool.h>
> >  #include <syslog.h>
> >  
> > +/* This would make more sense in util.h, but because we use it in die(), that
> > + * would cause awkward circular reference problems.
> > + */
> > +void passt_exit(int status) __attribute__((noreturn));
> > +
> >  #define LOGFILE_SIZE_DEFAULT		(1024 * 1024UL)
> >  #define LOGFILE_CUT_RATIO		30	/* When full, cut ~30% size */
> >  #define LOGFILE_SIZE_MIN		(5UL * MAX(BUFSIZ, PAGE_SIZE))
> > @@ -32,13 +37,13 @@ void logmsg_perror(int pri, const char *format, ...)
> >  #define die(...)							\
> >  	do {								\
> >  		err(__VA_ARGS__);					\
> > -		_exit(EXIT_FAILURE);					\
> > +		passt_exit(EXIT_FAILURE);				\
> >  	} while (0)
> >  
> >  #define die_perror(...)							\
> >  	do {								\
> >  		err_perror(__VA_ARGS__);				\
> > -		_exit(EXIT_FAILURE);					\
> > +		passt_exit(EXIT_FAILURE);				\
> >  	} while (0)
> >  
> >  extern int log_file;
> > diff --git a/passt.c b/passt.c
> > index 5ed88d07..cf38822f 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -177,8 +177,7 @@ static void exit_handler(int signal)
> >  {
> >  	(void)signal;
> >  
> > -	fsync_pcap_and_log();
> > -	_exit(EXIT_SUCCESS);
> > +	passt_exit(EXIT_SUCCESS);
> >  }
> >  
> >  /**
> > @@ -399,7 +398,7 @@ int main(int argc, char **argv)
> >  	flow_init();
> >  
> >  	if ((!c.no_udp && udp_init(&c)) || (!c.no_tcp && tcp_init(&c)))
> > -		_exit(EXIT_FAILURE);
> > +		passt_exit(EXIT_FAILURE);
> >  
> >  	proto_update_l2_buf(c.guest_mac);
> >  
> > diff --git a/pasta.c b/pasta.c
> > index 674b5542..5c693de1 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -70,15 +70,13 @@ void pasta_child_handler(int signal)
> >  	if (pasta_child_pid &&
> >  	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
> >  		if (infop.si_pid == pasta_child_pid) {
> > -			fsync_pcap_and_log();
> > -
> >  			if (infop.si_code == CLD_EXITED)
> > -				_exit(infop.si_status);
> > +				passt_exit(infop.si_status);
> >  
> >  			/* If killed by a signal, si_status is the number.
> >  			 * Follow common shell convention of returning it + 128.
> >  			 */
> > -			_exit(infop.si_status + 128);
> > +			passt_exit(infop.si_status + 128);
> >  
> >  			/* Nothing to do, detached PID namespace going away */
> >  		}
> > @@ -511,7 +509,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd)
> >  
> >  		if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) {
> >  			info("Namespace %s is gone, exiting", c->netns_base);
> > -			_exit(EXIT_SUCCESS);
> > +			passt_exit(EXIT_SUCCESS);
> >  		}
> >  	}
> >  }
> > @@ -539,7 +537,7 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
> >  			return;
> >  
> >  		info("Namespace %s is gone, exiting", c->netns_base);
> > -		_exit(EXIT_SUCCESS);
> > +		passt_exit(EXIT_SUCCESS);
> >  	}
> >  
> >  	close(fd);
> > diff --git a/tap.c b/tap.c
> > index e3ea61cb..9d1344b2 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1149,10 +1149,8 @@ void tap_sock_reset(struct ctx *c)
> >  {
> >  	info("Client connection closed%s", c->one_off ? ", exiting" : "");
> >  
> > -	if (c->one_off) {
> > -		fsync_pcap_and_log();
> > -		_exit(EXIT_SUCCESS);
> > -	}
> > +	if (c->one_off)
> > +		passt_exit(EXIT_SUCCESS);
> >  
> >  	/* Close the connected socket, wait for a new connection */
> >  	epoll_del(c->epollfd, c->fd_tap);
> > diff --git a/util.c b/util.c
> > index bfeb6191..e266c396 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -555,7 +555,7 @@ void pidfile_write(int fd, pid_t pid)
> >  
> >  	if (write(fd, pid_buf, n) < 0) {
> >  		perror("PID file write");
> > -		_exit(EXIT_FAILURE);
> > +		passt_exit(EXIT_FAILURE);
> >  	}
> >  
> >  	close(fd);
> > @@ -592,12 +592,12 @@ int __daemon(int pidfile_fd, int devnull_fd)
> >  
> >  	if (pid == -1) {
> >  		perror("fork");
> > -		_exit(EXIT_FAILURE);
> > +		passt_exit(EXIT_FAILURE);
> >  	}
> >  
> >  	if (pid) {
> >  		pidfile_write(pidfile_fd, pid);
> > -		_exit(EXIT_SUCCESS);
> > +		passt_exit(EXIT_SUCCESS);
> >  	}
> >  
> >  	if (setsid()				< 0 ||
> > @@ -605,7 +605,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
> >  	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
> >  	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
> >  	    close(devnull_fd))
> > -		_exit(EXIT_FAILURE);
> > +		passt_exit(EXIT_FAILURE);
> >  
> >  	return 0;
> >  }
> > @@ -1225,17 +1225,29 @@ void abort_with_msg(const char *fmt, ...)
> >  }
> >  
> >  /**
> > - * fsync_pcap_and_log() - Flush pcap and log files as needed
> > + * passt_exit() - Perform vital cleanup and exit
> > + *
> > + * We don't use exit(3) because on some C library versions it can do unexpected
> > + * things that hit our seccomp profile (e.g. futex() calls).  This is a bespoke
> > + * wrapper around _exit(2) performing just the cleanup that we need.
> >   *
> >   * #syscalls fsync
> >   */
> > -void fsync_pcap_and_log(void)
> > +void passt_exit(int status)
> >  {
> > +	/* 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");
> >  
> > +	/* Make sure we don't leave an incomplete log */
> >  	if (log_file != -1)
> >  		(void)fsync(log_file);
> > +
> > +	/* Make sure we dont leave any messages incomplete */
> 
> Nit just if you respin: "don't".

Oops, that's mildly embarrassing.  I will respin based on your
comments for 2/2.

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

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

[David Gibson]

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);
+
 	/* 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");
-- 
2.52.0

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

[Paul Holzinger]

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

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

[David Gibson]

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

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

[Paul Holzinger]

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.

>
>> 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
>>
-- 
Paul Holzinger

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

[David Gibson]

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