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

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

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