From 449ae9c2f1d675785f9d336ddbef0695085779a6 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 23 Aug 2011 14:01:51 -0600 Subject: [PATCH] qemu: refactor file opening In a SELinux or root-squashing NFS environment, libvirt has to go through some hoops to create a new file that qemu can then open() by name. Snapshots are a case where we want to guarantee an empty file that qemu can open; also, reopening a save file to convert it from being marked partial to complete requires a reopen to avoid O_DIRECT headaches. Refactor some existing code to make it easier to reuse in later patches. * src/qemu/qemu_migration.h (qemuMigrationToFile): Drop parameter. * src/qemu/qemu_migration.c (qemuMigrationToFile): Let cgroup do the stat, rather than asking caller to do it and pass info down. * src/qemu/qemu_driver.c (qemuOpenFile): New function, pulled from... (qemuDomainSaveInternal): ...here. (doCoreDump, qemuDomainSaveImageOpen): Use it here as well. --- src/qemu/qemu_driver.c | 248 +++++++++++++++++++------------------- src/qemu/qemu_migration.c | 13 +- src/qemu/qemu_migration.h | 2 +- 3 files changed, 133 insertions(+), 130 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eca32b4bbd..4e4c59c96d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2191,6 +2191,113 @@ qemuCompressProgramName(int compress) qemudSaveCompressionTypeToString(compress)); } +/* Internal function to properly create or open existing files, with + * ownership affected by qemu driver setup. */ +static int +qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver) +{ + struct stat sb; + bool is_reg = true; + bool need_unlink = false; + bool bypass_security = false; + int fd = -1; + uid_t uid = getuid(); + gid_t gid = getgid(); + + /* path might be a pre-existing block dev, in which case + * we need to skip the create step, and also avoid unlink + * in the failure case */ + if (oflags & O_CREAT) { + need_unlink = true; + if (stat(path, &sb) == 0) { + is_reg = !!S_ISREG(sb.st_mode); + /* If the path is regular file which exists + * already and dynamic_ownership is off, we don't + * want to change it's ownership, just open it as-is */ + if (is_reg && !driver->dynamicOwnership) { + uid = sb.st_uid; + gid = sb.st_gid; + } + } + } + + /* First try creating the file as root */ + if (!is_reg) { + fd = open(path, oflags & ~O_CREAT); + if (fd < 0) { + virReportSystemError(errno, _("unable to open %s"), path); + goto cleanup; + } + } else { + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, + uid, gid, 0)) < 0) { + /* If we failed as root, and the error was permission-denied + (EACCES or EPERM), assume it's on a network-connected share + where root access is restricted (eg, root-squashed NFS). If the + qemu user (driver->user) is non-root, just set a flag to + bypass security driver shenanigans, and retry the operation + after doing setuid to qemu user */ + if ((fd != -EACCES && fd != -EPERM) || + driver->user == getuid()) { + virReportSystemError(-fd, + _("Failed to create file '%s'"), + path); + goto cleanup; + } + + /* On Linux we can also verify the FS-type of the directory. */ + switch (virStorageFileIsSharedFS(path)) { + case 1: + /* it was on a network share, so we'll continue + * as outlined above + */ + break; + + case -1: + virReportSystemError(errno, + _("Failed to create file " + "'%s': couldn't determine fs type"), + path); + goto cleanup; + + case 0: + default: + /* local file - log the error returned by virFileOpenAs */ + virReportSystemError(-fd, + _("Failed to create file '%s'"), + path); + goto cleanup; + } + + /* Retry creating the file as driver->user */ + + if ((fd = virFileOpenAs(path, oflags, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, + driver->user, driver->group, + VIR_FILE_OPEN_AS_UID)) < 0) { + virReportSystemError(-fd, + _("Error from child process creating '%s'"), + path); + goto cleanup; + } + + /* Since we had to setuid to create the file, and the fstype + is NFS, we assume it's a root-squashing NFS share, and that + the security driver stuff would have failed anyway */ + + bypass_security = true; + } + } +cleanup: + if (needUnlink) + *needUnlink = need_unlink; + if (bypassSecurityDriver) + *bypassSecurityDriver = bypass_security; + + return fd; +} + /* This internal function expects the driver lock to already be held on * entry and the vm must be active + locked. Vm will be unlocked and * potentially free'd after this returns (eg transient VMs are freed @@ -2209,14 +2316,11 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, int rc; virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; - struct stat sb; - bool is_reg = false; + bool needUnlink = false; size_t len; unsigned long long offset; unsigned long long pad; int fd = -1; - uid_t uid = getuid(); - gid_t gid = getgid(); int directFlag = 0; virFileDirectFdPtr directFd = NULL; @@ -2304,107 +2408,18 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, header.xml_len = len; /* Obtain the file handle. */ - /* path might be a pre-existing block dev, in which case - * we need to skip the create step, and also avoid unlink - * in the failure case */ - if (stat(path, &sb) < 0) { - /* Avoid throwing an error here, since it is possible - * that with NFS we can't actually stat() the file. - * The subsequent codepaths will still raise an error - * if a truly fatal problem is hit */ - is_reg = true; - } else { - is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular file which exists - * already and dynamic_ownership is off, we don't - * want to change it's ownership, just open it as-is */ - if (is_reg && !driver->dynamicOwnership) { - uid=sb.st_uid; - gid=sb.st_gid; - } - } - - /* First try creating the file as root */ if (bypass_cache) { directFlag = virFileDirectFdFlag(); if (directFlag < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); - goto endjob; - } - } - if (!is_reg) { - fd = open(path, O_WRONLY | O_TRUNC | directFlag); - if (fd < 0) { - virReportSystemError(errno, _("unable to open %s"), path); - goto endjob; - } - } else { - int oflags = O_CREAT | O_TRUNC | O_WRONLY | directFlag; - if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, - uid, gid, 0)) < 0) { - /* If we failed as root, and the error was permission-denied - (EACCES or EPERM), assume it's on a network-connected share - where root access is restricted (eg, root-squashed NFS). If the - qemu user (driver->user) is non-root, just set a flag to - bypass security driver shenanigans, and retry the operation - after doing setuid to qemu user */ - rc = fd; - if (((rc != -EACCES) && (rc != -EPERM)) || - driver->user == getuid()) { - virReportSystemError(-rc, - _("Failed to create domain save file '%s'"), - path); - goto endjob; - } - - /* On Linux we can also verify the FS-type of the directory. */ - switch (virStorageFileIsSharedFS(path)) { - case 1: - /* it was on a network share, so we'll continue - * as outlined above - */ - break; - - case -1: - virReportSystemError(errno, - _("Failed to create domain save file " - "'%s': couldn't determine fs type"), - path); - goto endjob; - break; - - case 0: - default: - /* local file - log the error returned by virFileOpenAs */ - virReportSystemError(-rc, - _("Failed to create domain save file '%s'"), - path); - goto endjob; - break; - - } - - /* Retry creating the file as driver->user */ - - if ((fd = virFileOpenAs(path, oflags, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, - driver->user, driver->group, - VIR_FILE_OPEN_AS_UID)) < 0) { - virReportSystemError(-fd, - _("Error from child process creating '%s'"), - path); - goto endjob; - } - - /* Since we had to setuid to create the file, and the fstype - is NFS, we assume it's a root-squashing NFS share, and that - the security driver stuff would have failed anyway */ - - bypassSecurityDriver = true; + goto cleanup; } } - + fd = qemuOpenFile(driver, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, + &needUnlink, &bypassSecurityDriver); + if (fd < 0) + goto endjob; if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) goto endjob; @@ -2417,7 +2432,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, /* Perform the migration */ if (qemuMigrationToFile(driver, vm, fd, offset, path, qemuCompressProgramName(compressed), - is_reg, bypassSecurityDriver, + bypassSecurityDriver, QEMU_ASYNC_JOB_SAVE) < 0) goto endjob; if (VIR_CLOSE(fd) < 0) { @@ -2461,7 +2476,7 @@ cleanup: VIR_FORCE_CLOSE(fd); virFileDirectFdFree(directFd); VIR_FREE(xml); - if (ret != 0 && is_reg) + if (ret != 0 && needUnlink) unlink(path); if (event) qemuDomainEventQueue(driver, event); @@ -2705,18 +2720,19 @@ doCoreDump(struct qemud_driver *driver, goto cleanup; } } - if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, - S_IRUSR | S_IWUSR)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create '%s'"), path); + /* Core dumps usually imply last-ditch analysis efforts are + * desired, so we intentionally do not unlink even if a file was + * created. */ + if ((fd = qemuOpenFile(driver, path, + O_CREAT | O_TRUNC | O_WRONLY | directFlag, + NULL, NULL)) < 0) goto cleanup; - } if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) goto cleanup; if (qemuMigrationToFile(driver, vm, fd, 0, path, - qemuCompressProgramName(compress), true, false, + qemuCompressProgramName(compress), false, QEMU_ASYNC_JOB_DUMP) < 0) goto cleanup; @@ -3778,25 +3794,9 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, } oflags |= directFlag; } - if ((fd = virFileOpenAs(path, oflags, 0, - getuid(), getgid(), 0)) < 0) { - if ((fd != -EACCES && fd != -EPERM) || - driver->user == getuid()) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("cannot read domain image")); - goto error; - } - /* Opening as root failed, but qemu runs as a different user - * that might have better luck. */ - if ((fd = virFileOpenAs(path, oflags, 0, - driver->user, driver->group, - VIR_FILE_OPEN_AS_UID)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("cannot read domain image")); - goto error; - } - } + if ((fd = qemuOpenFile(driver, path, oflags, NULL, NULL)) < 0) + goto error; if (bypass_cache && (*directFd = virFileDirectFdNew(&fd, path)) == NULL) goto error; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 38b05a9bff..d239cc8a1b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2690,7 +2690,7 @@ int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, - bool is_reg, bool bypassSecurityDriver, + bool bypassSecurityDriver, enum qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2713,11 +2713,11 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, bypassSecurityDriver = true; } else { /* Phooey - we have to fall back on exec migration, where qemu - * has to popen() the file by name. We might also stumble on + * has to popen() the file by name, and block devices have to be + * given cgroup ACL permission. We might also stumble on * a race present in some qemu versions where it does a wait() * that botches pclose. */ - if (!is_reg && - qemuCgroupControllerActive(driver, + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -2729,7 +2729,10 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, rc = virCgroupAllowDevicePath(cgroup, path, VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc); - if (rc < 0) { + if (rc == 1) { + /* path was not a device, no further need for cgroup */ + virCgroupFree(&cgroup); + } else if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), path, vm->def->name); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 5c6921db53..ace411dfcf 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -143,7 +143,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver, int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, - bool is_reg, bool bypassSecurityDriver, + bool bypassSecurityDriver, enum qemuDomainAsyncJob asyncJob) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; -- GitLab