[libvirt] [RFC PATCH 0/4] Deadlock fix and some minor fixes
The first patch is a preparatory patch for the deadlock fix (patch 2). The cleanup path for 'virExecCommon' may now be superflous. Patches 3-4 are only minor fixes. Important: there may still be a deadlock for LXC (see the TODO in patch 2) Marc Hartmayer (4): util: Add virCommandGetGID and virCommandGetUID util: Fix deadlock across fork() lxc: Fixed a typo lxc: Fixed indentation src/libvirt_private.syms | 2 ++ src/lxc/lxc_container.c | 20 +++++++++++++++----- src/util/vircommand.c | 39 ++++++++++++++++++++++++++++----------- src/util/vircommand.h | 6 +++++- tests/commandtest.c | 15 ++++++++++----- 5 files changed, 60 insertions(+), 22 deletions(-) -- 2.5.5
These functions are used by an upcoming commit. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/libvirt_private.syms | 2 ++ src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 4 ++++ 3 files changed, 20 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9243c5591042..26c5ddb40505 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1506,6 +1506,8 @@ virCommandDaemonize; virCommandDoAsyncIO; virCommandExec; virCommandFree; +virCommandGetGID; +virCommandGetUID; virCommandHandshakeNotify; virCommandHandshakeWait; virCommandNew; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 60c1121dafea..fba73ca18eac 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1073,6 +1073,20 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) } +gid_t +virCommandGetGID(virCommandPtr cmd) +{ + return cmd->gid; +} + + +uid_t +virCommandGetUID(virCommandPtr cmd) +{ + return cmd->uid; +} + + void virCommandSetGID(virCommandPtr cmd, gid_t gid) { diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e7c2e513bae1..b401d7b238d7 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -68,6 +68,10 @@ int virCommandPassFDGetFDIndex(virCommandPtr cmd, void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); +gid_t virCommandGetGID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + +uid_t virCommandGetUID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + void virCommandSetGID(virCommandPtr cmd, gid_t gid); void virCommandSetUID(virCommandPtr cmd, uid_t uid); -- 2.5.5
On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
These functions are used by an upcoming commit.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/libvirt_private.syms | 2 ++ src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 4 ++++ 3 files changed, 20 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9243c5591042..26c5ddb40505 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1506,6 +1506,8 @@ virCommandDaemonize; virCommandDoAsyncIO; virCommandExec; virCommandFree; +virCommandGetGID; +virCommandGetUID; virCommandHandshakeNotify; virCommandHandshakeWait; virCommandNew; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 60c1121dafea..fba73ca18eac 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1073,6 +1073,20 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) } +gid_t +virCommandGetGID(virCommandPtr cmd) +{ + return cmd->gid; +} + + +uid_t +virCommandGetUID(virCommandPtr cmd) +{ + return cmd->uid; +} + + void virCommandSetGID(virCommandPtr cmd, gid_t gid) { diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e7c2e513bae1..b401d7b238d7 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -68,6 +68,10 @@ int virCommandPassFDGetFDIndex(virCommandPtr cmd, void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); +gid_t virCommandGetGID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + +uid_t virCommandGetUID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + void virCommandSetGID(virCommandPtr cmd, gid_t gid); void virCommandSetUID(virCommandPtr cmd, uid_t uid);
ACK. I guess the commit using those is still to come, right? -- Cedric
On Mon, Oct 09, 2017 at 10:04 PM +0200, Cedric Bosdonnat <cbosdonnat@suse.com> wrote:
On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
These functions are used by an upcoming commit.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/libvirt_private.syms | 2 ++ src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 4 ++++ 3 files changed, 20 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9243c5591042..26c5ddb40505 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1506,6 +1506,8 @@ virCommandDaemonize; virCommandDoAsyncIO; virCommandExec; virCommandFree; +virCommandGetGID; +virCommandGetUID; virCommandHandshakeNotify; virCommandHandshakeWait; virCommandNew; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 60c1121dafea..fba73ca18eac 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1073,6 +1073,20 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) } +gid_t +virCommandGetGID(virCommandPtr cmd) +{ + return cmd->gid; +} + + +uid_t +virCommandGetUID(virCommandPtr cmd) +{ + return cmd->uid; +} + + void virCommandSetGID(virCommandPtr cmd, gid_t gid) { diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e7c2e513bae1..b401d7b238d7 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -68,6 +68,10 @@ int virCommandPassFDGetFDIndex(virCommandPtr cmd, void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); +gid_t virCommandGetGID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + +uid_t virCommandGetUID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1); + void virCommandSetGID(virCommandPtr cmd, gid_t gid); void virCommandSetUID(virCommandPtr cmd, uid_t uid);
ACK.
Thanks.
I guess the commit using those is still to come, right?
Right.
-- Cedric
-- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
This commit fixes the deadlock introduced by commit 0980764dee687e8da86dc410c351759867163389. The call getgrouplist() of the glibc library isn't safe to be called in between fork and exec (see commit 75c125641ac73473ba4b0542524d67a184769c8e). Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec") Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 12 +++++++++++- src/util/vircommand.c | 25 ++++++++++++++----------- src/util/vircommand.h | 2 +- tests/commandtest.c | 15 ++++++++++----- 4 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ec6d6a86b0b6..1f220c602b0a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2182,6 +2182,8 @@ static int lxcContainerChild(void *data) virDomainFSDefPtr root; virCommandPtr cmd = NULL; int hasReboot; + gid_t *groups = NULL; + int ngroups; if (NULL == vmDef) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2297,6 +2299,13 @@ static int lxcContainerChild(void *data) goto cleanup; } + /* TODO is it safe to call it here or should this call be moved in + * front of the clone() as otherwise there might be a risk for a + * deadlock */ + if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd), + &groups)) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(ttyPath); @@ -2307,7 +2316,7 @@ static int lxcContainerChild(void *data) if (ret == 0) { VIR_DEBUG("Executing init binary"); /* this function will only return if an error occurred */ - ret = virCommandExec(cmd); + ret = virCommandExec(cmd, groups, ngroups); } if (ret != 0) { @@ -2317,6 +2326,7 @@ static int lxcContainerChild(void *data) virGetLastErrorMessage()); } + VIR_FREE(groups); virCommandFree(cmd); return ret; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index fba73ca18eac..41a61da49f82 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -465,15 +465,10 @@ virCommandHandshakeChild(virCommandPtr cmd) } static int -virExecCommon(virCommandPtr cmd) +virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups) { - gid_t *groups = NULL; - int ngroups; int ret = -1; - if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) - goto cleanup; - if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 || cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) { VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx", @@ -495,7 +490,6 @@ virExecCommon(virCommandPtr cmd) ret = 0; cleanup: - VIR_FREE(groups); return ret; } @@ -519,6 +513,8 @@ virExec(virCommandPtr cmd) const char *binary = NULL; int ret; struct sigaction waxon, waxoff; + gid_t *groups = NULL; + int ngroups; if (cmd->args[0][0] != '/') { if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) { @@ -589,6 +585,9 @@ virExec(virCommandPtr cmd) childerr = null; } + if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) + goto cleanup; + pid = virFork(); if (pid < 0) @@ -756,7 +755,7 @@ virExec(virCommandPtr cmd) } # endif - if (virExecCommon(cmd) < 0) + if (virExecCommon(cmd, groups, ngroups) < 0) goto fork_error; if (virCommandHandshakeChild(cmd) < 0) @@ -799,6 +798,7 @@ virExec(virCommandPtr cmd) should never jump here on error */ VIR_FREE(binarystr); + VIR_FREE(groups); /* NB we don't virReportError() on any failures here because the code which jumped here already raised @@ -2167,6 +2167,8 @@ virCommandProcessIO(virCommandPtr cmd) /** * virCommandExec: * @cmd: command to run + * @groups: array of supplementary group IDs used for the command + * @ngroups: number of group IDs in @groups * * Exec the command, replacing the current process. Meant to be called * in the hook after already forking / cloning, so does not attempt to @@ -2176,7 +2178,7 @@ virCommandProcessIO(virCommandPtr cmd) * Will not return on success. */ #ifndef WIN32 -int virCommandExec(virCommandPtr cmd) +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) { if (!cmd ||cmd->has_error == ENOMEM) { virReportOOMError(); @@ -2188,7 +2190,7 @@ int virCommandExec(virCommandPtr cmd) return -1; } - if (virExecCommon(cmd) < 0) + if (virExecCommon(cmd, groups, ngroups) < 0) return -1; execve(cmd->args[0], cmd->args, cmd->env); @@ -2199,7 +2201,8 @@ int virCommandExec(virCommandPtr cmd) return -1; } #else -int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED) +int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED, gid_t *groups ATTRIBUTE_UNUSED, + int ngroups ATTRIBUTE_UNUSED) { /* Mingw execve() has a broken signature. Disable this * function until gnulib fixes the signature, since we diff --git a/src/util/vircommand.h b/src/util/vircommand.h index b401d7b238d7..d59278cf5f6c 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -173,7 +173,7 @@ void virCommandWriteArgLog(virCommandPtr cmd, char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -int virCommandExec(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) ATTRIBUTE_RETURN_CHECK; int virCommandRun(virCommandPtr cmd, int *exitstatus) ATTRIBUTE_RETURN_CHECK; diff --git a/tests/commandtest.c b/tests/commandtest.c index 1f6f16bcde73..7d73f638a2e2 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1070,6 +1070,9 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) int rv = 0; ssize_t tries = 100; pid_t pid; + gid_t *groups = NULL; + int ngroups; + virCommandPtr cmd = virCommandNew("some/nonexistent/binary"); if (pipe(pipeFD) < 0) { fprintf(stderr, "Unable to create pipe\n"); @@ -1081,6 +1084,10 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } + if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd), + &groups)) < 0) + goto cleanup; + /* Now, fork and try to exec a nonexistent binary. */ pid = virFork(); if (pid < 0) { @@ -1090,11 +1097,7 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) if (pid == 0) { /* Child */ - virCommandPtr cmd = virCommandNew("some/nonexistent/binary"); - - rv = virCommandExec(cmd); - - virCommandFree(cmd); + rv = virCommandExec(cmd, groups, ngroups); if (safewrite(pipeFD[1], &rv, sizeof(rv)) < 0) fprintf(stderr, "Unable to write to pipe\n"); @@ -1129,6 +1132,8 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) cleanup: VIR_FORCE_CLOSE(pipeFD[0]); VIR_FORCE_CLOSE(pipeFD[1]); + VIR_FREE(groups); + virCommandFree(cmd); return ret; } -- 2.5.5
The commit looks good to me, but I'ld rather have Dan have a look at it. I'm not expert enough to tell what's safe and what is not. -- Cedric On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
This commit fixes the deadlock introduced by commit 0980764dee687e8da86dc410c351759867163389. The call getgrouplist() of the glibc library isn't safe to be called in between fork and exec (see commit 75c125641ac73473ba4b0542524d67a184769c8e).
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec") Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 12 +++++++++++- src/util/vircommand.c | 25 ++++++++++++++----------- src/util/vircommand.h | 2 +- tests/commandtest.c | 15 ++++++++++----- 4 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ec6d6a86b0b6..1f220c602b0a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2182,6 +2182,8 @@ static int lxcContainerChild(void *data) virDomainFSDefPtr root; virCommandPtr cmd = NULL; int hasReboot; + gid_t *groups = NULL; + int ngroups; if (NULL == vmDef) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2297,6 +2299,13 @@ static int lxcContainerChild(void *data) goto cleanup; } + /* TODO is it safe to call it here or should this call be moved in + * front of the clone() as otherwise there might be a risk for a + * deadlock */ + if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd), + &groups)) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(ttyPath); @@ -2307,7 +2316,7 @@ static int lxcContainerChild(void *data) if (ret == 0) { VIR_DEBUG("Executing init binary"); /* this function will only return if an error occurred */ - ret = virCommandExec(cmd); + ret = virCommandExec(cmd, groups, ngroups); } if (ret != 0) { @@ -2317,6 +2326,7 @@ static int lxcContainerChild(void *data) virGetLastErrorMessage()); } + VIR_FREE(groups); virCommandFree(cmd); return ret; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index fba73ca18eac..41a61da49f82 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -465,15 +465,10 @@ virCommandHandshakeChild(virCommandPtr cmd) } static int -virExecCommon(virCommandPtr cmd) +virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups) { - gid_t *groups = NULL; - int ngroups; int ret = -1; - if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) - goto cleanup; - if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 || cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) { VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx", @@ -495,7 +490,6 @@ virExecCommon(virCommandPtr cmd) ret = 0; cleanup: - VIR_FREE(groups); return ret; } @@ -519,6 +513,8 @@ virExec(virCommandPtr cmd) const char *binary = NULL; int ret; struct sigaction waxon, waxoff; + gid_t *groups = NULL; + int ngroups; if (cmd->args[0][0] != '/') { if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) { @@ -589,6 +585,9 @@ virExec(virCommandPtr cmd) childerr = null; } + if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) + goto cleanup; + pid = virFork(); if (pid < 0) @@ -756,7 +755,7 @@ virExec(virCommandPtr cmd) } # endif - if (virExecCommon(cmd) < 0) + if (virExecCommon(cmd, groups, ngroups) < 0) goto fork_error; if (virCommandHandshakeChild(cmd) < 0) @@ -799,6 +798,7 @@ virExec(virCommandPtr cmd) should never jump here on error */ VIR_FREE(binarystr); + VIR_FREE(groups); /* NB we don't virReportError() on any failures here because the code which jumped here already raised @@ -2167,6 +2167,8 @@ virCommandProcessIO(virCommandPtr cmd) /** * virCommandExec: * @cmd: command to run + * @groups: array of supplementary group IDs used for the command + * @ngroups: number of group IDs in @groups * * Exec the command, replacing the current process. Meant to be called * in the hook after already forking / cloning, so does not attempt to @@ -2176,7 +2178,7 @@ virCommandProcessIO(virCommandPtr cmd) * Will not return on success. */ #ifndef WIN32 -int virCommandExec(virCommandPtr cmd) +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) { if (!cmd ||cmd->has_error == ENOMEM) { virReportOOMError(); @@ -2188,7 +2190,7 @@ int virCommandExec(virCommandPtr cmd) return -1; } - if (virExecCommon(cmd) < 0) + if (virExecCommon(cmd, groups, ngroups) < 0) return -1; execve(cmd->args[0], cmd->args, cmd->env); @@ -2199,7 +2201,8 @@ int virCommandExec(virCommandPtr cmd) return -1; } #else -int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED) +int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED, gid_t *groups ATTRIBUTE_UNUSED, + int ngroups ATTRIBUTE_UNUSED) { /* Mingw execve() has a broken signature. Disable this * function until gnulib fixes the signature, since we diff --git a/src/util/vircommand.h b/src/util/vircommand.h index b401d7b238d7..d59278cf5f6c 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -173,7 +173,7 @@ void virCommandWriteArgLog(virCommandPtr cmd, char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -int virCommandExec(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) ATTRIBUTE_RETURN_CHECK; int virCommandRun(virCommandPtr cmd, int *exitstatus) ATTRIBUTE_RETURN_CHECK; diff --git a/tests/commandtest.c b/tests/commandtest.c index 1f6f16bcde73..7d73f638a2e2 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1070,6 +1070,9 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) int rv = 0; ssize_t tries = 100; pid_t pid; + gid_t *groups = NULL; + int ngroups; + virCommandPtr cmd = virCommandNew("some/nonexistent/binary"); if (pipe(pipeFD) < 0) { fprintf(stderr, "Unable to create pipe\n"); @@ -1081,6 +1084,10 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } + if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd), + &groups)) < 0) + goto cleanup; + /* Now, fork and try to exec a nonexistent binary. */ pid = virFork(); if (pid < 0) { @@ -1090,11 +1097,7 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) if (pid == 0) { /* Child */ - virCommandPtr cmd = virCommandNew("some/nonexistent/binary"); - - rv = virCommandExec(cmd); - - virCommandFree(cmd); + rv = virCommandExec(cmd, groups, ngroups); if (safewrite(pipeFD[1], &rv, sizeof(rv)) < 0) fprintf(stderr, "Unable to write to pipe\n"); @@ -1129,6 +1132,8 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) cleanup: VIR_FORCE_CLOSE(pipeFD[0]); VIR_FORCE_CLOSE(pipeFD[1]); + VIR_FREE(groups); + virCommandFree(cmd); return ret; }
On Mon, Oct 09, 2017 at 09:14:56PM +0200, Marc Hartmayer wrote:
This commit fixes the deadlock introduced by commit 0980764dee687e8da86dc410c351759867163389. The call getgrouplist() of the glibc library isn't safe to be called in between fork and exec (see commit 75c125641ac73473ba4b0542524d67a184769c8e).
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec") Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 12 +++++++++++- src/util/vircommand.c | 25 ++++++++++++++----------- src/util/vircommand.h | 2 +- tests/commandtest.c | 15 ++++++++++----- 4 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index ec6d6a86b0b6..1f220c602b0a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2182,6 +2182,8 @@ static int lxcContainerChild(void *data) virDomainFSDefPtr root; virCommandPtr cmd = NULL; int hasReboot; + gid_t *groups = NULL; + int ngroups;
if (NULL == vmDef) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2297,6 +2299,13 @@ static int lxcContainerChild(void *data) goto cleanup; }
+ /* TODO is it safe to call it here or should this call be moved in + * front of the clone() as otherwise there might be a risk for a + * deadlock */
Yes, clone() is equiv to fork() so it needs to be before clone()
+ if ((ngroups = virGetGroupList(virCommandGetUID(cmd), virCommandGetGID(cmd), + &groups)) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(ttyPath);
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 :|
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 1f220c602b0a..5791a9b1f958 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2167,7 +2167,7 @@ static int lxcContainerSetUserGroup(virCommandPtr cmd, * This function is run in the process clone()'d in lxcStartContainer. * Perform a number of container setup tasks: * Setup container file system - * mount container /proca + * mount container /proc * Then exec's the container init * * Returns 0 on success or -1 in case of error -- 2.5.5
On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 1f220c602b0a..5791a9b1f958 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2167,7 +2167,7 @@ static int lxcContainerSetUserGroup(virCommandPtr cmd, * This function is run in the process clone()'d in lxcStartContainer. * Perform a number of container setup tasks: * Setup container file system - * mount container /proca + * mount container /proc * Then exec's the container init * * Returns 0 on success or -1 in case of error
ACK -- Cedric
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 5791a9b1f958..b7216d6ee863 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2254,8 +2254,8 @@ static int lxcContainerChild(void *data) if (!virFileExists(vmDef->os.init)) { virReportSystemError(errno, - _("cannot find init path '%s' relative to container root"), - vmDef->os.init); + _("cannot find init path '%s' relative to container root"), + vmDef->os.init); goto cleanup; } @@ -2275,7 +2275,7 @@ static int lxcContainerChild(void *data) if (lxcContainerSendContinue(argv->handshakefd) < 0) { virReportSystemError(errno, "%s", - _("Failed to send continue signal to controller")); + _("Failed to send continue signal to controller")); goto cleanup; } -- 2.5.5
On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/lxc/lxc_container.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 5791a9b1f958..b7216d6ee863 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2254,8 +2254,8 @@ static int lxcContainerChild(void *data) if (!virFileExists(vmDef->os.init)) { virReportSystemError(errno, - _("cannot find init path '%s' relative to container root"), - vmDef->os.init); + _("cannot find init path '%s' relative to container root"), + vmDef->os.init); goto cleanup; } @@ -2275,7 +2275,7 @@ static int lxcContainerChild(void *data) if (lxcContainerSendContinue(argv->handshakefd) < 0) { virReportSystemError(errno, "%s", - _("Failed to send continue signal to controller")); + _("Failed to send continue signal to controller")); goto cleanup; }
ACK -- Cedric
On Mon, Oct 09, 2017 at 09:14:54PM +0200, Marc Hartmayer wrote:
The first patch is a preparatory patch for the deadlock fix (patch 2). The cleanup path for 'virExecCommon' may now be superflous. Patches 3-4 are only minor fixes.
Important: there may still be a deadlock for LXC (see the TODO in patch 2)
I've pushed this patch series as is - please send a followup to address the further issue in LXC 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 :|
participants (3)
-
Cedric Bosdonnat -
Daniel P. Berrange -
Marc Hartmayer