[PATCH trivial 0/2] split out os_close_all_open_fd and use it in net/tap.c too

[PATCH trivial 0/2] split out os_close_all_open_fd and use it in net/tap.c too

[Michael Tokarev]

We have at least two places in qemu where we're closing all possible file
descriptors, - in async-teardown.c and in net/tap.c.  While async-teardown
one uses either close_range() or readdir(/proc/self/fd), the two calls in
net/tap.c loops from 3 to RLIMIT_NOFILE, which might be quite slow, and
it actually *is* slow on some systems (eg, just qemu-system-x86_64 startup
with a tap device is very slow on alpine linux).

While for net/tap.c, maybe the better fix is to get rid of this closing
entirely and use O_CLOEXEC instead, this needs to be prepared at first,
while we alredy have almost ready-to-be-used implementation which only
needs to be moved into a common place.

Michael Tokarev (2):
  close_all_open_fd(): move to oslib-posix.c
  net/tap: use os_close_all_open_fd() instead of open-coding it

 include/sysemu/os-posix.h |  1 +
 net/tap.c                 | 15 ++-------------
 system/async-teardown.c   | 37 +------------------------------------
 util/oslib-posix.c        | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 49 deletions(-)

-- 
2.39.2

[PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Michael Tokarev]

Initially in async-teardown.c, but the same construct is used
elsewhere too.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 include/sysemu/os-posix.h |  1 +
 system/async-teardown.c   | 37 +------------------------------------
 util/oslib-posix.c        | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index dff32ae185..4c91d03f44 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -53,6 +53,7 @@ bool os_set_runas(const char *user_id);
 void os_set_chroot(const char *path);
 void os_setup_post(void);
 int os_mlock(void);
+void os_close_all_open_fd(int minfd);
 
 /**
  * qemu_alloc_stack:
diff --git a/system/async-teardown.c b/system/async-teardown.c
index 396963c091..41d3d94935 100644
--- a/system/async-teardown.c
+++ b/system/async-teardown.c
@@ -26,40 +26,6 @@
 
 static pid_t the_ppid;
 
-/*
- * Close all open file descriptors.
- */
-static void close_all_open_fd(void)
-{
-    struct dirent *de;
-    int fd, dfd;
-    DIR *dir;
-
-#ifdef CONFIG_CLOSE_RANGE
-    int r = close_range(0, ~0U, 0);
-    if (!r) {
-        /* Success, no need to try other ways. */
-        return;
-    }
-#endif
-
-    dir = opendir("/proc/self/fd");
-    if (!dir) {
-        /* If /proc is not mounted, there is nothing that can be done. */
-        return;
-    }
-    /* Avoid closing the directory. */
-    dfd = dirfd(dir);
-
-    for (de = readdir(dir); de; de = readdir(dir)) {
-        fd = atoi(de->d_name);
-        if (fd != dfd) {
-            close(fd);
-        }
-    }
-    closedir(dir);
-}
-
 static void hup_handler(int signal)
 {
     /* Check every second if this process has been reparented. */
@@ -85,9 +51,8 @@ static int async_teardown_fn(void *arg)
     /*
      * Close all file descriptors that might have been inherited from the
      * main qemu process when doing clone, needed to make libvirt happy.
-     * Not using close_range for increased compatibility with older kernels.
      */
-    close_all_open_fd();
+    os_close_all_open_fd(0);
 
     /* Set up a handler for SIGHUP and unblock SIGHUP. */
     sigaction(SIGHUP, &sa, NULL);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 7c297003b9..09d0ce4da6 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -27,6 +27,7 @@
  */
 
 #include "qemu/osdep.h"
+#include <dirent.h>
 #include <termios.h>
 
 #include <glib/gprintf.h>
@@ -106,6 +107,41 @@ int qemu_get_thread_id(void)
 #endif
 }
 
