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

[PATCH v3] 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 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 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 naemsapce 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.

Instead of directly monitoring the target namespace, we could have
tried to monitor a process with a given PID, using pidfd_open() to
get a handle on it, to decide when to terminate.

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 <pholzing@redhat.com>
Link: https://github.com/containers/podman/pull/21563#issuecomment-1948200214
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
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..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..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

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

[David Gibson]

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

On Mon, Feb 19, 2024 at 09:05:33AM +0100, Stefano Brivio wrote:
> 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 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 naemsapce 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.
> 
> Instead of directly monitoring the target namespace, we could have
> tried to monitor a process with a given PID, using pidfd_open() to
> get a handle on it, to decide when to terminate.
> 
> 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 <pholzing@redhat.com>
> Link: https://github.com/containers/podman/pull/21563#issuecomment-1948200214
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> 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..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).

I realised part of the reason this seems so awkward to me is that
we're describing our normal behaviour w.r.t. netns lifetime, in the
context of an option that disables that.  So, maybe rephrase something like::

  --no-netns-quit
  Don't exit based on the state of the network namespace.

  Usually we exit if... <details of the logic>.


>  
> .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 };

I still don't like this initialisation, but I can live with it.  Also,
it's slightly shorter than the next line.

> +	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);
>  	}

-- 
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 v3] pasta: Don't try to watch namespaces in procfs with inotify, use timer instead

[Stefano Brivio]

On Mon, 19 Feb 2024 19:26:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 19, 2024 at 09:05:33AM +0100, Stefano Brivio wrote:
> > 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 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 naemsapce 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.
> > 
> > Instead of directly monitoring the target namespace, we could have
> > tried to monitor a process with a given PID, using pidfd_open() to
> > get a handle on it, to decide when to terminate.
> > 
> > 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 <pholzing@redhat.com>
> > Link: https://github.com/containers/podman/pull/21563#issuecomment-1948200214
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > 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..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).  
> 
> I realised part of the reason this seems so awkward to me is that
> we're describing our normal behaviour w.r.t. netns lifetime, in the
> context of an option that disables that.  So, maybe rephrase something like::
> 
>   --no-netns-quit
>   Don't exit based on the state of the network namespace.
> 
>   Usually we exit if... <details of the logic>.
> 
> 
> >  
> > .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 };  
> 
> I still don't like this initialisation, but I can live with it.  Also,
> it's slightly shorter than the next line.

Shorter than "bool try_inotify = true;"? They're both 24 characters...?

-- 
Stefano

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

[Paul Holzinger]

Hi Stefano, thanks for the patch.

On 19/02/2024 09:05, Stefano Brivio wrote:
> 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.
Note we do not want to track buildah, we pass down the pid of the 
container process so the tree looks like this:

buildah
\_ container process
\_ pasta

We pass down the pid of the container process to pasta and it needs to 
exit with it. One buildah can spawn multiple pasta processes at once. 
Each RUN instruction in a Dockerfile will setup a new container and thus 
a new netns.

>
> 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 naemsapce is given as a PID, or as a PID-based procfs
typo namespace
> 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.
>
> Instead of directly monitoring the target namespace, we could have
> tried to monitor a process with a given PID, using pidfd_open() to
> get a handle on it, to decide when to terminate.
>
> 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().
Well I mean the race is always there regardless of pidfd_open, already 
when you open /proc/$PID/ns/net the race exists as it may no longer be 
the correct pid if the process exited (and was reaped) before it can 
open the path. I think if we really want a race free interface then it 
would make the most sense to have the parent pass down a pidfd to pasta, 
this allows pasta to poll the fd to see the exit and also to call setns 
on that fd.
>
> 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.
If the parent opens the pidfd then this would not be an issue.
>
> 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>
> ---
> 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..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..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);
>   	}

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

[Stefano Brivio]

On Mon, 19 Feb 2024 13:40:59 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> Hi Stefano, thanks for the patch.
> 
> On 19/02/2024 09:05, Stefano Brivio wrote:
> > 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.  
> Note we do not want to track buildah, we pass down the pid of the 
> container process so the tree looks like this:
> 
> buildah
> \_ container process
> \_ pasta
> 
> We pass down the pid of the container process to pasta and it needs to 
> exit with it. One buildah can spawn multiple pasta processes at once. 
> Each RUN instruction in a Dockerfile will setup a new container and thus 
> a new netns.

Oh, I didn't know that, thanks for clarifying, I'll fix this in v4.

> > 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 naemsapce is given as a PID, or as a PID-based procfs  
> typo namespace

Fixed.

> > 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.
> >
> > Instead of directly monitoring the target namespace, we could have
> > tried to monitor a process with a given PID, using pidfd_open() to
> > get a handle on it, to decide when to terminate.
> >
> > 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().  
> Well I mean the race is always there regardless of pidfd_open, already 
> when you open /proc/$PID/ns/net the race exists as it may no longer be 
> the correct pid if the process exited (and was reaped) before it can 
> open the path.

For a PID referring to a third process, you're right, I'll change this
paragraph.

But if the PID is the one of the parent, we don't have this issue
because conf() is called before __daemon() in passt.c. If the parent
terminates before we get a reference to /proc/$PID/ns/net, pasta will
also terminate.

> I think if we really want a race free interface then it 
> would make the most sense to have the parent pass down a pidfd to pasta, 
> this allows pasta to poll the fd to see the exit and also to call setns 
> on that fd.

...and if we want a pidfd referring to another process that's not the
parent, right, that's the only way: the parent could first get a pidfd
referring to a child process, and then pass that to pasta. I guess we
should implement this at some point.

> >
> > 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.  
> If the parent opens the pidfd then this would not be an issue.

Right, we could then just add it to the epoll set -- I'll adjust this
paragraph as well.

Does the patch otherwise work for you? I guess we would need a new
release if we want to check this with Buildah, right?

-- 
Stefano

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

[Paul Holzinger]

On 19/02/2024 14:06, Stefano Brivio wrote:
> Does the patch otherwise work for you? I guess we would need a new
> release if we want to check this with Buildah, right?

Yes I tested it locally now, it works correctly with buildah and podman 
build.
And yes a timely release would be appreciated so we can get it into our 
CI and make pasta the default for Podman 5.0.

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

[Stefano Brivio]

On Mon, 19 Feb 2024 14:40:35 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 19/02/2024 14:06, Stefano Brivio wrote:
> > Does the patch otherwise work for you? I guess we would need a new
> > release if we want to check this with Buildah, right?  
> 
> Yes I tested it locally now, it works correctly with buildah and podman 
> build.

Great, thanks.

> And yes a timely release would be appreciated so we can get it into our 
> CI and make pasta the default for Podman 5.0.

Sure, release coming in a bit.

-- 
Stefano