[PATCH v4] pasta: Don't try to watch namespaces in procfs with inotify, use timer instead

[PATCH v4] pasta: Don't try to watch namespaces in procfs with inotify, use timer instead

[Stefano Brivio]

We watch network namespace entries to detect when we should quit
(unless --no-netns-quit is passed), and these might stored in a tmpfs
typically mounted at /run/user/UID or /var/run/user/UID, or found in
procfs at /proc/PID/ns/.

Currently, we try to use inotify for any possible location of those
entries, but inotify, of course, doesn't work on pseudo-filesystems
(see inotify(7)).

The man page reflects this: the description of --no-netns-quit
implies that we won't quit anyway if the namespace is not "bound to
the filesystem".

Well, we won't quit, but, since commit 9e0dbc894813 ("More
deterministic detection of whether argument is a PID, PATH or NAME"),
we try. And, indeed, this is harmless, as the caveat from that
commit message states.

Now, it turns out that Buildah, a tool to create container images,
sharing its codebase with Podman, passes a procfs entry to pasta, and
expects pasta to exit once the network namespace is not needed
anymore, that is, once the original container process, also spawned
by Buildah, terminates.

Get this to work by using the timer fallback mechanism if the
namespace name is passed as a path belonging to a pseudo-filesystem.
This is expected to be procfs, but I covered sysfs and devpts
pseudo-filesystems as well, because nothing actually prevents
creating this kind of directory structure and links there.

Note that fstatfs(), according to some versions of man pages, was
apparently "deprecated" by the LSB. My reasoning for using it is
essentially this:
  https://lore.kernel.org/linux-man/f54kudgblgk643u32tb6at4cd3kkzha6hslahv24szs4raroaz@ogivjbfdaqtb/t/#u

...that is, there was no such thing as an LSB deprecation, and
anyway there's no other way to get the filesystem type.

Also note that, while it might sound more obvious to detect the
filesystem type using fstatfs() on the file descriptor itself
(c->pasta_netns_fd), the reported filesystem type for it is nsfs, no
matter what path was given to pasta. If we use the parent directory,
we'll typically have either tmpfs or procfs reported.

If the target namespace is given as a PID, or as a PID-based procfs
entry, we don't risk races if this PID is recycled: our handle on
/proc/PID/ns will always refer to the original namespace associated
with that PID, and we don't re-open this entry from procfs to check
it.

There's, however, a remaining race possibility if the parent process
is not the one associated to the network namespace we operate on: in
that case, the parent might pass a procfs entry associated to a PID
that was recycled by the time we parse it. This can't happen if the
namespace PID matches the one of the parent, because we detach from
the controlling terminal after parsing the namespace reference.

To avoid this type of race, if desired, we could add the option for
the parent to pass a PID file descriptor, that the parent obtained
via pidfd_open(). This is beyond the scope of this change.

Update the man page to reflect that, even if the target network
namespace is passed as a procfs path or a PID, we'll now quit when
the procfs entry is gone.

Reported-by: Paul Holzinger <pholzing@redhat.com>
Link: https://github.com/containers/podman/pull/21563#issuecomment-1948200214
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
I'm pushing this in a bit as Podman's CI needs it soon, but posting
anyway to ease later reviews.

v4: Fix some further paragraphs in the commit message. Make man page
    change hopefully easier to follow.

v3: Given that we now open c->netns_dir before checking the
    filesystem type, we could as well pass this file descriptor to
    fstatfs() to do the check, instead of statfs() on the path.

    Fix a couple of paragraphs in the commit message.

v2: Coverity Scan isn't happy if we "check" (kind of) c->netns_dir
    with statfs() before opening it in a non-atomic way. Just to make
    things clear, false positive or not: open it, check it, close it
    if it wasn't needed: we don't rely on the check.

 passt.1 |  8 ++++++--
 pasta.c | 24 +++++++++++++++++++-----
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/passt.1 b/passt.1
index dc2d719..e97ef73 100644
--- a/passt.1
+++ b/passt.1
@@ -550,8 +550,12 @@ without \-\-userns.
 
 .TP
 .BR \-\-no-netns-quit
-If the target network namespace is bound to the filesystem (that is, if PATH or
-NAME are given as target), do not exit once the network namespace is deleted.
+Do not exit once the target namespace reference is removed.
+
+Without this option, \fBpasta\fR will terminate if the target network namespace
+is bound to the filesystem, and the given path is deleted, or if the target
+network namespace is represented by a procfs entry, and that entry is deleted,
+representing the fact that a process with the given PID terminated.
 
 .TP
 .BR \-\-config-net