+/*
+ * Close all open file descriptors starting with minfd and up.
+ * Not using close_range for increased compatibility with older kernels.
+ */
+void os_close_all_open_fd(int minfd)
+{
+    struct dirent *de;
+    int fd, dfd;
+    DIR *dir;
+
+#ifdef CONFIG_CLOSE_RANGE
+    int r = close_range(minfd, ~0U, 0);
+    if (!r) {
+        /* Success, no need to try other ways. */
+        return;
+    }
+#endif
+
+    dir = opendir("/proc/self/fd");
+    if (!dir) {
+        /* If /proc is not mounted, there is nothing that can be done. */
+        return;
+    }
+    /* Avoid closing the directory. */
+    dfd = dirfd(dir);
+
+    for (de = readdir(dir); de; de = readdir(dir)) {
+        fd = atoi(de->d_name);
+        if (fd >= minfd && fd != dfd) {
+            close(fd);
+        }
+    }
+    closedir(dir);
+}
+
 int qemu_daemon(int nochdir, int noclose)
 {
     return daemon(nochdir, noclose);
-- 
2.39.2

Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Laurent Vivier]

Le 25/01/2024 à 23:29, Michael Tokarev a écrit :
> Initially in async-teardown.c, but the same construct is used
> elsewhere too.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   include/sysemu/os-posix.h |  1 +
>   system/async-teardown.c   | 37 +------------------------------------
>   util/oslib-posix.c        | 36 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 38 insertions(+), 36 deletions(-)
> 
> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> index dff32ae185..4c91d03f44 100644
> --- a/include/sysemu/os-posix.h
> +++ b/include/sysemu/os-posix.h
> @@ -53,6 +53,7 @@ bool os_set_runas(const char *user_id);
>   void os_set_chroot(const char *path);
>   void os_setup_post(void);
>   int os_mlock(void);
> +void os_close_all_open_fd(int minfd);
>   
>   /**
>    * qemu_alloc_stack:
> diff --git a/system/async-teardown.c b/system/async-teardown.c
> index 396963c091..41d3d94935 100644
> --- a/system/async-teardown.c
> +++ b/system/async-teardown.c
> @@ -26,40 +26,6 @@
>   
>   static pid_t the_ppid;
>   
> -/*
> - * Close all open file descriptors.
> - */
> -static void close_all_open_fd(void)
> -{
> -    struct dirent *de;
> -    int fd, dfd;
> -    DIR *dir;
> -
> -#ifdef CONFIG_CLOSE_RANGE
> -    int r = close_range(0, ~0U, 0);
> -    if (!r) {
> -        /* Success, no need to try other ways. */
> -        return;
> -    }
> -#endif
> -
> -    dir = opendir("/proc/self/fd");
> -    if (!dir) {
> -        /* If /proc is not mounted, there is nothing that can be done. */
> -        return;
> -    }
> -    /* Avoid closing the directory. */
> -    dfd = dirfd(dir);
> -
> -    for (de = readdir(dir); de; de = readdir(dir)) {
> -        fd = atoi(de->d_name);
> -        if (fd != dfd) {
> -            close(fd);
> -        }
> -    }
> -    closedir(dir);
> -}
> -
>   static void hup_handler(int signal)
>   {
>       /* Check every second if this process has been reparented. */
> @@ -85,9 +51,8 @@ static int async_teardown_fn(void *arg)
>       /*
>        * Close all file descriptors that might have been inherited from the
>        * main qemu process when doing clone, needed to make libvirt happy.
> -     * Not using close_range for increased compatibility with older kernels.
>        */
> -    close_all_open_fd();
> +    os_close_all_open_fd(0);
>   
>       /* Set up a handler for SIGHUP and unblock SIGHUP. */
>       sigaction(SIGHUP, &sa, NULL);
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 7c297003b9..09d0ce4da6 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -27,6 +27,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include <dirent.h>
>   #include <termios.h>
>   
>   #include <glib/gprintf.h>
> @@ -106,6 +107,41 @@ int qemu_get_thread_id(void)
>   #endif
>   }
>   
> +/*
> + * Close all open file descriptors starting with minfd and up.
> + * Not using close_range for increased compatibility with older kernels.
> + */
> +void os_close_all_open_fd(int minfd)
> +{
> +    struct dirent *de;
> +    int fd, dfd;
> +    DIR *dir;
> +
> +#ifdef CONFIG_CLOSE_RANGE
> +    int r = close_range(minfd, ~0U, 0);
> +    if (!r) {
> +        /* Success, no need to try other ways. */
> +        return;
> +    }
> +#endif
> +
> +    dir = opendir("/proc/self/fd");
> +    if (!dir) {
> +        /* If /proc is not mounted, there is nothing that can be done. */
> +        return;
> +    }
> +    /* Avoid closing the directory. */
> +    dfd = dirfd(dir);
> +
> +    for (de = readdir(dir); de; de = readdir(dir)) {
> +        fd = atoi(de->d_name);
> +        if (fd >= minfd && fd != dfd) {
> +            close(fd);
> +        }
> +    }
> +    closedir(dir);
> +}

