Re: [PATCH] treewide: Flush pcap and log files, if used, before exiting

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

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

On Wed, Aug 13, 2025 at 06:45:05PM +0200, Stefano Brivio wrote:
> I didn't imagine that occasionally truncated pcap and log files, as a
> result of commit a9d63f91a59a ("passt-repair: use _exit() over
> return"), would be such a big deal, until I tried to debug TCP issues
> with this beauty:

I think the problems are more introduced by the previous patch
d0006fa784a7 ("treewide: use _exit() over exit()").

> 
>   while true; do ./pasta --trace -l /tmp/pasta.log -p /tmp/pasta.pcap --config-net -t 5555 -- socat TCP-LISTEN:5555 OPEN:/tmp/large.rcv,trunc & (sleep 0.3; socat -T2 OPEN:large.bin TCP:88.198.0.164:5555; ); wait; diff large.bin /tmp/large.rcv || break; done
> 
> ...flush files and pcap if we're using them. Ignore fsync() errors for
> the log file as we obviously can't reliably log them.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Hmmmmm.

I mean, yes, AFAICT this patch will address the immediate problem.
But between this and 081df67d1fb2 ("conf: flush stdout before early
exit") it seems more and more to me that _exit() just isn't what we
want.  Basically the assertion in d0006fa784a7 that "no exit handlers
are needed" doesn't really seem to be true.

Here we're adding a new syscall to work around the problems with
_exit().  In which case, why don't we add futex() to the syscall list
and go back to exit(3).  With Laurent working on multi-threading we
might well want futexes anyhow.

> diff --git a/README.md b/README.md
> index 54fed07..8f188f4 100644
> --- a/README.md
> +++ b/README.md
> @@ -291,7 +291,7 @@ speeding up local connections, and usually requiring NAT. _pasta_:
>  * ✅ all capabilities dropped, other than `CAP_NET_BIND_SERVICE` (if granted)
>  * ✅ with default options, user, mount, IPC, UTS, PID namespaces are detached
>  * ✅ no external dependencies (other than a standard C library)
> -* ✅ restrictive seccomp profiles (30 syscalls allowed for _passt_, 41 for
> +* ✅ restrictive seccomp profiles (33 syscalls allowed for _passt_, 43 for
>    _pasta_ on x86_64)
>  * ✅ examples of [AppArmor](/passt/tree/contrib/apparmor) and
>    [SELinux](/passt/tree/contrib/selinux) profiles available
> diff --git a/log.c b/log.c
> index 33b89fc..21e3673 100644
> --- a/log.c
> +++ b/log.c
> @@ -35,7 +35,7 @@ static int	log_sock = -1;		/* Optional socket to system logger */
>  static char	log_ident[BUFSIZ];	/* Identifier string for openlog() */
>  static int	log_mask;		/* Current log priority mask */
>  
> -static int	log_file = -1;		/* Optional log file descriptor */
> +int		log_file = -1;		/* Optional log file descriptor */
>  static size_t	log_size;		/* Maximum log file size in bytes */
>  static size_t	log_written;		/* Currently used bytes in log file */
>  static size_t	log_cut_size;		/* Bytes to cut at start on rotation */
> diff --git a/log.h b/log.h
> index 08aa88c..c8473c0 100644
> --- a/log.h
> +++ b/log.h
> @@ -41,6 +41,7 @@ void logmsg_perror(int pri, const char *format, ...)
>  		_exit(EXIT_FAILURE);					\
>  	} while (0)
>  
> +extern int log_file;
>  extern int log_trace;
>  extern bool log_conf_parsed;
>  extern bool log_stderr;
> diff --git a/passt.c b/passt.c
> index 388d10f..a4ec115 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -170,6 +170,7 @@ static void exit_handler(int signal)
>  {
>  	(void)signal;
>  
> +	fsync_pcap_and_log();
>  	_exit(EXIT_SUCCESS);
>  }
>  
> diff --git a/pasta.c b/pasta.c
> index c207692..687406b 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -70,6 +70,8 @@ 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);
>  
> diff --git a/pcap.c b/pcap.c
> index 46d11a2..e9d8d80 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -37,7 +37,7 @@
>  
>  #define PCAP_VERSION_MINOR 4
>  
> -static int pcap_fd = -1;
> +int pcap_fd = -1;
>  
>  struct pcap_pkthdr {
>  	uint32_t tv_sec;
> diff --git a/pcap.h b/pcap.h
> index 9795f2e..2aeb53e 100644
> --- a/pcap.h
> +++ b/pcap.h
> @@ -6,6 +6,8 @@
>  #ifndef PCAP_H
>  #define PCAP_H
>  
> +extern int pcap_fd;
> +
>  void pcap(const char *pkt, size_t l2len);
>  void pcap_multiple(const struct iovec *iov, size_t frame_parts, unsigned int n,
>  		   size_t offset);
> diff --git a/tap.c b/tap.c
> index 6db5d88..cddc164 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1117,8 +1117,10 @@ void tap_sock_reset(struct ctx *c)
>  {
>  	info("Client connection closed%s", c->one_off ? ", exiting" : "");
>  
> -	if (c->one_off)
> +	if (c->one_off) {
> +		fsync_pcap_and_log();
>  		_exit(EXIT_SUCCESS);
> +	}
>  
>  	/* Close the connected socket, wait for a new connection */
>  	epoll_del(c, c->fd_tap);
> diff --git a/util.c b/util.c
> index 3e93d47..c492f90 100644
> --- a/util.c
> +++ b/util.c
> @@ -34,6 +34,7 @@
>  #include "passt.h"
>  #include "packet.h"
>  #include "log.h"
> +#include "pcap.h"
>  #ifdef HAS_GETRANDOM
>  #include <sys/random.h>
>  #endif
> @@ -1045,3 +1046,17 @@ void abort_with_msg(const char *fmt, ...)
>  	 */
>  	abort();
>  }
> +
> +/**
> + * fsync_pcap_and_log() - Flush pcap and log files as needed
> + *
> + * #syscalls fsync
> + */
> +void fsync_pcap_and_log(void)
> +{
> +	if (pcap_fd != -1 && fsync(pcap_fd))
> +		warn_perror("Failed to flush pcap file, it might be truncated");
> +
> +	if (log_file != -1)
> +		(void)fsync(log_file);
> +}
> diff --git a/util.h b/util.h
> index b5b1b31..2a8c38f 100644
> --- a/util.h
> +++ b/util.h
> @@ -226,6 +226,7 @@ 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);
>  
>  /**
>   * af_name() - Return name of an address family

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