diff --git a/pasta.c b/pasta.c
index 01d1511..61feaa9 100644
--- a/pasta.c
+++ b/pasta.c
@@ -33,6 +33,7 @@
 #include <sys/timerfd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/statfs.h>
 #include <fcntl.h>
 #include <sys/wait.h>
 #include <signal.h>
@@ -41,6 +42,7 @@
 #include <netinet/in.h>
 #include <net/ethernet.h>
 #include <sys/syscall.h>
+#include <linux/magic.h>
 
 #include "util.h"
 #include "passt.h"
@@ -390,12 +392,21 @@ void pasta_netns_quit_init(const struct ctx *c)
 	union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT_INOTIFY };
 	struct epoll_event ev = { .events = EPOLLIN };
 	int flags = O_NONBLOCK | O_CLOEXEC;
-	int fd;
+	struct statfs s = { 0 };
+	bool try_inotify = true;
+	int fd = -1, dir_fd;
 
 	if (c->mode != MODE_PASTA || c->no_netns_quit || !*c->netns_base)
 		return;
 
-	if ((fd = inotify_init1(flags)) < 0)
+	if ((dir_fd = open(c->netns_dir, O_CLOEXEC | O_RDONLY)) < 0)
+		die("netns dir open: %s, exiting", strerror(errno));
+
+	if (fstatfs(dir_fd, &s)          || s.f_type == DEVPTS_SUPER_MAGIC ||
+	    s.f_type == PROC_SUPER_MAGIC || s.f_type == SYSFS_MAGIC)
+		try_inotify = false;
+
+	if (try_inotify && (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) {
@@ -409,11 +420,11 @@ void pasta_netns_quit_init(const struct ctx *c)
 		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.nsdir_fd = dir_fd;
 
 		ref.type = EPOLL_TYPE_NSQUIT_TIMER;
+	} else {
+		close(dir_fd);
 	}
 
 	if (fd > FD_REF_MAX)
@@ -463,6 +474,9 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
 
 	fd = openat(ref.nsdir_fd, c->netns_base, O_PATH | O_CLOEXEC);
 	if (fd < 0) {
+		if (errno == EACCES)	/* Expected for existing procfs entry */
+			return;
+
 		info("Namespace %s is gone, exiting", c->netns_base);
 		exit(EXIT_SUCCESS);
 	}
-- 
@@ -33,6 +33,7 @@
 #include <sys/timerfd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/statfs.h>
 #include <fcntl.h>
 #include <sys/wait.h>
 #include <signal.h>
@@ -41,6 +42,7 @@
 #include <netinet/in.h>
 #include <net/ethernet.h>
 #include <sys/syscall.h>
+#include <linux/magic.h>
 
 #include "util.h"
 #include "passt.h"
@@ -390,12 +392,21 @@ void pasta_netns_quit_init(const struct ctx *c)
 	union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT_INOTIFY };
 	struct epoll_event ev = { .events = EPOLLIN };
 	int flags = O_NONBLOCK | O_CLOEXEC;
-	int fd;
+	struct statfs s = { 0 };
+	bool try_inotify = true;
+	int fd = -1, dir_fd;
 
 	if (c->mode != MODE_PASTA || c->no_netns_quit || !*c->netns_base)
 		return;
 
-	if ((fd = inotify_init1(flags)) < 0)
+	if ((dir_fd = open(c->netns_dir, O_CLOEXEC | O_RDONLY)) < 0)
+		die("netns dir open: %s, exiting", strerror(errno));
+
+	if (fstatfs(dir_fd, &s)          || s.f_type == DEVPTS_SUPER_MAGIC ||
+	    s.f_type == PROC_SUPER_MAGIC || s.f_type == SYSFS_MAGIC)
+		try_inotify = false;
+
+	if (try_inotify && (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) {
@@ -409,11 +420,11 @@ void pasta_netns_quit_init(const struct ctx *c)
 		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.nsdir_fd = dir_fd;
 
 		ref.type = EPOLL_TYPE_NSQUIT_TIMER;
+	} else {
+		close(dir_fd);
 	}
 
 	if (fd > FD_REF_MAX)
@@ -463,6 +474,9 @@ void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref)
 
 	fd = openat(ref.nsdir_fd, c->netns_base, O_PATH | O_CLOEXEC);
 	if (fd < 0) {
+		if (errno == EACCES)	/* Expected for existing procfs entry */
+			return;
+
 		info("Namespace %s is gone, exiting", c->netns_base);
 		exit(EXIT_SUCCESS);
 	}
-- 
2.39.2