I think the way using sysconf(_SC_OPEN_MAX) is more portable, simpler and cleaner than the one using 
/proc/self/fd.

Thanks,
Laurent
> +
>   int qemu_daemon(int nochdir, int noclose)
>   {
>       return daemon(nochdir, noclose);

Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Daniel P. Berrangé]

On Fri, Jan 26, 2024 at 08:44:13AM +0100, Laurent Vivier wrote:
> Le 25/01/2024 à 23:29, Michael Tokarev a écrit :
> > Initially in async-teardown.c, but the same construct is used
> > elsewhere too.
> > 
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > ---
> >   include/sysemu/os-posix.h |  1 +
> >   system/async-teardown.c   | 37 +------------------------------------
> >   util/oslib-posix.c        | 36 ++++++++++++++++++++++++++++++++++++
> >   3 files changed, 38 insertions(+), 36 deletions(-)
> > 
> > diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> > index dff32ae185..4c91d03f44 100644
> > --- a/include/sysemu/os-posix.h
> > +++ b/include/sysemu/os-posix.h
> > @@ -53,6 +53,7 @@ bool os_set_runas(const char *user_id);
> >   void os_set_chroot(const char *path);
> >   void os_setup_post(void);
> >   int os_mlock(void);
> > +void os_close_all_open_fd(int minfd);
> >   /**
> >    * qemu_alloc_stack:
> > diff --git a/system/async-teardown.c b/system/async-teardown.c
> > index 396963c091..41d3d94935 100644
> > --- a/system/async-teardown.c
> > +++ b/system/async-teardown.c
> > @@ -26,40 +26,6 @@
> >   static pid_t the_ppid;
> > -/*
> > - * Close all open file descriptors.
> > - */
> > -static void close_all_open_fd(void)
> > -{
> > -    struct dirent *de;
> > -    int fd, dfd;
> > -    DIR *dir;
> > -
> > -#ifdef CONFIG_CLOSE_RANGE
> > -    int r = close_range(0, ~0U, 0);
> > -    if (!r) {
> > -        /* Success, no need to try other ways. */
> > -        return;
> > -    }
> > -#endif
> > -
> > -    dir = opendir("/proc/self/fd");
> > -    if (!dir) {
> > -        /* If /proc is not mounted, there is nothing that can be done. */
> > -        return;
> > -    }
> > -    /* Avoid closing the directory. */
> > -    dfd = dirfd(dir);
> > -
> > -    for (de = readdir(dir); de; de = readdir(dir)) {
> > -        fd = atoi(de->d_name);
> > -        if (fd != dfd) {
> > -            close(fd);
> > -        }
> > -    }
> > -    closedir(dir);
> > -}
> > -
> >   static void hup_handler(int signal)
> >   {
> >       /* Check every second if this process has been reparented. */
> > @@ -85,9 +51,8 @@ static int async_teardown_fn(void *arg)
> >       /*
> >        * Close all file descriptors that might have been inherited from the
> >        * main qemu process when doing clone, needed to make libvirt happy.
> > -     * Not using close_range for increased compatibility with older kernels.
> >        */
> > -    close_all_open_fd();
> > +    os_close_all_open_fd(0);
> >       /* Set up a handler for SIGHUP and unblock SIGHUP. */
> >       sigaction(SIGHUP, &sa, NULL);
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 7c297003b9..09d0ce4da6 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -27,6 +27,7 @@
> >    */
> >   #include "qemu/osdep.h"
> > +#include <dirent.h>
> >   #include <termios.h>
> >   #include <glib/gprintf.h>
> > @@ -106,6 +107,41 @@ int qemu_get_thread_id(void)
> >   #endif
> >   }
> > +/*
> > + * Close all open file descriptors starting with minfd and up.
> > + * Not using close_range for increased compatibility with older kernels.
> > + */
> > +void os_close_all_open_fd(int minfd)
> > +{
> > +    struct dirent *de;
> > +    int fd, dfd;
> > +    DIR *dir;
> > +
> > +#ifdef CONFIG_CLOSE_RANGE
> > +    int r = close_range(minfd, ~0U, 0);
> > +    if (!r) {
> > +        /* Success, no need to try other ways. */
> > +        return;
> > +    }
> > +#endif
> > +
> > +    dir = opendir("/proc/self/fd");
> > +    if (!dir) {
> > +        /* If /proc is not mounted, there is nothing that can be done. */
> > +        return;
> > +    }
> > +    /* Avoid closing the directory. */
> > +    dfd = dirfd(dir);
> > +
> > +    for (de = readdir(dir); de; de = readdir(dir)) {
> > +        fd = atoi(de->d_name);
> > +        if (fd >= minfd && fd != dfd) {
> > +            close(fd);
> > +        }
> > +    }
> > +    closedir(dir);
> > +}
> 
> I think the way using sysconf(_SC_OPEN_MAX) is more portable, simpler and
> cleaner than the one using /proc/self/fd.

