From: sirn <sirn@users.noreply.github.com>
To: ml@inbox.vuxu.org
Subject: Re: [PR PATCH] [Updated] libvirt: fix deadlock for musl
Date: Sun, 08 Nov 2020 11:27:53 +0100 [thread overview]
Message-ID: <20201108102753.wILJC4CnaMabYtEBTbC_nCtv3S29dFAtlMA8lm7mXH4@z> (raw)
In-Reply-To: <gh-mailinglist-notifications-41a7ca26-5023-4802-975b-f1789d68868e-void-packages-25905@inbox.vuxu.org>
[-- Attachment #1: Type: text/plain, Size: 886 bytes --]
There is an updated pull request by sirn against master on the void-packages repository
https://github.com/sirn/void-packages libvirt-musl-fix
https://github.com/void-linux/void-packages/pull/25905
libvirt: fix deadlock for musl
libvirt has been broken under musl since v6.3 due to how it uses malloc/free after fork causing a deadlock. When libvirt is in this state, any attempt to connect to it (virsh/virt-manager) will just hangs forever waiting for connection until a `daemon-init` process under libvirt is killed (#23724, [alpine/aports#11602](https://gitlab.alpinelinux.org/alpine/aports/-/issues/11602), [libvirt/libvirt#52](https://gitlab.com/libvirt/libvirt/-/issues/52)).
This PR backport 2 patches from Alpine 3.12 to fix the issue. Going to need some testing on glibc, though.
A patch file from https://github.com/void-linux/void-packages/pull/25905.patch is attached
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-libvirt-musl-fix-25905.patch --]
[-- Type: text/x-diff, Size: 10176 bytes --]
From c731153c0f104e826eea3ee162106f450d849b2e Mon Sep 17 00:00:00 2001
From: Kridsada Thanabulpong <sirn@ogsite.net>
Date: Tue, 27 Oct 2020 07:51:55 +0900
Subject: [PATCH] libvirt: fix deadlock for musl
---
...avoid-free-when-reset-log-after-fork.patch | 142 ++++++++++++++++++
.../improve-generic-mass-close-of-fds.patch | 134 +++++++++++++++++
srcpkgs/libvirt/template | 2 +-
3 files changed, 277 insertions(+), 1 deletion(-)
create mode 100644 srcpkgs/libvirt/patches/avoid-free-when-reset-log-after-fork.patch
create mode 100644 srcpkgs/libvirt/patches/improve-generic-mass-close-of-fds.patch
diff --git a/srcpkgs/libvirt/patches/avoid-free-when-reset-log-after-fork.patch b/srcpkgs/libvirt/patches/avoid-free-when-reset-log-after-fork.patch
new file mode 100644
index 00000000000..7f7887b85b1
--- /dev/null
+++ b/srcpkgs/libvirt/patches/avoid-free-when-reset-log-after-fork.patch
@@ -0,0 +1,142 @@
+https://www.redhat.com/archives/libvir-list/2020-August/msg00596.html
+
+Doing malloc/free after fork is technically not allowed in POSIX and
+deadlocks[1] with musl libc.
+
+[1]: https://gitlab.com/libvirt/libvirt/-/issues/52
+
+Signed-off-by: Natanael Copa <ncopa alpinelinux org>
+---
+ src/util/vircommand.c | 4 ++--
+ src/util/virlog.c | 44 +++++++++++++++++++++++++++++++++----------
+ src/util/virlog.h | 1 +
+ 3 files changed, 37 insertions(+), 12 deletions(-)
+
+diff --git a/src/util/vircommand.c b/src/util/vircommand.c
+index 76f7eb9a3d..17e5bb00d3 100644
+--- src/util/vircommand.c
++++ src/util/vircommand.c
+@@ -304,7 +304,7 @@ virFork(void)
+ /* Make sure any hook logging is sent to stderr, since child
+ * process may close the logfile FDs */
+ logprio = virLogGetDefaultPriority();
+- virLogReset();
++ virLogResetWithoutFree();
+ virLogSetDefaultPriority(logprio);
+
+ /* Clear out all signal handlers from parent so nothing
+@@ -861,7 +861,7 @@ virExec(virCommandPtr cmd)
+ goto fork_error;
+
+ /* Close logging again to ensure no FDs leak to child */
+- virLogReset();
++ virLogResetWithoutFree();
+
+ if (cmd->env)
+ execve(binary, cmd->args, cmd->env);
+diff --git a/src/util/virlog.c b/src/util/virlog.c
+index 3217e5eb73..3959de5ca7 100644
+--- src/util/virlog.c
++++ src/util/virlog.c
+@@ -108,8 +108,8 @@ static size_t virLogNbOutputs;
+ */
+ static virLogPriority virLogDefaultPriority = VIR_LOG_DEFAULT;
+
+-static void virLogResetFilters(void);
+-static void virLogResetOutputs(void);
++static void virLogResetFilters(bool freemem);
++static void virLogResetOutputs(bool freemem);
+ static void virLogOutputToFd(virLogSourcePtr src,
+ virLogPriority priority,
+ const char *filename,
+@@ -284,8 +284,30 @@ virLogReset(void)
+ return -1;
+
+ virLogLock();
+- virLogResetFilters();
+- virLogResetOutputs();
++ virLogResetFilters(true);
++ virLogResetOutputs(true);
++ virLogDefaultPriority = VIR_LOG_DEFAULT;
++ virLogUnlock();
++ return 0;
++}
++
++/**
++ * virLogResetWithoutFree:
++ *
++ * Reset the logging module to its default initial state, but avoid doing
++ * free() so it can be used after fork and before exec.
++ *
++ * Returns 0 if successful, and -1 in case or error
++ */
++int
++virLogResetWithoutFree(void)
++{
++ if (virLogInitialize() < 0)
++ return -1;
++
++ virLogLock();
++ virLogResetFilters(false);
++ virLogResetOutputs(false);
+ virLogDefaultPriority = VIR_LOG_DEFAULT;
+ virLogUnlock();
+ return 0;
+@@ -324,9 +346,10 @@ virLogSetDefaultPriority(virLogPriority priority)
+ * Removes the set of logging filters defined.
+ */
+ static void
+-virLogResetFilters(void)
++virLogResetFilters(bool freemem)
+ {
+- virLogFilterListFree(virLogFilters, virLogNbFilters);
++ if (freemem)
++ virLogFilterListFree(virLogFilters, virLogNbFilters);
+ virLogFilters = NULL;
+ virLogNbFilters = 0;
+ virLogFiltersSerial++;
+@@ -371,9 +394,10 @@ virLogFilterListFree(virLogFilterPtr *list, int count)
+ * Removes the set of logging output defined.
+ */
+ static void
+-virLogResetOutputs(void)
++virLogResetOutputs(bool freemem)
+ {
+- virLogOutputListFree(virLogOutputs, virLogNbOutputs);
++ if (freemem)
++ virLogOutputListFree(virLogOutputs, virLogNbOutputs);
+ virLogOutputs = NULL;
+ virLogNbOutputs = 0;
+ }
+@@ -1392,7 +1416,7 @@ virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
+ return -1;
+
+ virLogLock();
+- virLogResetOutputs();
++ virLogResetOutputs(true);
+
+ #if HAVE_SYSLOG_H
+ /* syslog needs to be special-cased, since it keeps the fd in private */
+@@ -1435,7 +1459,7 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters)
+ return -1;
+
+ virLogLock();
+- virLogResetFilters();
++ virLogResetFilters(true);
+ virLogFilters = filters;
+ virLogNbFilters = nfilters;
+ virLogUnlock();
+diff --git a/src/util/virlog.h b/src/util/virlog.h
+index 984a9d5a43..69f7b1ef94 100644
+--- src/util/virlog.h
++++ src/util/virlog.h
+@@ -168,6 +168,7 @@ void virLogSetDefaultOutput(const char *fname, bool godaemon, bool privileged);
+ void virLogLock(void);
+ void virLogUnlock(void);
+ int virLogReset(void);
++int virLogResetWithoutFree(void);
+ int virLogParseDefaultPriority(const char *priority);
+ int virLogPriorityFromSyslog(int priority);
+ void virLogMessage(virLogSourcePtr source,
+--
+2.28.0
diff --git a/srcpkgs/libvirt/patches/improve-generic-mass-close-of-fds.patch b/srcpkgs/libvirt/patches/improve-generic-mass-close-of-fds.patch
new file mode 100644
index 00000000000..c180fa46a57
--- /dev/null
+++ b/srcpkgs/libvirt/patches/improve-generic-mass-close-of-fds.patch
@@ -0,0 +1,134 @@
+https://www.redhat.com/archives/libvir-list/2020-August/msg00598.html
+
+Add a portable generic implementation of virMassClose as fallback on
+non-FreeBSD and non-glibc.
+
+This implementation uses poll(2) to look for open files to keep
+performance reasonable while not using any mallocs.
+
+This solves a deadlock with musl libc.
+
+Signed-off-by: Natanael Copa <ncopa alpinelinux org>
+---
+ src/util/vircommand.c | 76 +++++++++++++++++++++++++++++++++----------
+ 1 file changed, 58 insertions(+), 18 deletions(-)
+
+diff --git a/src/util/vircommand.c b/src/util/vircommand.c
+index 17e5bb00d3..06579cfb44 100644
+--- src/util/vircommand.c
++++ src/util/vircommand.c
+@@ -443,7 +443,7 @@ virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups)
+ return 0;
+ }
+
+-# ifdef __linux__
++# if defined(__linux__) && defined(__GLIBC__)
+ /* On Linux, we can utilize procfs and read the table of opened
+ * FDs and selectively close only those FDs we don't want to pass
+ * onto child process (well, the one we will exec soon since this
+@@ -482,17 +482,7 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED,
+ VIR_DIR_CLOSE(dp);
+ return ret;
+ }
+-
+-# else /* !__linux__ */
+-
+-static int
+-virCommandMassCloseGetFDsGeneric(virCommandPtr cmd G_GNUC_UNUSED,
+- virBitmapPtr fds)
+-{
+- virBitmapSetAll(fds);
+- return 0;
+-}
+-# endif /* !__linux__ */
++# endif /* __linux__ && __GLIBC__ */
+
+ # ifdef __FreeBSD__
+
+@@ -546,7 +536,7 @@ virCommandMassClose(virCommandPtr cmd,
+ return 0;
+ }
+
+-# else /* ! __FreeBSD__ */
++# elif defined(__GLIBC__) /* ! __FreeBSD__ */
+
+ static int
+ virCommandMassClose(virCommandPtr cmd,
+@@ -574,13 +564,8 @@ virCommandMassClose(virCommandPtr cmd,
+ if (!(fds = virBitmapNew(openmax)))
+ return -1;
+
+-# ifdef __linux__
+ if (virCommandMassCloseGetFDsLinux(cmd, fds) < 0)
+ return -1;
+-# else
+- if (virCommandMassCloseGetFDsGeneric(cmd, fds) < 0)
+- return -1;
+-# endif
+
+ fd = virBitmapNextSetBit(fds, 2);
+ for (; fd >= 0; fd = virBitmapNextSetBit(fds, fd)) {
+@@ -598,6 +583,61 @@ virCommandMassClose(virCommandPtr cmd,
+ return 0;
+ }
+
++#else /* ! __FreeBSD__ && ! __GLIBC__ */
++static int
++virCommandMassClose(virCommandPtr cmd,
++ int childin,
++ int childout,
++ int childerr)
++{
++ static struct pollfd pfds[1024];
++ int fd = 0;
++ int i, total;
++ int max_fd = sysconf(_SC_OPEN_MAX);
++
++ if (max_fd < 0) {
++ virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed"));
++ return -1;
++ }
++
++ total = max_fd - fd;
++ for (i = 0; i < (total < 1024 ? total : 1024); i++)
++ pfds[i].events = 0;
++
++ while (fd < max_fd) {
++ int nfds, r = 0;
++
++ total = max_fd - fd;
++ nfds = total < 1024 ? total : 1024;
++
++ for (i = 0; i < nfds; i++)
++ pfds[i].fd = fd + i;
++
++ do {
++ r = poll(pfds, nfds, 0);
++ } while (r == -1 && errno == EINTR);
++
++ if (r < 0) {
++ virReportSystemError(errno, "%s", _("poll() failed"));
++ return -1;
++ }
++
++ for (i = 0; i < nfds; i++)
++ if (pfds[i].revents != POLLNVAL) {
++ if (pfds[i].fd == childin || pfds[i].fd == childout || pfds[i].fd == childerr)
++ continue;
++ if (!virCommandFDIsSet(cmd, pfds[i].fd)) {
++ VIR_MASS_CLOSE(pfds[i].fd);
++ } else if (virSetInherit(pfds[i].fd, true) < 0) {
++ virReportSystemError(errno, _("failed to preserve fd %d"), pfds[i].fd);
++ return -1;
++ }
++ }
++ fd += nfds;
++ }
++ return 0;
++}
++
+ # endif /* ! __FreeBSD__ */
+
+ /*
+--
+22.28.0
diff --git a/srcpkgs/libvirt/template b/srcpkgs/libvirt/template
index 4da40ef692b..f7cbafb9af8 100644
--- a/srcpkgs/libvirt/template
+++ b/srcpkgs/libvirt/template
@@ -1,7 +1,7 @@
# Template file for 'libvirt'
pkgname=libvirt
version=6.9.0
-revision=1
+revision=2
build_style=meson
configure_args="-Dqemu_user=libvirt -Dqemu_group=libvirt -Drunstatedir=/run"
hostmakedepends="automake libtool perl pkg-config lvm2 parted gettext-devel
next prev parent reply other threads:[~2020-11-08 10:27 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-26 22:45 [PR PATCH] " sirn
2020-10-26 22:52 ` [PR PATCH] [Updated] " sirn
2020-11-06 17:33 ` ahesford
2020-11-08 10:27 ` sirn [this message]
2020-11-08 10:36 ` sirn
2020-11-17 20:38 ` Logarithmus
2020-11-17 20:38 ` Logarithmus
2020-11-17 20:40 ` Logarithmus
2020-12-23 17:35 ` gch1p
2021-01-02 3:41 ` [PR PATCH] [Closed]: " ahesford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201108102753.wILJC4CnaMabYtEBTbC_nCtv3S29dFAtlMA8lm7mXH4@z \
--to=sirn@users.noreply.github.com \
--cc=ml@inbox.vuxu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).