Re: [PATCH] pasta: Add fallback timer mechanism to check if namespace is gone

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

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

On Thu, Feb 15, 2024 at 11:39:11PM +0100, Stefano Brivio wrote:
> We don't know how frequently this happens, but hitting
> fs.inotify.max_user_watches or similar sysctl limits is definitely
> not out of question, and Paul mentioned that, for example, Podman's
> CI environments hit similar issues in the past.
> 
> Introduce a fallback mechanism based on a timer file descriptor: we
> grab the directory handle at startup, and we can then use openat(),
> triggered periodically, to check if the (network) namespace directory
> still exists. If openat() fails at some point, exit.
> 
> Link: https://github.com/containers/podman/pull/21563#issuecomment-1943505707
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  passt.c |  6 ++--
>  passt.h |  2 ++
>  pasta.c | 85 +++++++++++++++++++++++++++++++++++++++++++++------------
>  pasta.h |  2 +-
>  4 files changed, 73 insertions(+), 22 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index aaa8e58..13670b9 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -201,7 +201,7 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd = -1, quit_fd;
> +	int nfds, i, devnull_fd = -1, pidfile_fd = -1;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
>  	struct ctx c = { 0 };
> @@ -274,7 +274,7 @@ int main(int argc, char **argv)
>  	if (c.force_stderr || isatty(fileno(stdout)))
>  		__openlog(log_name, LOG_PERROR, LOG_DAEMON);
>  
> -	quit_fd = pasta_netns_quit_init(&c);
> +	pasta_netns_quit_init(&c);
>  
>  	tap_sock_init(&c);
>  
> @@ -371,7 +371,7 @@ loop:
>  			tap_listen_handler(&c, eventmask);
>  			break;
>  		case EPOLL_TYPE_NSQUIT:
> -			pasta_netns_quit_handler(&c, quit_fd);
> +			pasta_netns_quit_handler(&c, ref);

Hm.  As a rule, I've been trying to use a separate EPOLL_TYPE for each
different handler, rather than having secondary dispatch based on
other details, even if those different handlers are accomplishing
similar purposes (e.g. TAP_PASTA vs. TAP_PASST).

