From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by passt.top (Postfix, from userid 1000) id E729B5A027B; Sat, 17 Feb 2024 15:18:59 +0100 (CET) From: Stefano Brivio To: passt-dev@passt.top Subject: [PATCH v2] pasta: Don't try to watch namespaces in procfs with inotify, use timer instead Date: Sat, 17 Feb 2024 15:18:59 +0100 Message-Id: <20240217141859.2358370-1-sbrivio@redhat.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: NWFOZS4SNW4ZP3MGNJHDKILQRQFCDEUU X-Message-ID-Hash: NWFOZS4SNW4ZP3MGNJHDKILQRQFCDEUU X-MailFrom: sbrivio@passt.top X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Paul Holzinger , David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 Buildah process 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 statfs(), 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 robust to detect the filesystem type using fstatfs() on the file descriptor (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. The timer, however, still uses the file descriptor of the parent directory later, as it has no access to the filesystem, and that avoids possible races if the PID is recycled: if the original process terminates, the handle we have on /proc/PID/ns still refers to it, not to any other process with the same PID. We could have used pidfd_open() to get a handle on the parent process. But it's not guaranteed that the parent process is actually the one associated to the network namespace we operate on, and if we get a PID file descriptor for a PID (parent or not) or path that was given on our command line, this inherently causes a race condition as that PID might have been recycled by the time we call pidfd_open(). Even assuming the process we want to watch is the parent process, and a race-free usage of pidfd_open() would have been possible, I'm not very enthusiastic about enabling yet another system call in the seccomp profile just for this, while openat() is needed anyway. 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 Link: https://github.com/containers/podman/pull/21563#issuecomment-1948200214 Signed-off-by: Stefano Brivio --- 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..de6e3bf 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. +If the target network namespace is bound to the filesystem, do not exit once +that path is deleted. + +If the target network namespace is represented by a procfs entry, do not exit +once that entry is removed from procfs (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..465fe1a 100644 --- a/pasta.c +++ b/pasta.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,7 @@ #include #include #include +#include #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 (statfs(c->netns_dir, &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