A fallback that uses _SC_OPEN_MAX is good for portability, but it is
should not be considered a replacement for iterating over /proc/self/fd,
rather an additional fallback for non-Linux, or when /proc is not mounted.
It is not uncommon for _SC_OPEN_MAX to be *exceedingly* high

  $ podman run -it quay.io/centos/centos:stream9
  [root@4a440d62935c /]# ulimit -n
  524288

Iterating over 1/2 a million FDs is a serious performance penalty that
we don't want to have, so _SC_OPEN_MAX should always be the last resort.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Michael Tokarev]

26.01.2024 12:06, Daniel P. Berrangé wrote:
> On Fri, Jan 26, 2024 at 08:44:13AM +0100, Laurent Vivier wrote:
>> Le 25/01/2024 à 23:29, Michael Tokarev a écrit :


>> I think the way using sysconf(_SC_OPEN_MAX) is more portable, simpler and
>> cleaner than the one using /proc/self/fd.
> 
> A fallback that uses _SC_OPEN_MAX is good for portability, but it is
> should not be considered a replacement for iterating over /proc/self/fd,
> rather an additional fallback for non-Linux, or when /proc is not mounted.
> It is not uncommon for _SC_OPEN_MAX to be *exceedingly* high
> 
>    $ podman run -it quay.io/centos/centos:stream9
>    [root@4a440d62935c /]# ulimit -n
>    524288
> 
> Iterating over 1/2 a million FDs is a serious performance penalty that
> we don't want to have, so _SC_OPEN_MAX should always be the last resort.

 From yesterday conversation in IRC which started this:

  <mmlb> open files          (-n) 1073741816

(it is a docker container)
They weren't able to start qemu.. :)

Sanity of such setting is questionable, but ok.

Not only linux implement close_range(2) syscall, it is also
available on some *BSDs.

And the most important point is, - we should aim at using O_CLOEXEC
everywhere, without this need to close each FD at exec time.  I think
qemu is the only software with such paranoid closing when just running
an interface setup script..

So yes, loop though all FDs is okay too as a last resort but..
For scripts in net/tap.c, this isn't necessary at all.  I want to take
a look at all open(2)/socket(2)/etc calls in qemu to ensure they're all
using O_CLOEXEC or are closed promptly, after which this code can be
removed entirely, hopefully.  Maybe this patch wont be needed after
that (so only async-teardown will need that code since it doesn't
do exec()).

Thanks,

/mjt

Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Daniel P. Berrangé]

