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

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

[Stefano Brivio]

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);
 			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 */
-- 
@@ -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 */
-- 
2.39.2

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

[David Gibson]

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

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

[Stefano Brivio]

On Fri, 16 Feb 2024 14:00:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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).

Yes, I have to admit that the (ref.nsdir_fd != -1) if clause in the
handler is a bit of a hack, and a rather gratuitous one. Changed in v2
to use a new epoll reference type.

-- 
Stefano