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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list