On Fri, Jan 26, 2024 at 01:45:39PM +0300, Michael Tokarev wrote:
> 26.01.2024 12:06, Daniel P. Berrangé wrote:
> > On Fri, Jan 26, 2024 at 08:44:13AM +0100, Laurent Vivier wrote:
> > > Le 25/01/2024 à 23:29, Michael Tokarev a écrit :
> 
> 
> > > I think the way using sysconf(_SC_OPEN_MAX) is more portable, simpler and
> > > cleaner than the one using /proc/self/fd.
> > 
> > A fallback that uses _SC_OPEN_MAX is good for portability, but it is
> > should not be considered a replacement for iterating over /proc/self/fd,
> > rather an additional fallback for non-Linux, or when /proc is not mounted.
> > It is not uncommon for _SC_OPEN_MAX to be *exceedingly* high
> > 
> >    $ podman run -it quay.io/centos/centos:stream9
> >    [root@4a440d62935c /]# ulimit -n
> >    524288
> > 
> > Iterating over 1/2 a million FDs is a serious performance penalty that
> > we don't want to have, so _SC_OPEN_MAX should always be the last resort.
> 
> From yesterday conversation in IRC which started this:
> 
>  <mmlb> open files          (-n) 1073741816
> 
> (it is a docker container)
> They weren't able to start qemu.. :)
> 
> Sanity of such setting is questionable, but ok.
> 
> Not only linux implement close_range(2) syscall, it is also
> available on some *BSDs.
> 
> And the most important point is, - we should aim at using O_CLOEXEC
> everywhere, without this need to close each FD at exec time.  I think
> qemu is the only software with such paranoid closing when just running
> an interface setup script..

We should try to use O_CLOEXEC everywhere, but at the same time QEMU
links to a large number of libraries, and we can't assume that they've
reliably used O_CLOEXEC. Non-QEMU owned code that is mapped in process
likely dwarfs QEMU owned code by a factor of x10.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Michael Tokarev]

26.01.2024 14:01, Daniel P. Berrangé:
[]
> We should try to use O_CLOEXEC everywhere, but at the same time QEMU
> links to a large number of libraries, and we can't assume that they've
> reliably used O_CLOEXEC. Non-QEMU owned code that is mapped in process
> likely dwarfs QEMU owned code by a factor of x10.

There are quite a few points here.

As I already mentioned, qemu is one of very few software out here which
is this paranoid, - I know no other software which does this.  External
libs are being fixed too, and that's the proper place to do that.

Please note that currently we only close all files when executing scripts
to setup/teardown tap interfaces, but not, say, when spawning a process
to receive migration stream and in some other places, where such closing
might be much more important.

This close_all_open_fd() can check all FDs it finds open for O_CLOEXEC
as a debugging aid, - maybe we missed something in qemu already.  After
it's done, we'll have much better confidence already.

And something else I forgot to mention :)

/mjt

[PATCH trivial 2/2] net/tap: use os_close_all_open_fd() instead of open-coding it

[Michael Tokarev]

Current code loops over every file descriptor up to SC_OPEN_MAX/RLIMIT_NOFILE
which might be huge and the loop might be slow.  But we already have
os_close_all_open_fd() which is fast.  Use it.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 net/tap.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index c698b70475..11c85c50dc 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -459,13 +459,7 @@ static void launch_script(const char *setup_script, const char *ifname,
         return;
     }
     if (pid == 0) {
-        int open_max = sysconf(_SC_OPEN_MAX), i;
-
-        for (i = 3; i < open_max; i++) {
-            if (i != fd) {
-                close(i);
-            }
-        }
+        os_close_all_open_fd(3);
         parg = args;
         *parg++ = (char *)setup_script;
         *parg++ = (char *)ifname;
@@ -549,16 +543,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
         return -1;
     }
     if (pid == 0) {
-        int open_max = sysconf(_SC_OPEN_MAX), i;
         char *fd_buf = NULL;
         char *br_buf = NULL;
         char *helper_cmd = NULL;
 
-        for (i = 3; i < open_max; i++) {
-            if (i != sv[1]) {
-                close(i);
-            }
-        }
+        os_close_all_open_fd(3);
 
         fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
 
-- 
2.39.2