On Wed, Mar 18, 2020 at 06:32:11PM +0100, Michal Privoznik wrote:

The virFileMakePathWithMode() which is our recursive version of mkdir() fails, it simply just returns a negative value with errno set. No error is reported (as compared to virFileTouch() for instance).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0e2252f6cf..48bf5ae559 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -14643,8 +14643,12 @@ qemuDomainCreateDeviceRecursive(const char *device, * proper owner and mode. Bind mount only after that. */ } else if (isDir) { if (create && - virFileMakePathWithMode(devicePath, sb.st_mode) < 0) + virFileMakePathWithMode(devicePath, sb.st_mode) < 0) { + virReportSystemError(errno, + _("Unable to make dir %s"), + devicePath); goto cleanup; + } } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("unsupported device type %s 0%o"), -- 2.24.1

Reviewed-by: Pavel Mores <pmores@redhat.com>

On Wed, Mar 18, 2020 at 06:32:12PM +0100, Michal Privoznik wrote:

If building namespace fails somewhere in the middle (that is some files exists under devMountsSavePath[i]), then plain rmdir() is not enough to remove dir. Umount the temp location and use virFileDeleteTree() to remove the directory.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 48bf5ae559..2724607311 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -15311,9 +15311,12 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, ret = 0; cleanup: for (i = 0; i < ndevMountsPath; i++) { +#if defined(__linux__) + umount(devMountsSavePath[i]); +#endif /* defined(__linux__) */ /* The path can be either a regular file or a dir. */ if (virFileIsDir(devMountsSavePath[i])) - rmdir(devMountsSavePath[i]); + virFileDeleteTree(devMountsSavePath[i]); else unlink(devMountsSavePath[i]); } -- 2.24.1

Reviewed-by: Pavel Mores <pmores@redhat.com>

On Wed, Mar 18, 2020 at 06:32:13PM +0100, Michal Privoznik wrote:

The @devPath variable is not modifiable. It merely just points to string containing path where private devtmpfs is being constructed. Make it const so it doesn't look weird that it's not freed.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2724607311..a4c07fffcb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -15176,7 +15176,7 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm) { struct qemuDomainCreateDeviceData data; - char *devPath = NULL; + const char *devPath = NULL; char **devMountsPath = NULL, **devMountsSavePath = NULL; size_t ndevMountsPath = 0, i; int ret = -1; -- 2.24.1

Reviewed-by: Pavel Mores <pmores@redhat.com>

On Wed, Mar 18, 2020 at 06:32:14PM +0100, Michal Privoznik wrote:

The @src is not always a file. It may also be a directory (for instance qemuDomainCreateDeviceRecursive() assumes that) - even though it doesn't happen usually. Anyway, mount() can mount only a dir onto a dir and a file onto a file.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c index 0f31554323..9c175b041f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3743,8 +3743,17 @@ int virFileBindMountDevice(const char *src, const char *dst) { - if (virFileTouch(dst, 0666) < 0) - return -1; + if (!virFileExists(dst)) { + if (virFileIsDir(src)) { + if (virFileMakePath(dst)) {

Personally, I'd consider if (virFileMakePath(dst) < 0) clearer here for a reader who's not familiar with the function and what exactly it returns. Reviewed-by: Pavel Mores <pmores@redhat.com>

+ virReportSystemError(errno, _("Unable to make dir %s"), dst); + return -1; + } + } else { + if (virFileTouch(dst, 0666) < 0) + return -1; + } + }

if (mount(src, dst, "none", MS_BIND, NULL) < 0) { virReportSystemError(errno, _("Failed to bind %s on to %s"), src, -- 2.24.1

On Wed, Mar 18, 2020 at 06:32:15PM +0100, Michal Privoznik wrote:

When running a function in a forked child, so far the only thing we could report is exit status of the child and the error message. However, it may be beneficial to the caller to know the actual error that happened in the child.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 2 +- src/util/virprocess.c | 51 ++++++++++++++++++++++++++++++++++++--- tests/commandtest.c | 43 +++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 3020921be8..2a38c03ba9 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1987,7 +1987,7 @@ exclude_file_name_regexp--sc_flags_usage = \ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/)

-exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$) +exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$|tests/commandtest.c$$)

exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ ^(build-aux/syntax-check\.mk|include/libvirt/virterror\.h|src/remote/remote_daemon_dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 24135070b7..e8674402f9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1126,6 +1126,23 @@ virProcessRunInMountNamespace(pid_t pid G_GNUC_UNUSED,

#ifndef WIN32 +typedef struct { + int code; + int domain; + char message[VIR_ERROR_MAX_LENGTH]; + virErrorLevel level; + char str1[VIR_ERROR_MAX_LENGTH]; + char str2[VIR_ERROR_MAX_LENGTH]; + /* str3 doesn't seem to be used. Ignore it. */ + int int1; + int int2; +} errorData; + +typedef union { + errorData data; + char bindata[sizeof(errorData)]; +} errorDataBin; + static int virProcessRunInForkHelper(int errfd, pid_t ppid, @@ -1134,9 +1151,19 @@ virProcessRunInForkHelper(int errfd, { if (cb(ppid, opaque) < 0) { virErrorPtr err = virGetLastError(); + errorDataBin bin = { 0 }; + if (err) { - size_t len = strlen(err->message) + 1; - ignore_value(safewrite(errfd, err->message, len)); + bin.data.code = err->code; + bin.data.domain = err->domain; + ignore_value(virStrcpy(bin.data.message, err->message, sizeof(bin.data.message))); + bin.data.level = err->level; + ignore_value(virStrcpy(bin.data.str1, err->str1, sizeof(bin.data.str1))); + ignore_value(virStrcpy(bin.data.str2, err->str2, sizeof(bin.data.str2))); + bin.data.int1 = err->int1; + bin.data.int2 = err->int2; + + ignore_value(safewrite(errfd, bin.bindata, sizeof(bin))); }

return -1; @@ -1188,16 +1215,32 @@ virProcessRunInFork(virProcessForkCallback cb, } else { int status; g_autofree char *buf = NULL; + errorDataBin bin; + int nread;

VIR_FORCE_CLOSE(errfd[1]); - ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); + nread = virFileReadHeaderFD(errfd[0], sizeof(bin), &buf); ret = virProcessWait(child, &status, false); if (ret == 0) { ret = status == EXIT_CANCELED ? -1 : status; if (ret) { virReportError(VIR_ERR_INTERNAL_ERROR, _("child reported (status=%d): %s"), - status, NULLSTR(buf)); + status, NULLSTR(bin.data.message)); + + if (nread == sizeof(bin)) { + memcpy(bin.bindata, buf, sizeof(bin)); + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, + bin.data.domain, + bin.data.code, + bin.data.level, + bin.data.str1, + bin.data.str2, + NULL, + bin.data.int1, + bin.data.int2, + "%s", bin.data.message); + } } } } diff --git a/tests/commandtest.c b/tests/commandtest.c index a64aa9ad33..f4a2c67c05 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED) }

+static int +test28Callback(pid_t pid G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ + virReportSystemError(ENODATA, "%s", "some error message"); + return -1; +} + + +static int +test28(const void *unused G_GNUC_UNUSED) +{ + /* Not strictly a virCommand test, but this is the easiest place + * to test this lower-level interface. */ + virErrorPtr err; + + if (virProcessRunInFork(test28Callback, NULL) != -1) { + fprintf(stderr, "virProcessRunInFork did not fail\n"); + return -1; + } + + if (!(err = virGetLastError())) { + fprintf(stderr, "Expected error but got nothing\n"); + return -1; + } + + if (!(err->code == VIR_ERR_SYSTEM_ERROR && + err->domain == 0 && + STREQ(err->message, "some error message: No data available") && + err->level == VIR_ERR_ERROR && + STREQ(err->str1, "%s") && + STREQ(err->str2, "some error message: No data available") && + err->int1 == ENODATA && + err->int2 == -1)) { + fprintf(stderr, "Unexpected error object\n"); + return -1; + } + + return 0; +} + + static int mymain(void) { @@ -1354,6 +1396,7 @@ mymain(void) DO_TEST(test25); DO_TEST(test26); DO_TEST(test27); + DO_TEST(test28);

return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.24.1

Reviewed-by: Pavel Mores <pmores@redhat.com>

On Wed, Mar 18, 2020 at 06:32:15PM +0100, Michal Privoznik wrote:

When running a function in a forked child, so far the only thing we could report is exit status of the child and the error message. However, it may be beneficial to the caller to know the actual error that happened in the child.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 2 +- src/util/virprocess.c | 51 ++++++++++++++++++++++++++++++++++++--- tests/commandtest.c | 43 +++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index a64aa9ad33..f4a2c67c05 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED) }

+static int +test28Callback(pid_t pid G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ + virReportSystemError(ENODATA, "%s", "some error message"); + return -1; +} + + +static int +test28(const void *unused G_GNUC_UNUSED) +{ + /* Not strictly a virCommand test, but this is the easiest place + * to test this lower-level interface. */ + virErrorPtr err; + + if (virProcessRunInFork(test28Callback, NULL) != -1) { + fprintf(stderr, "virProcessRunInFork did not fail\n"); + return -1; + } + + if (!(err = virGetLastError())) { + fprintf(stderr, "Expected error but got nothing\n"); + return -1; + } + + if (!(err->code == VIR_ERR_SYSTEM_ERROR && + err->domain == 0 && + STREQ(err->message, "some error message: No data available") && + err->level == VIR_ERR_ERROR && + STREQ(err->str1, "%s") && + STREQ(err->str2, "some error message: No data available") && + err->int1 == ENODATA && + err->int2 == -1)) { + fprintf(stderr, "Unexpected error object\n"); + return -1; + }

This new test fails on FreeBSD $ VIR_TEST_DEBUG=1 VIR_TEST_RANGE=28 ./commandtest TEST: commandtest 28) Command Exec test28 test ... Unexpected error object libvirt: error : some error message: Input/output error FAILED IIUC the problem here is that the STREQ check is assuming that the ENODATA errno results in the string "No data available". The strings are not standardized by POSIX AFAIK, so C libraries can use any reasonable text for them. So this check is not portable. Perhaps its enough to use STRPREFIX(err->str2, "some error message:") ? 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 :|

On Wed, Mar 18, 2020 at 06:32:16PM +0100, Michal Privoznik wrote:

When a QEMU process dies in the middle of a hotplug, then we fail to restore the seclabels on the device. The problem is that if the thread doing hotplug locks the domain object first and thus blocks the thread that wants to do qemuProcessStop(), the seclabel cleanup code will see vm->pid still set and mount namespace used and therefore try to enter the namespace represented by the PID. But the PID is gone really and thus entering will fail and no restore is done. What we can do is to try enter the namespace (if requested to do so) but if entering fails, fall back to no NS mode.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814481

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 16 ++++++++++++---- src/security/security_selinux.c | 16 ++++++++++++---- 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9046b51004..11fff63bc7 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -640,15 +640,23 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,

list->lock = lock;

+ if (pid != -1) { + rc = virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list); + if (rc < 0) { + if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR) + pid = -1; + else + goto cleanup; + } + } + if (pid == -1) { if (lock) rc = virProcessRunInFork(virSecurityDACTransactionRun, list); else rc = virSecurityDACTransactionRun(pid, list); - } else { - rc = virProcessRunInMountNamespace(pid, - virSecurityDACTransactionRun, - list); }

if (rc < 0) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c94f31727c..8aeb6e45a5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1163,15 +1163,23 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,

list->lock = lock;

+ if (pid != -1) { + rc = virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list); + if (rc < 0) { + if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR) + pid = -1; + else + goto cleanup; + } + } + if (pid == -1) { if (lock) rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list); else rc = virSecuritySELinuxTransactionRun(pid, list); - } else { - rc = virProcessRunInMountNamespace(pid, - virSecuritySELinuxTransactionRun, - list); }

if (rc < 0) -- 2.24.1

Reviewed-by: Pavel Mores <pmores@redhat.com>