Re: [libvirt] [PATCHv2] Make domain save work on root-squash NFS

Move *all* file operations related to creation and writing of libvirt header to the domain save file into a hook function that is called by virFileOperation. First try to call virFileOperation as root. If that fails with EACCESS, and (in the case of Linux) statfs says that we're trying to save the file on an NFS share, rerun virFileOperation, telling it to fork a child process and setuid to the qemu user. This is the only way we can successfully create a file on a root-squashed NFS server. This patch (along with setting dynamic_ownership=0 in qemu.conf) makes qemudDomainSave work on root-squashed NFS. --- Note that this version of the patch avoids a problem the original had with NFS shares whose directories aren't even readable by root - in those cases, statfs would fail, so we would never learn that the file was on NFS, and just fail the entire operation. The solution is to repeatedly call statfs on a smaller and smaller part of the full path until it succeeds - this will at most continue until the mount point of the share, which will properly report the fstype for the share, rather than for the mount point itself. src/qemu/qemu_driver.c | 165 ++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 139 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e4b493..da92cf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -47,6 +47,11 @@ #include <sys/ioctl.h> #include <sys/un.h> +#ifdef __linux__ +#include <sys/vfs.h> +#include <linux/magic.h> +#endif + #include "virterror_internal.h" #include "logging.h" #include "datatypes.h" @@ -3956,14 +3961,44 @@ struct qemud_save_header { int unused[15]; }; +struct fileOpHookData { + virDomainPtr dom; + const char *path; + char *xml; + struct qemud_save_header *header; +}; + +static int qemudDomainSaveFileOpHook(int fd, void *data) { + struct fileOpHookData *hdata = data; + int ret = 0; + + if (safewrite(fd, hdata->header, sizeof(*hdata->header)) != sizeof(*hdata->header)) { + ret = errno; + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to write save header to '%s'"), hdata->path); + goto endjob; + } + + if (safewrite(fd, hdata->xml, hdata->header->xml_len) != hdata->header->xml_len) { + ret = errno; + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to write xml to '%s'"), hdata->path); + goto endjob; + } +endjob: + return ret; +} + + static int qemudDomainSave(virDomainPtr dom, const char *path) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - int fd = -1; char *xml = NULL; struct qemud_save_header header; + struct fileOpHookData hdata; + int bypassSecurityDriver = 0; int ret = -1; int rc; virDomainEventPtr event = NULL; @@ -4027,34 +4062,113 @@ static int qemudDomainSave(virDomainPtr dom, } header.xml_len = strlen(xml) + 1; + /* Setup hook data needed by virFileOperation hook function */ + hdata.dom = dom; + hdata.path = path; + hdata.xml = xml; + hdata.header = &header; + /* Write header to file, followed by XML */ - if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create '%s'"), path); - goto endjob; - } - if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("failed to write save header")); - goto endjob; - } + /* First try creating the file as root */ + if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY, + S_IRUSR|S_IWUSR, + getuid(), getgid(), + qemudDomainSaveFileOpHook, &hdata, + 0)) != 0) { + + /* If we failed as root, and the error was permission-denied + (EACCES), 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 ((rc != EACCES) || + driver->user == getuid()) { + virReportSystemError(rc, _("Failed to create domain save file '%s'"), + path); + goto endjob; + } - if (safewrite(fd, xml, header.xml_len) != header.xml_len) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("failed to write xml")); - goto endjob; - } +#ifdef __linux__ + /* On Linux we can also verify the FS-type of the directory. */ + char *dirpath, *p; + struct statfs st; + int statfs_ret; - if (close(fd) < 0) { - virReportSystemError(errno, - _("unable to save file %s"), - path); - goto endjob; + if ((dirpath = strdup(path)) == NULL) { + virReportOOMError(); + goto endjob; + } + + do { + // Try less and less of the path until we get to a + // directory we can stat. Even if we don't have 'x' + // permission on any directory in the path on the NFS + // server (assuming it's NFS), we will be able to stat the + // mount point, and that will properly tell us if the + // fstype is NFS. + + if ((p = strrchr(dirpath, '/')) == NULL) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Invalid relative path '%s' for domain save file"), + path); + VIR_FREE(dirpath); + goto endjob; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0'; + + statfs_ret = statfs(dirpath, &st); + + } while ((statfs_ret == -1) && (p != dirpath)); + + if (statfs_ret == -1) { + virReportSystemError(errno, + _("Failed to create domain save file '%s'" + " statfs of all elements of path failed."), + path); + VIR_FREE(dirpath); + goto endjob; + } + + if (st.f_type != NFS_SUPER_MAGIC) { + virReportSystemError(rc, + _("Failed to create domain save file '%s'" + " (fstype of '%s' is 0x%X"), + path, dirpath, st.f_type); + VIR_FREE(dirpath); + goto endjob; + } + VIR_FREE(dirpath); +#endif + + /* Retry creating the file as driver->user */ + + if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, + driver->user, driver->group, + qemudDomainSaveFileOpHook, &hdata, + VIR_FILE_OP_AS_UID)) != 0) { + virReportSystemError(rc, _("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 = 1; } - fd = -1; - if (driver->securityDriver && + + if ((!bypassSecurityDriver) && + driver->securityDriver && driver->securityDriver->domainSetSavedStateLabel && driver->securityDriver->domainSetSavedStateLabel(vm, path) == -1) goto endjob; @@ -4081,7 +4195,8 @@ static int qemudDomainSave(virDomainPtr dom, if (rc < 0) goto endjob; - if (driver->securityDriver && + if ((!bypassSecurityDriver) && + driver->securityDriver && driver->securityDriver->domainRestoreSavedStateLabel && driver->securityDriver->domainRestoreSavedStateLabel(vm, path) == -1) goto endjob; @@ -4106,8 +4221,6 @@ endjob: vm = NULL; cleanup: - if (fd != -1) - close(fd); VIR_FREE(xml); if (ret != 0) unlink(path);