>  			break;
>  		case EPOLL_TYPE_TCP:
>  			tcp_sock_handler(&c, ref, eventmask);
> diff --git a/passt.h b/passt.h
> index a9e8f15..1c6bb13 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -84,6 +84,7 @@ enum epoll_type {
>   * @udp:	UDP-specific reference part
>   * @icmp:	ICMP-specific reference part
>   * @data:	Data handled by protocol handlers
> + * @nsdir_fd:	netns dirfd for fallback timer checking if namespace is gone
>   * @u64:	Opaque reference for epoll_ctl() and epoll_wait()
>   */
>  union epoll_ref {
> @@ -99,6 +100,7 @@ union epoll_ref {
>  			union udp_epoll_ref udp;
>  			union icmp_epoll_ref icmp;
>  			uint32_t data;
> +			int nsdir_fd;
>  		};
>  	};
>  	uint64_t u64;
> diff --git a/pasta.c b/pasta.c
> index 94807a3..60b6223 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -30,6 +30,7 @@
>  #include <sys/epoll.h>
>  #include <sys/inotify.h>
>  #include <sys/mount.h>
> +#include <sys/timerfd.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
> @@ -356,57 +357,105 @@ void pasta_ns_conf(struct ctx *c)
>  	proto_update_l2_buf(c->mac_guest, NULL);
>  }
>  
> +/**
> + * pasta_netns_quit_timer() - Set up fallback timer to monitor namespace
> + *
> + * Return: timerfd file descriptor, negative error code on failure
> + */
> +static int pasta_netns_quit_timer(void)
> +{
> +	int fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> +	struct itimerspec it = { { 1, 0 }, { 1, 0 } }; /* one-second interval */
> +
> +	if (fd == -1) {
> +		err("timerfd_create(): %s", strerror(errno));
> +		return -errno;
> +	}
> +
> +	if (timerfd_settime(fd, 0, &it, NULL) < 0) {
> +		err("timerfd_settime(): %s", strerror(errno));
> +		close(fd);
> +		return -errno;
> +	}
> +
> +	return fd;
> +}
> +
>  /**
>   * pasta_netns_quit_init() - Watch network namespace to quit once it's gone
>   * @c:		Execution context
>   *
> - * Return: inotify file descriptor, -1 on failure or if not needed/applicable
> + * Return: file descriptor (inotify or timerfd), -1 if not needed
>   */
>  int pasta_netns_quit_init(const struct ctx *c)
>  {
> +	union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT, .nsdir_fd = -1 };
> +	struct epoll_event ev = { .events = EPOLLIN };
>  	int flags = O_NONBLOCK | O_CLOEXEC;
> -	union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT };
> -	struct epoll_event ev = {
> -		.events = EPOLLIN
> -	};
> -	int inotify_fd;
> +	int fd = -1;
>  
>  	if (c->mode != MODE_PASTA || c->no_netns_quit || !*c->netns_base)
>  		return -1;
>  
> -	if ((inotify_fd = inotify_init1(flags)) < 0) {
> -		perror("inotify_init(): won't quit once netns is gone");
> -		return -1;
> +	if ((fd = inotify_init1(flags)) < 0)
> +		warn("inotify_init1(): %s, use a timer", strerror(errno));
> +
> +	if (fd >= 0 && inotify_add_watch(fd, c->netns_dir, IN_DELETE) < 0) {
> +		warn("inotify_add_watch(): %s, use a timer",
> +		     strerror(errno));
> +		close(fd);
> +		fd = -1;
>  	}
>  
> -	if (inotify_add_watch(inotify_fd, c->netns_dir, IN_DELETE) < 0) {
> -		perror("inotify_add_watch(): won't quit once netns is gone");
> -		return -1;
> +	if (fd < 0) {
> +		if ((fd = pasta_netns_quit_timer()) < 0)
> +			die("Failed to set up fallback netns timer, exiting");
> +
> +		ref.nsdir_fd = open(c->netns_dir, O_CLOEXEC | O_RDONLY);
> +		if (ref.nsdir_fd < 0)
> +			die("netns dir open: %s, exiting", strerror(errno));
>  	}
>  
> -	ref.fd = inotify_fd;
> +	if (fd > FD_REF_MAX)
> +		die("netns monitor file number %i too big, exiting", fd);
> +
> +	ref.fd = fd;
>  	ev.data.u64 = ref.u64;
> -	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, inotify_fd, &ev);
> +	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, fd, &ev);
>  
> -	return inotify_fd;
> +	return fd;
>  }
>  
>  /**
>   * pasta_netns_quit_handler() - Handle ns directory events, exit if ns is gone
>   * @c:		Execution context
> - * @inotify_fd:	inotify file descriptor with watch on namespace directory
> + * @ref:	epoll reference for inotify or timer descriptor
>   */
> -void pasta_netns_quit_handler(struct ctx *c, int inotify_fd)
> +void pasta_netns_quit_handler(struct ctx *c, union epoll_ref ref)
>  {
>  	char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
>  	const struct inotify_event *in_ev = (struct inotify_event *)buf;
>  
> -	if (read(inotify_fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev))
> +	if (ref.nsdir_fd != -1) {
> +		uint64_t expirations;
> +		int fd;
> +
> +		read(ref.fd, &expirations, sizeof(expirations));
> +
> +		if ((fd = openat(ref.nsdir_fd, c->netns_base, O_PATH)) < 0)
> +			goto gone;
> +
> +		close(fd);
> +		return;
> +	}
> +
> +	if (read(ref.fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev))
>  		return;
>  
>  	if (strncmp(in_ev->name, c->netns_base, sizeof(c->netns_base)))
>  		return;
>  
> +gone:
>  	info("Namespace %s is gone, exiting", c->netns_base);
>  	exit(EXIT_SUCCESS);
>  }
> diff --git a/pasta.h b/pasta.h
> index 5d20063..c120e94 100644
> --- a/pasta.h
> +++ b/pasta.h
> @@ -14,6 +14,6 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
>  void pasta_ns_conf(struct ctx *c);
>  void pasta_child_handler(int signal);
>  int pasta_netns_quit_init(const struct ctx *c);
> -void pasta_netns_quit_handler(struct ctx *c, int inotify_fd);
> +void pasta_netns_quit_handler(struct ctx *c, union epoll_ref ref);
>  
>  #endif /* PASTA_H */

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