[libvirt] [PATCH 0/1] - for comments/help only, nearly working root-squash-domain-save
(This patch requires that you first apply the virFileOperation patches that I sent to the list yesterday.) I'm hoping someone else's keen eye can see what I'm failing to see here. The patch following this message makes domain save "work" on root-squash NFS (both the libvirt header and the QEMU state are saved to the file), but the files that it saves don't restore properly, even when simply saving to a local directory w/o forking - the guest screen comes up black, or reboots immediately, or some other such thing. I've looked at the data files and the code, and can't see what difference is causing this - at least the header written by libvirt seems to be the same, and the part written by qemu starts in the same place and looks similar. If anyone's got an idea, please let me know! I'd like to get this figured out and a working patch submitted before the day is done.
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. --- src/qemu/qemu_driver.c | 151 +++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 125 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0fb2413..4d3784c 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,99 @@ 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. */ + struct statfs st; + char *dirpath, *p; - if (close(fd) < 0) { - virReportSystemError(errno, - _("unable to save file %s"), - path); - goto endjob; + if ((dirpath = strdup(path)) == NULL) { + virReportOOMError(); + goto endjob; + } + + if ((p = strrchr(dirpath, '/')) == NULL) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Invalid path '%s' for domain save file"), + path); + VIR_FREE(dirpath); + goto endjob; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0'; + + if (statfs(dirpath, &st) == -1) { + virReportSystemError(rc, + _("Failed to create domain save file '%s'" + " statfs of '%s' failed (%d)"), + path, errno); + VIR_FREE(dirpath); + goto endjob; + } + + VIR_FREE(dirpath); + + if (st.f_type != NFS_SUPER_MAGIC) { + virReportSystemError(rc, + _("Failed to create domain save file '%s' (fstype 0x%X"), + path, st.f_type); + goto endjob; + } +#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 +4181,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 +4207,6 @@ endjob: vm = NULL; cleanup: - if (fd != -1) - close(fd); VIR_FREE(xml); if (ret != 0) unlink(path); -- 1.6.6.1
On Fri, Feb 19, 2010 at 02:30:08AM -0500, Laine Stump wrote:
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. [...] +#ifdef __linux__ + /* On Linux we can also verify the FS-type of the directory. */ + struct statfs st; + char *dirpath, *p; [...] + + if ((p = strrchr(dirpath, '/')) == NULL) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Invalid path '%s' for domain save file"), + path); + VIR_FREE(dirpath); + goto endjob; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0';
I wasn't sure why that was needed but since the man page states "path is the pathname of any file within the mounted file system" so okay we have to get back to the directory which is an existing file.
+ if (statfs(dirpath, &st) == -1) { + virReportSystemError(rc, + _("Failed to create domain save file '%s'" + " statfs of '%s' failed (%d)"), + path, errno); + VIR_FREE(dirpath); + goto endjob; + } + + VIR_FREE(dirpath); + + if (st.f_type != NFS_SUPER_MAGIC) { + virReportSystemError(rc, + _("Failed to create domain save file '%s' (fstype 0x%X"), + path, st.f_type); + goto endjob; + }
Okay, we really need to error out if not on NFS (I can't think of another kind of filesystem which would exhibit the same behaviour)
+#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 +4181,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 +4207,6 @@ endjob: vm = NULL;
cleanup: - if (fd != -1) - close(fd); VIR_FREE(xml); if (ret != 0) unlink(path);
Okay, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
FYI, I just verified that the restore failures I was seeing after applying this patch were actually happening *without* the patch as well, and are unrelated to domain save (it's a race condition in domain restore that needs to be dealt with separately), so this patch is okay to put in I verified I've been testing with an unmodified form of this patch, *EXCEPT* that I hadn't done make syntax-check on it (since I didn't really think that it was working code at the time ;-)), and there is one occurence of white-space at the end of a line. Should I resend with that change? Or do you want to just fix it up? Also, notice that this patch saves the domain file with 0660 permission (umask will normally turn it into 0640) as we had thought that would be part of the way to allow restore from a root-squashed NFS server (just make sure that the reader had group read permissions). Now it seems we will be using the trick of running the restore code setuid instead, so the 0660 mode will no longer necessary. Should I revise this patch to create the file as 0600, or just do that as part of the upcoming domain restore patch? (a couple more comments are inline) On 02/22/2010 11:57 AM, Daniel Veillard wrote:
On Fri, Feb 19, 2010 at 02:30:08AM -0500, Laine Stump wrote:
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.
[...]
+#ifdef __linux__ + /* On Linux we can also verify the FS-type of the directory. */ + struct statfs st; + char *dirpath, *p;
[...]
+ + if ((p = strrchr(dirpath, '/')) == NULL) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Invalid path '%s' for domain save file"), + path); + VIR_FREE(dirpath); + goto endjob; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0';
I wasn't sure why that was needed but since the man page states
"path is the pathname of any file within the mounted file system"
so okay we have to get back to the directory which is an existing file.
Heh. Yeah, I tried to do it with the filename first, and that didn't work, so we're stuck with the silly char* calisthenics.
+ if (statfs(dirpath,&st) == -1) { + virReportSystemError(rc, + _("Failed to create domain save file '%s'" + " statfs of '%s' failed (%d)"), + path, errno); + VIR_FREE(dirpath); + goto endjob; + } + + VIR_FREE(dirpath); + + if (st.f_type != NFS_SUPER_MAGIC) { + virReportSystemError(rc, + _("Failed to create domain save file '%s' (fstype 0x%X"), + path, st.f_type); + goto endjob; + }
Okay, we really need to error out if not on NFS (I can't think of another kind of filesystem which would exhibit the same behaviour)
There is a bugzilla report that talks about similar behavior on an smbfs share (they were having problems with storage volumes, but it seemed to smell the same) but I haven't tried it out. If it turns out to work similarly, we can add a case for SMBFS_SUPER_MAGIC. My problem with this piece of code is that it's Linux-only. Other systems either don't have statfs, or it is implemented differently and doesn't have the f_type property. It would be nice if there was a method that worked the same on all platforms...
+#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 +4181,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 +4207,6 @@ endjob: vm = NULL;
cleanup: - if (fd != -1) - close(fd); VIR_FREE(xml); if (ret != 0) unlink(path);
Okay, ACK,
Daniel
On Mon, Feb 22, 2010 at 01:09:52PM -0500, Laine Stump wrote:
FYI, I just verified that the restore failures I was seeing after applying this patch were actually happening *without* the patch as well, and are unrelated to domain save (it's a race condition in domain restore that needs to be dealt with separately), so this patch is okay to put in
I verified I've been testing with an unmodified form of this patch, *EXCEPT* that I hadn't done make syntax-check on it (since I didn't really think that it was working code at the time ;-)), and there is one occurence of white-space at the end of a line.
Should I resend with that change? Or do you want to just fix it up?
No, that's fine.
Also, notice that this patch saves the domain file with 0660 permission (umask will normally turn it into 0640) as we had thought that would be part of the way to allow restore from a root-squashed NFS server (just make sure that the reader had group read permissions). Now it seems we will be using the trick of running the restore code setuid instead, so the 0660 mode will no longer necessary. Should I revise this patch to create the file as 0600, or just do that as part of the upcoming domain restore patch?
0660 will be OK, since we setgid() too. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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); -- 1.6.6.1
On Mon, Mar 01, 2010 at 11:13:26AM -0500, Laine Stump wrote:
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);
ACK, we need this one too for root-squash NFS save, I just rebased it a bit, the cleanup part of the patch and the virReportSystemError raised a small cast problem for %X, Pushed too, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump